Comment formatter

Question

private string FormatComments(string comments)
{
 //If comments has no space and inserted as a single line then following code will break
 //the comments into multiple lines.
 StringBuilder sb = new StringBuilder();
 int c = 65;
 int l = comments.Length;
 if (l < c)
 return comments;
 int r = l % c;
 int lines = (l - r) / c;
 if (lines == 0)
 return comments;
 int index = 0;
 for (int i = 0; i < lines; i++)
 {
 if (index > comments.Length)
 break;
 string indexVal = comments[index + c].ToString();
 sb = sb.AppendLine(comments.Substring(index, c) + (indexVal == " " ? "" : "- "));
 index = index + c;
 }
 if(r > 0)
 sb = sb.AppendLine(comments.Substring(index, r));
 return sb.ToString();
}

private void SaveValuesInViewState()
{
 string[] roomStr = this.RoomsString.Split('~');
 List<Room> rooms = new List<Room>();
 Room room = null;
 foreach (string str in roomStr)
 {
 room = new Room();
 string[] vals = str.Split('|');
 // 
 room.NoOfRooms = Convert.ToInt32(vals[0]);
 room.Type = (RoomType)Enum.Parse(typeof(RoomType), vals[1]);
 room.NumberOfCots = Convert.ToInt32(vals[2]);
 string[] childInfo = vals[3].Split('^');
 if (Convert.ToInt32(childInfo[0]) > 0)
 {
 foreach (string age in childInfo[1].Split(','))
 {
 room.ChildrenAges.Add(Convert.ToInt32(age));
 }
 }
 rooms.Add(room);
 }
 this.Rooms = rooms;
}

Answer 1

1) You're introducing too many variables, understanding the code tends to be less easy more variables (useless variables) you have and more mutable they are. In your first method I would remove l and r variables. Instead of l you can simply use comments.Length and r is not that needed (see next point). I would also remove lines and i iterator, index can be used instead:

for (int index = 0 ; index <= comments.Length ; index += c) { }

2) Following condition seems to be useless, I can't imagine any reasonable values for comments.Length and c which would make this condition to be true keeping in mind that we have already checked that l >= c:

 int r = l % c;
 int lines = (l - r) / c;
 if (lines == 0)
 return comments; 

3) Use less crypty names for variables. We do not do math here. Why c? I can guess it probably but I won't like to.

4) Why string here? It is definitely a char:

string indexVal = comments[index + c].ToString(); 

5) Do not introduce room variable in second method outside of loop. It is used only inside so it should be defined there.

6) You use overcomplicated format in for rooms in second method, especially around child ages. Right now you have something like this: 5|1|3|0~5|1|3|2^8,10. You have 4 different separators here, but I think ^ can be safely removed. Just put there ages if you have any or leave empty you have no children ages. You should be definitely able to parse this instead: 5|1|3|~5|1|3|8,10.

• Zebi: I would recommend using a good serialization format for your view model data. Take a look at JSON.net. JsonConvert.SerializeObject(rooms) and JsonConvert.Deserialize<List<Room>>(jsonString) would be all code required to get your data back and forth.

Answer 2

Using a lower-case "el" as an identifier is a bad practice. It is easy to confuse it with the literal 1 in many fonts.

• Winston Ewert: Then use a better font. (l is bad for being cryptic regardless, but one should use a font that makes the difference clear when coding)

Answer 3

I agree with all the comments posted so far but no one has explicity addressed the magic number 65 referenced by 'c'. Don't use magic numbers, given enough time they will bite you in the butt!!

It's better to pass in the these types of constraints. So I would refactor the code to either be:

private string FormatComments(string comments, int characterLimit) { ... } 

or make it a class member, which I think is better:

private string FormatComments(string comments) 
{ 
 ...
 if( comments.length < this.CharacterLimit ) 
 return comments; 
 int overflow = comments.length % this.CharacterLimit; 
 int lines = (comment.length - overflow) / this.CharacterLimit; 
 ...etc... 
} 

Finally, again -- just to restate, don't use one letter variable names unless it's i, j, k, n, or x or unless your function is less than 3 lines long. Never use 'l' for a variable name, go ahead and spell out 'len' if you are storing a length.

Remember, you write code for other programmers. BTW, consider yourself in the "other programmers" group because a year from now when you are debugging your old code you'll appreciate that you took the time to write readable code; don't take shortcuts. Check out http://cleancoders.com for tips and ideas.

Answer 4

As others have mentioned, the big things are:

• Try using more descriptive names for your variables (MaxLineLength, comments.Length, charsOnLastLine, etc.)
• Make c a constant (const int MaxLineLength = 65), a class variable or an argument to the method.

I would move StringBuilder sb = new StringBuilder(); down close towards the for loop. If l < c (or indeed lines == 0 somehow) then you will have wasted creating a StringBuilder for no reason.

You do not need to assign sb back to sb. You can just do sb.AppendLine(...);. StringBuilder is a mutable object, and any operations (such as AppendLine) change the state of StringBuilder. The reason why you need to do things like stringVar = stringVar + "bar" is because string is immutable, and all operations create new strings because the original string can't be changed.

There is no need to convert indexVal to a string.

char indexVal = comments[index + MaxLineLength];
sb = sb.AppendLine(comments.Substring(index, MaxLineLength) + (indexVal == ' ' ? "" : "- "));