April 26, 2009

Unnecessary comments must go

There are about as many commenting styles out there as there are programmers. Probably more. Some programmers believe that comments are pure goodness, and the more comments a codebase has, the better. Documentation is good, right? More comments mean more documentation.

Some programmers feel that comments just get in the way, and sometimes lie. They’re to be avoided, mistrusted, shunned even. Get that peanut butter out of my chocolate.
In his book Clean Code, Bob Martin says:
Indeed, comments are, at best, a necessary evil. If our programming languages were expressive enough, or if we had the talent to subtly wield those languages to express our intent, we would not need comments very much – perhaps not at all.
I tend to agree.

He goes on to categorize comments as Good and Bad. Good comments being those that express legal terms, those that are truly informative, explain intent, clarify code that is otherwise tricky to understand, warnings, TODOs, or comments that amplify the importance of a piece of code that may otherwise seem inconsequential. In my experience, those types of comments (with the exception of Legal comments) are actually pretty rare.

Bad comments are those that are redundant (the comment says practically what the code says), misleading, mandated (like documentation comments for every method), journals (who edited what, and when),  noisy (like decorating the default constructor with code that says “this is the default constructor”), position markers (“Properties here”, “Methods here”), attributions (“Eric did this”), commented out code, and such.

Here’s what it boils down to for me: I want to read code, not comments. It’s the code that gets executed, not the comments. So when I’m looking at some code, the comments just get in the way.

If I ever get the itch to add a clarifying comment, I try to step back and see if there’s a way I can provide clarification in the code itself. Is the method really long? Then break it up into smaller chunks. Can the variables be named better? How about the method name itself?

The vast majority of the time, it turns out that by writing the code a bit differently, a comment is not actually needed.

So, in my ongoing experiment with refactoring BlogEngine.NET, one of my first acts when I open a class file is to cull the unnecessary comments. Cut them out. With the comments out of the way, I can actually see the code, and figure out what’s going on under the hood.

An example, from the Post.cs class:

   29         #region Constructor
   31         /// <summary>
   32         /// The default contstructor assign default values.
   33         /// </summary>
   34         public Post()
   35         {
   36             base.Id = Guid.NewGuid();
   37             _Comments = new List<Comment>();
   38             _Categories = new StateList<Category>();
   39             _Tags = new StateList<string>();
   40             DateCreated = DateTime.Now;
   41             _IsPublished = true;
   42             _IsCommentsEnabled = true;
   43         }
   45         #endregion

The “default constructor” comment is just clutter. It’s got to go. Nobody who could possibly understand what this code is doing would fail to understand that it is the default constructor. The comment just gets in the way. And, as an aside, did you catch the misspelling?

I remove the evil code-hiding region markers also.

Another example, from Utils.cs:

  102         /// <summary>
  103         /// Removes the HTML whitespace.
  104         /// </summary>
  105         /// <param name="html">The HTML.</param>
  106         public static string RemoveHtmlWhitespace(string html)
  107         {
  108             if (string.IsNullOrEmpty(html))
  109                 return string.Empty;
  111             html = REGEX_BETWEEN_TAGS.Replace(html, "> ");
  112             html = REGEX_LINE_BREAKS.Replace(html, string.Empty);
  114             return html.Trim();
  115         }

Do I need a comment to tell me that the RemoveHtmlWhitespace() method removes the HTML whitespace?

And there’s a whole slew of these, too:

   21         private Guid _Id;
   22         /// <summary>
   23         /// The Id of the comment.
   24         /// </summary>
   25         public Guid Id
   26         {
   27             get { return _Id; }
   28             set { _Id = value; }
   29         }

When the (completely unnecessary) comments are removed and Auto-Implemented Properties are employed, the above code turns into:

   10  public Guid Id { get; set; }

So much better, isn’t it?