Head.SmackOnTable();

I often have this discussion with less learned members of staff, with regards to commenting code. The main argument is “Comment code so I can see what is going on!” and “If you don’t comment the code, I dont understand”. In the first instance, I recommend refactoring the code and rewriting it, so you do not NEED the comments, and usually (99.99%) with the second argument for commenting is, If you do not understand the code, should you really be touching it?

There is the 3rd style of commenting I sometimes come across, and that is the Newbie Comment. Typically, the code has been lifted from a coding website, and someone has left the comments in there, because they either dont know how to use their text editor, or they dont understand the code they are coping and pasting. Both of which, to me at least, indicate that the person should not be writing code and should spend more time understanding the framework and how things work in general, like the following:

//create file object
 FileInfo fi = new FileInfo(filePath);
 //read the file into a stream reader
 StreamReader sr = fi.OpenText();
 //store the contents in a string
 string emailBody = sr.ReadToEnd();
 sr.Close();

The most common comments I see are totally useless, and simply repeat what the line below will do, for example i++; //increment i. How is that comment at all useful? Its not, it just tells you what the line is doing, which the line itself should tell you. Its duplication of code, you are repeating yourself, which is violation of the DRY principle:

Don’t Repeat Yourself

From the wonderful book of The Progmatic Programmer. The comment adds nothing to the overall readability of the code, so it shouldnt be there, it is simply extra clutter to make things more difficult to read and maintain.

Often, when working with someone else’s code, it works, but it isnt easy to maintain and read. Basically its ugly, and sometimes its like an ugly bag of snakes. Take this example:

Note: This code has been changed to protect the guilty.

[HandleError]
 public ActionResult LogOn(string username, string password, bool rememberMe)
 {
 string failureText = _websiteService.Login(username, password, rememberMe);
 if (!string.IsNullOrEmpty(failureText))
 {
 TempData["LoginFailure"] = failureText;
 return RedirectToAction("CheckoutAddress");
 }
 else
 Response.Redirect(Url.Action("CheckoutAddress", "Orders"));

 return View();
 }

Now, to me, this rings alarm bells, for multiple reasons. First off, you have failureText which claims to login, but actually gets you if it failed or not. Next thing you have is the “If there is no failure text, then it has worked”. Thirdly, you have the defunct return view, even though it shouldnt really be there.

How should this code be structured I hear you cry? Well, something like this:

[HandleError]
public ActionResult LogOn(string username, string password, bool rememberMe)
{
 if (AreCredentialsValid(username, password) && IsUserAllowedToLogin(username))
 {
 SetAuthenticationCookie(username,password);
 return RedirectToAction("CheckoutAddress");
 }
 else
 {
 ShowInvalidCredentialsPanel();
 return RedirectToAction("CheckoutAddress");
 }
}

Much more easier to read, much easier to maintain and modify, and no having to look for magic strings and just assuming someone can login because there wasnt an error.