Write Comments that Matter

Introduction

Commenting code is a good thing! It helps other programmers to read your code and as such reduces the chance for bugs. WRONG! Commenting code CAN be a good thing and it CAN help others (and yourself) to read your code, or it can do quite the opposite... Just for clarity, this article talks about comments in code, not XML comments (that document code and give you some IntelliSense).

What you first need to understand is that each written comment is a line of text that has to be maintained. After you alter a piece of code that is commented, you will have to read the comments and possibly alter them if they don't hold true anymore. In my experience, this doesn't happen to often. Fixes are made quickly and time is sparse, too sparse to rewrite any comments! The result is often code littered with unclear comments that just don't explain the code it is supposed to clear up...

Too often have I seen comments that didn't make sense, that didn't add value to the code, that were unclear, or that were just plain wrong. So let's take a look at how we can reduce comments.

// i is the selected index.
int i =  myControl.SelectedIndex();

Now, there are two things wrong in this code. First of all, the variable name i is unclear. What does i represent? Apparently, the coder thought so too, because he commented on what i is. So with that comment, our code is understandable again, right? No. Even without the comment, we could see that i is the SelectedIndex of myControl, simply because it stands right there. So does that make it right to simply delete the comment and our code will be clear again? Still no. What if i is used later in our code? Let's say we have the following line of code a few lines ahead.

// Code here...
someList.Insert(i, "Hello");
// More code here... 

So what is i now? We must read a few lines back to see what i represents. Or we could write another neat comment to tell us that i is the selected index! No, no no! Commenting isn't the answer! Writing clear code is!
So let's rewrite the first bit of code so that it is clear what i represents and which also eliminates the need for any comments.

int selectedIndex = myControl.SelectedIndex();
// Code here...
someList.Insert(selectedIndex, "Hello");
// More code here...

By properly naming our variables, we now know the purpose of i and we eliminated the need for comments.

Let's take a look at another example of wrong commenting.

// List<T> represents a strongly typed list of objects that can be accessed by index.
// List<T> uses an Array internally so accessing an item by index is super fast.
// It also makes use of so-called Generics. This is a technique... etc.
List<int> productIds = new List<int>(); 

While this comment certainly adds something to the code, being what a List<T> is and how it works, I highly discourage you to use these kind of comments in your code. Why? First of all, List<T> and Generics are part of the .NET Framework and are very well documented on MSDN and other websites. I hope the person that is going to work on your code knows how to use these Classes or at least how to find out. Whatever the case, it is not your job to document the framework you are using. Also, what if new requirements or insights require not a list of product IDs, but a dictionary of product IDs and the Product Classes they represent? You would have written all those comments for nothing and the person who is going to change the code needs a lot of time to go through your comments and may even feel obliged to document the now used Dictionary(int, Product) in much the same way! Keep in mind that we are writing code, not how-to books.

So how about the next example?

// Save the customer to the database.
customer.SaveToDatabase();

The code is easy to read. It looks like the coder thought about his variable- and function names. The comment looks clear and to the point too. Still I say nay. What does the comment add to the code? Absolutely nothing. It simply switches the words customer and SaveToDatabase (and adds some spaces). The comment is just a grammatically correct version of our code. That might help your little sister to follow our code, but your little sister isn't going to maintain this. Programmers are going to maintain it, and if a programmer cannot guess what customer.SaveToDatabase does he might as well find another career right away!

Now here's an interesting one. It really shows how neglected comments are and why you should use them as sparingly as possible. Someone copy-pasted the code above. Apparently, the code for product looked much the same, but wasn't abstracted out enough to re-use existing code, hence copy-paste was used instead. This was the result.

// Save the customer to the database.
product.SaveToDatabase();

While copy-pasting is a sin in its own right, let's focus on the comment here. It wasn't changed, it probably wasn't even read by the programmer who pasted this! Everything works, everything compiles, but we have this big lying comment staring at us. What happened here? Did the programmer mean to save the customer? Did the customer get saved here at some point in time? It's probably best to just delete this comment. And had we never written it in the first place, that might've saved us some time, effort and confusion! Mind you, these are real-world examples I found in production code!

And while we're on the subject of commenting the obvious, how about commenting the non-obvious in a non-obvious manner?

// A helper variable.
string helper = null;

Yeah, that comment really made a difference... We still don't know what helper is or does! Reading probably about twenty lines further I found this.

if (something)
{
   helper = "Some pretty long message";
   MessageBox.Show(helper);
}

That's all it did... Now we could have done all of this without the helper variable, but if we were using a variable to hold our string message, perhaps for readability, we could've at least named it something like messageBoxMessage, message or even msg, any of those would've been clearer than helper. Second, we could've kept the scope of the variable a lot smaller. Keeping scope of variables as small as possible is pretty good practice and helps for better readable code. So instead of declaring it twenty lines earlier, we could've declared it inside the if statement. Let's look at the new code.

if (something)
{
   string message = "Some pretty long message";
   MessageBox.Show(message);
}

Much better, no?

Another sin I have found is keeping a change log in code. Consider the next example.

// Ticket #666, Sander Rossel 2012-09-28.
if (someVar)
{
   //  ...

Apparently, the customer demanded that something did or did not happen if someVar is true. When we are reading the code, do we really care this was ticket #666, that I fixed (or broke) it and that it was on 28 september 2012? I think not. What's also a problem is that many fixes are not made in a few lines in a single file. Often, we need to change a few Classes, maybe add some code and even delete code. Do we need to put the ticket comment in each file? How are we going to comment deleted code? Now if we do want to see who coded what, there are plenty of tools for us to track and find out, one being Source Control tools. For example, I use Tortoise SVN or Git which both let you see exactly which user wrote what line of code or when a file was edited, what was edited and by whom. Not a single comment (or even a whole bookwork of comments) can beat that!

Something I also find very often is commented out code. Somehow programmers are afraid to delete code. They feel they've spent some good time writing their code and simply deleting it would hurt the code's feelings or something. I've even heard excuses like "I'm not sure if this code is still being used so I'll just comment it out instead of delete it." What!? There is no difference for the compiler! So if the application is going to work with the code commented out, it will work with the code deleted as well!

I literally find entire code files, hundreds of lines of code, commented out... How does this help anyone? If, for some reason, and it happens, you've been a little to enthusiastic with the delete button there's always Source Control to get it back.

This kind of comment is very often related to the kind of comment I describe above, keeping change logs in code. Commented out code is something like a change log, a log of how your code has changed. Just not a very clear or structured one. So just use Source Control!

I want to present to you a last example of a badly written comment.

// Calculate the alpha from the omega to achieve world domination.
int omega  = //... difficult math here.
int alpha =  //... more difficult math here. 

The comment explains that we need alpha and omega to achieve world domination. You might have wondered why you needed alpha and omega, now you know. However, when reading this comment, the first thing that comes to mind is what is alpha and what is omega? And why is the difficult math that follows not commented? Let's rewrite that.

// Alpha and omega are necessary to achieve world domination.
// Alpha is simply the answer to life, the universe and everything.
// Omega stands for infinite power and is necessary to calculate alpha.
// To calculate omega we need to...
int omega = //... difficult math here.
// Now that we have calculated omega we can calculate alpha by...
int alpha = //... more difficult math here.

Now this is actually somewhat of a gray area. You and I don't know what alpha and omega are, but perhaps it is common knowledge (or assumed common knowledge) at Evil & Co., the company that concocted this evil plan and wrote the code. In that case, it would be time consuming and not adding to the code. This example shows that a 'good' comment can be highly subjective. Likewise, if everyone working at Evil & Co. has a masters degree in math, it might not be necessary to explain the difficult math.

So I make it sound like comments are the root of all evil. Are there no good comments? Sure there are, let's consider them a necessary evil.

// This pattern matches any word starting with p and ending with n.
string pattern = @"\bp(\w*)n\b"
Regex regex = new Regex(pattern, RegexOptions.IgnoreCase);

Now that might save any future programmer a lot of time! I don't know about you, but I don't use regular expressions all that often and finding out what the pattern matches exactly would have me search the internet for at least a few minutes. When the pattern grows larger, the gain from the comment grows too.

Let's look at more helpful, even crucial, comments.

try
{
   // Log something to the database here...
}
catch
{
   // Do nothing. If logging fails the program should continue like nothing happened.
}  

Now an empty catch block might not be deserving of a beauty prize, but at least you know why it is there. More importantly, you might have removed the empty catch block because calling code would have sufficiently caught and handled any Exception anyway. However, now that the comment is there, you know the empty catch block is there by design and you better not touch it!

In conclusion, I can only say that writing good comments is difficult and what might be a good comment in one situation might be bad in another. However, if your code is littered with comments that don't clear up any code people might learn to ignore your comments and miss the important ones. On top of that, comments are lines of text which have to be maintained just like any other code. As such, comments deserve the same thought as your regular code. If a comment isn't worth writing, then don't write it! If it is, think twice, then think on what you are going to write and if you are still convinced the comment is absolutely necessary for future generations to maintain your code, go for it.

"Don't comment bad code-rewrite it." -- (Brian W. Kernighan and P.J. Plaugher)

History

• 28^th September, 2012: Initial version