Home » Avoid… » Most code comments

Most code comments

Overview

I’ve got an issue with the most code comments I see. All in all, it is OK to write comments, but only if there is really no way to nicely express the intention through the names used in the code and method signatures. In most cases you can do that!

Sometimes I find a helpful and meaningful comment in the code base. However, this happens quite seldom and is more an exception than a rule. Much more often and regularly I see places in the code where intentions are unclear and comments are either missing or do not help at all. Or even worse – comments are misleading, mostly because they are outdated! But even more often I see comments which are literally redundant, because they don’t add anything to what the code already says. Unfortunately, this is very common. And that’s why I decided to write this article and pack it with examples for the good, the bad and the ugly code comments.

The cause of the problem

Code comments are basically of two types – the header comments (<summary> in C#) placed on top of methods, classes, properties and the code comments between the lines of implementation. In both cases too often the comment either has no value at all, or indicates that the name of the member or the commented code is not self-explanatory and has to be improved. In my opinion, there are two main reasons for this issue:

  • First – bad habits of developers with good intentions. They feel that they need to write comments, as they were taught to. They are trying to explain what they are doing through the comments instead of doing it through the code and finding better names. Even worse, such habits become adopted by other developers, repeating what their colleagues do.
  • Second reason – guidelines and managers with good intentions, asking developers to comment every class and method according to the guidelines. Needless to say, that such guidelines clearly miss the goal and result is a huge amount of obvious comments. That’s where “Gets or sets” examples come from (see below).

To be honest, if I wouldn’t know that initial intentions are good, I would feel that most developers are mocking their colleagues with most of the comments I come across. Especially because developers are intelligent enough to write computer code and understand complicated systems, I wouldn’t expect them to do something meaningless like commenting constructor with ‘//constructor’. That’s confusing…

Why to avoid

  • Many comments are simply useless. They don’t add any value to the code itself. See examples below.
  • The code should be as self-explanatory as possible and require very few explanations in the form of comments. Even a helpful comment indicates that the code is not self-explanatory. So, before commenting anything you should first refactor the unobvious code to make your intentions clear, and maybe there will be no need at all for the comment afterwards.
  • Compiler reads code, not comments. Therefore, there can be all kinds of differences between what code does and what comment says, and obviously the code wins. In a code base which changes daily every comment has a high risk to become accidentally outdated. The common scenario is when someone quickly changes something in the code, without being aware of a comment which is placed, for example, in the header of the class, dozens of LOC before. The compiler, of course, won’t recognize the problem and application will work perfectly. But the comment, even if it was nice and meaningful before, now starts lying about the class or method. Nevertheless it still pretends to explain what the code does or why. This is really dangerous because this is misleading other developers, resulting in introducing bugs.
  • I think that for the reasons described above every developer knows that the only truly reliable thing in the code base is the code, not the comments. Software does what the code does, not what comments say. So, the developers do not blindly trust comments and check what the code is doing anyway. Comments are treated as probably outdated hints written by people with good intentions, not as truth. So again, the goal of having helpful comments is often not achieved and good intentions end up with a lot of effort for nothing.
  • Comments add extra lines to your code or extra length to your code lines.

Alternatives

Short: comments can be avoided if proper naming and object-oriented code with small methods are the case.

  • Split the code blocks into the small ones. Just extract methods/classes/variables and name them in a way that the names describe what they do, leaving no need for commenting.
  • If the code is already well-structured, rename methods/classes/variables explaining what the code inside does. Try to say what is happening in the code in a short phrase, then make the code as close to this phrase as possible. For example, if what the code is supposed to do is “if customer has a birthday today display happy birthday widget with customer’s age”, that’s how it should look like:

    if (customer.HasABirthdayToday())
            Display(new HappyBirthdayWidget().With(customer.Age));
    

Coding

Examples of comments in not self-explanatory code

In today’s object-oriented programming languages it’s quite easy to write code which you can read almost like a prose. If the code is good enough you won’t need comments. And even better – the former value of comments is merged into the code.

In the following code the “confirm” parameter is not self-explanatory:

public class Email
{
        /// <summary>
        /// Sends an e-mail to the customer
        /// </summary>
        /// <param name="customer">customer to send e-mail to</param>
        /// <param name="confirm">request a read receipt</param>
        public void SendTo(Customer customer, bool confirm)
        {
                ...

It’s obvious that a method with such a signature sends an e-mail to the customer, so the first two comments are not needed. Only the third one is valuable. It explains what the mystic “confirm” parameter does. The core problem is the not descriptive parameter name “confirm” itself. And here comes an easy trick. I would be happy if this would be even the only one thing you’ll remember from this article:

  • Take the comment: request a read receipt
  • Remove the blanks and make it according to your naming conventions (camel case in my example): requestAReadReceipt
  • Rename the parameter confirm to requestAReadReceipt
  • Remove the comment.

There is one even better thing to do – start naming parameters, variables, etc this way from the beginning. Use short precise phrases in proper English for that. It’s amazing how better the code becomes when instead of writing a useful comment a good name is being picked. This is a real level up for the code, improving the code readability!

Here’s the final refactored version of Email.SendTo method with self-explanatory signature:

    public class Email
    {
        public void SendTo(Customer customer, bool requestAReadReceipt)
        {

More examples for removing the comment and renaming the variable or the method to what the comment initially said (so that no information is lost in the code base):

Comment required:   Comment not required:
    int timeout;	//timeout in ms
 
    int timeoutInMs;
    int usrErrLogAttemptCnt;	//count of failed login attempts
 
    int countOfFailedLoginAttempts;
    getRslt(curSearchParams);	//get results for current search parameters
 
    getResultsFor(currentSearchParameters);

Here’s in an example from Martin Fowler’s “Refactoring” book, chapter “Extract Method”. Comment is there to tell a developer what is happening in the next two lines. Having a properly called method instead makes the initial method smaller and nicely encapsulates the task of printing the details into another small (and therefore easy to read and understand) private method.

Comment required:

void printOwing(double amount) {
    printBanner();
 
    //print details
    System.out.println ("name:" + _name);
    System.out.println ("amount" + amount);
}
  Comment nor required:

void printOwing(double amount) {
    printBanner();
    printDetails(amount);
}
 
void printDetails (double amount) {
    System.out.println ("name:" + _name);
    System.out.println ("amount" + amount);
}

The “Clean Code” book has 20+ pages dedicated to good comments and bad comments. I find one of the examples particularly enlightening:

This is an evaluation of customers data and a comment telling what is happening:

// Check to see if the employee is eligible for full benefits
if ((employee.flags & HOURLY_FLAG) && (employee.age > 65))

This is a refactored version with proper implementation of object orientation: an Employee class offers a method which evaluates its data. The method name tells what it does. This is the way how we want code to be.

if (employee.isEligibleForFullBenefits())

Examples of really useful comments

This comment explains why is something happening, what’s the intention is. That’s exactly what comments should do. It is hard to make the code explain it, so the comment is really helpful here. A second comment with “to do” also reveals that this is an intermediate solution and tells exactly when it should be removed.

// Requested on 01.02.20XX by support guys, because on holidays only few of them are in the office
//  and they can't answer all the website users calling regarding feature X, so X will be switched
//  off on holidays. Approved by Manfred, the manager.
// To do: starting with 01.05.20XX an external call center will be responsible for users support on
//  holidays, so please remove this hack after this date.
if (TodayIsAPublicHoliday())
        TurnOffTheX();

Here something not obvious happens due to the performance optimization. It is essential to comment such places, because otherwise someone can remove the needless looking ToList() method call.

// It is technically not required to convert query to list, but if you don't do it here,
// there will be a performance issue due to lazy loading, latest when query will deliver more than 100 results.
public IEnumerable<Something> Get()
{
        ...
        return queryResults.ToList();

Following comment contains information about the parameter which can’t be put into the method signature itself.

/// <param name="format">Format for the displayed value. Default is "{Id} ({FullName})".
///                      Supported are: {Id}, {FullName}, {Age}, {FirstName}, {LastName}</param>
public void SetFormat(string format)

Examples of useless comments without any extra value

Useless comments for properties

Here the common case in C# code, where properties and classes have summary tags, which can be used to automatically create a documentation. Sounds like a good idea, but looking at the outcome… the good intention missed the target again.

Following comments simply repeat what the code already says.

/// <summary>
/// Gets or sets the customer
/// </summary>
public Customer Customer { get; set; }

or simply:

/// <summary>
/// Customer
/// </summary>
public Customer Customer { get; set; }

Actually there are tools which create such comments automatically from the code.

Following are the real examples from a commercial .NET library. Comment pretends to be a phrase in English which is longer than the property name, but look closer – the comments tell nothing you want to know about the properties, adding no extra value to what the names and data types already say:

//
// Summary:
//     Gets a value indicating whether this instance is in deactivate.
public bool IsInDeactivate { get; }
//
// Summary:
//     Gets a value indicating whether this instance is in delete.
public bool IsInDelete { get; }

I had to work with this library and would really REALLY like to know, what does “instance is in deactivate” mean. But all I’ve got was a comment which, sadly, doesn’t reveal the mystery of “IsInDeactivate” but shows me a middle finger instead. It’s like checking a foreign word in a dictionary and finding, instead of translation, the same word, transcribed in your native language. Yes, formally, the dictionary has this word, but practically for you it’s just useless.

Useless comments for methods

public class Customer
{
        /// <summary>
        /// Constructor
        /// </summary>
        public Customer()

or more words but with the same meaninglessness:

/// <summary>
/// Creates an instance of customer
/// </summary>
public Customer()

Actually, I often remove such comments. Because it’s 3 lines of verbal noise in the code editor.

And, already mentioned, an e-mail class:

public class Email
{
        /// <summary>
        /// Send mail
        /// </summary>
        /// <param name="customer">customer</param>
        /// <param name="confirm">confirm</param>
        public void SendTo(Customer customer, bool confirm)
        {
                //here comes the logic
        }

In this example there are 3 comments, none of which says more that the code already does. I would like to know what the “confirm” parameter does. Who confirms what?! The comment is required there, but existing comment is unfortunately absolutely useless. It can be removed without any loss of information.

By the way, having an Email and a Customer class makes the signature already good understandable, which is an advantage of object-orientation. It’s also safer, than to operate with strings and other primitive types, because, you can’t pass anything other than an instance of customer to this method.

Another improvement here would be to split this method into two methods. Actually, a boolean parameter indicates that this method does two things – one if boolean parameter is true, and another one if it is false. Having two separate public methods would solve the problem and remove the conditional logic from it:

        public void SendTo(Customer customer)
                ...
        public void SendRequestingReadReceiptTo(Customer customer)
                ...

Useless comments in the implementation

Some comments pretend to help:

// for each customer
foreach (var customer in customers)
{
        customer.DoSomething();
}

It’s obvious that loop does something for each customer in the list. This is already written in the line ‘foreach (var customer in customers)’ which is even almost a proper English – ‘for each customer in customers’.

Another example of obviousness:

// if amount is over the limit
if (amount > limit)
{
        //do something with customer
        customer.DoSomething();
}

Some comments are just weird. A real life example:

// return result
return result;

I won’t comment on this comment (*ironic*).

Level Up Code, December 2013

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: