Commenting on Comments

A well-crafted comment, placed in the right spot, can be one of the most helpful and clarifying things in a codebase. On the other hand, nothing can clutter up a module more and slow down developers than frivolous, dogmatic, or irrelevant comments.

Comments are a feature of a language, and thus, they are not inherently good or evil. However, they can be helpful or harmful depending on how they are used.

In this post, we're going to discuss some of the most common cases of effective and ineffective commenting practices, and try to understand this feature that can cause so much heated debate among developers.

The Bad Parts

One of the most common motivations for adding comments in the code is that the code is bad. A badly written algorithm or a huge if-else chain are all prime candidates for comments. We know that we have made a mess. So we say to ourselves, "I'd better comment that!". And this is how evil comments are born.

Most comments tend to fall into this category. Usually, they are excuses for smelly code or justifications for poor design. These comments amount to little more than the ramblings of a madman.

Mumbling

Plopping a comment because of policy or because you feel like it is not a solution. It's a hack. If you decide to add a comment, you have to make sure it is the best comment for the situation. Below is an example of a comment that had potential. The author though didn't want to spend the time or effort to write something meaningful.

public void LoadProperties()
{
    IConfigruation configuration = new ConfigurationBuilder()
        .AddJsonFile("patches.json")
        .AddEnvironmentVariables()
        .Build();

    var section = configuration.GetSection("EntityManager");
    var exists = section.Exists();

    if(exists) {
        EntityManager.Host = section.GetValue<string>("Host");
        EntityManager.Provider = section.GetValue<string>("Provider");
    }

    // No section means all defaults are loaded
}

What does this comment mean? It obviously means something to the author. But its meaning doesn't come through to us. Apparently if exists is false, there was no EntityManager section. But who loads the default values? Where they loaded before the call of LoadProperties? Or does it mean that the EntityManager already has default values? Or there is another method called after LoadProperties that loads the default configuration for the EntityManager? Does the author tried to comfort themselves for leaving a gap in the logic? Or -and this is a scary prospect- this is a TODO comment and the author tries to tell themselves to revisit this code later and actually load the defaults?

Such a comment, in a best case scenario will force us to examine code in other parts of the system to figure out what's going on. In a worst case scenario, it forces us to make arbitrary assumptions. At first, I assummed that the defaults are preloaded when the EntityManager class was instantiated.

Redundant Comments

In the next example, we have a simple function with a comment that is completely redundant. The comment is likely harder to understand than the code.

// This is a utility method that returns true when this.Closed is 
// true. If timeout is reached then throws an Exception.
public bool WaitToClose(long timeoutMillis){
    if(Closed)
        return true;

    Thread.Sleep(timeoutMillis);

    if(!Closed)
        throw new System.Exception();

    return true;
}

What is the purpose of this comment? It's certainly not more informative than the code. It does not provide any documentation either. It does not provide any rationale. It is not easier to understand than the code. In fact, it is less precise and entices the reader to accept that lack of precision instead of true understanding. It acts like a salesperson trying to convince us that there is no need to check under the hood.

Now imagine that we have a legion of such useless and redundant comments across the codebase. They serve only to clutter the code. They serve no documentary purpose at all.

Misleading Comments

They say that the road to hell is paved with good intentions. Oftentimes a developer will try their best to write a meaningful comment, but it will not be precise enough to be accurate. Remember for a moment the redundant and subtly misleading comment from the previous example.

public bool WaitToClose(long timeoutMillis){
    if(Closed)
        return true;

    Thread.Sleep(timeoutMillis);

    if(!Closed)
        throw new System.Exception();

    return true;
}

Did you see how this comment is misleading? The method doesn't return when this.Closed becomes true. The method returns if this.Closed is true. Otherwise, it will wait for some timeout and then it will throw an Exception if this.Closed is still false.

This is a subtle bit of misinformation, hidden inside a comment that is harder to read than the body of the code. It can cause a developer to just skim through the comment and expect that this will return as soon as this.Closed becomes true. This developer will soon find themselves debugging the code to find out why it executes so slowly.

Noise Comments

We occasionally come across comments that are nothing but digital white noise. They restate the obvious and provide no new, insightful information.

// The day of the month
private int _dayOfMonth;

These comments are unhelpful and we eventually learn to ignore them. As we read through code, our eyes simply skip them. Eventually, these comments condition us to develop an even worse habit. After we encounter enough noise comments, we tend to skip all the comments we see in the code base.

Banners

Sometimes, developers mark particular positions in the source file for different purposes. This can be done to mark a specific section of the code or create a rudimentary organizational system, among other uses. For example:

// ======================= Controller Actions ===================

It's rare for a banner like this to make sense. Even if it sometimes makes sense to group certain functions, it's more likely to be a code smell than a good opportunity. In general these comments are clutter and should be eliminated.

Commented-Out Code

Few practices are as abhorrent as commenting-out code.

foreach (string filename in Directory.EnumerateFiles(StartDirectory))
{
    using (FileStream SourceStream = File.Open(filename, FileMode.Open))
    {
        using (FileStream DestinationStream = File.Create(EndDirectory + filename.Substring(filename.LastIndexOf('\\'))))
        {
            await SourceStream.CopyToAsync(DestinationStream);
        }
    }

    // using (FileStream SourceStream = File.Open(filename, FileMode.Open))
    // {
    //     await SourceStream.CopyToAsync(Orchestrator.Stream);
    // }
}

People who see commented-out code will probably never delete it. They'll think it was left there for a reason and that it's too important to delete. So commented-out code gathers like dregs in the bottom of a coffee cup, gradually becoming more and more difficult to sift through and decipher, often obscuring the purpose of the code containing it.

The Good Parts

Even though most comments are malicious, and only obscure the code and mislead developers, there are some comments that are necessary or even beneficial.

Informative Comments

It is sometimes useful to provide basic information with a comment. For example, the following code has a comment that explains the return value of an abstract method:

// Returns the data stored in this data structure.
protected abstract IEnumerable TraverseStructure();

A comment like that can sometimes be useful, but it is better to use the name of the function to convey the information where possible. For example, in this case the comment would be redundant by renaming the function GetData.

Here's a case that's a bit better:

// format matched ddd dd MMM yyyy-HH:MM:ss.fff
string pattern = @"\w{3} \d{1,2} \w{3}\-\d{2}:\d{2}:\d{2}.\d{3}";

In this case the comment let us know that the regular expression is intended to match a date and time string using the specified format string.

Explanation of Intent

Sometimes a comment goes beyond just useful information about the implementation and provides the intent behind a decision. Below, is such a case of a decision documented by a comment.

When comparing two objects, the author decided that they wanted to use a different algorithm on particular objects.

public int IndexOfPattern(ISegment segment, string pattern)
{
    if (segment is MultilineSegment){
        /** The search algorithm turned out to be slower than Boyer-Moore
         * algorithm for the data sets in the segment, so we have used a
         * more complex, but faster method even though this problem does not
         * at first seem amenable to a string search technique
         */
        return BoyerMoore(segment, pattern);
    }

    return MatchPattern(segment, pattern);
}

TODO Comments

It is sometimes reasonable to leave "To Do" notes in the form of TODO comments. In the following case, the TODO comment explains why the function has a degenerate implementation and what the function's future should be.

//TODO these are not needed
// will be removed when we change our versioning model.
protected VersionInfo makeVersion()
{
    return null;
}

TODOs are small, non-essential tasks that the developer thinks should be completed at some point, but cannot be done at the moment. It might be a reminder to delete a deprecated feature or a plea for someone else to look at a problem. It might be a request to someone else to think of a better name or a reminder to make a change that is dependent on a planned event. Whatever a TODO might be, it is not an excuse to leave bad code in the system.

Amplification

A comment may be used to emphasize or amplify the importance of something that might otherwise seem inconsequential or unimportant.

string path = PathAnalyzer.Path().Trim();
// the trim is really important here. It removes the starting 
// spaces that could cause the item to be recognized 
// as another item.
new PathItem(path, path.Content, this.level + 1);

Documentation of Public API

There is nothing quite so helpful and satisfying as a well-documented public API. It would be difficult, at best, to write code without proper documentation. Having clear and concise documentation can make all the difference regarding code quality and maintainability.

If you are writing a public API, then you should certainly write thorough, accurate, and user-friendly documentation for it. But keep in mind the rest of the comments we discussed here. Documentation can be just as misleading, nonlocal, and dishonest as any other kind of comment.

Comments, Yay or Nay?

Crafting the appropriate comment is one of the more difficult and subtle parts of day to day coding. A well-placed comment can make your day, while a bad, irrelevant or outdated comment can cause hours of frustration and debugging.

Code is constantly changing and evolving — pieces of it are constantly moving around. Refactorings can completely change the code in a module, and algorithms can be removed and replaced. Introducing a feature can cause ripple effects that mutate the code around it. Unfortunately, comments don't always follow these changes. And all too often, the comments get separated from the code they describe, becoming orphaned, inaccurate ramblings.

Before writing a comment, always consider whether there's a way to express yourself through code. If there's no way to do so, make sure your comment is clear, concise, and correctly reflects your intention.