I recently came across a blog entry on Importance of Writing Code Comments in Software Development. It’s a proposition I’ve heard many times before, so I took a read to see if there was some reasoning I may have missed. There wasn’t.

Like the author of that article, I spend far more time reading code than writing it. O tempora o mores! Comments are not what I want to see, my friends, Clean Code is what I want to see!

Robert C. “Uncle Bob” Martin (more than) once declared: Comments are lies. He is (partially) right!

Here is some code from a project at my workplace. I’ve changed some of the names to protect the guilty but otherwise this is genuine:

//  --------------------------------------------------------------------------
//  Method Name:        CProtocolField::setValue
//  Definition:         This method sets the value attribute of the CProtocolField.
//  Qualification:      none
//  Export:             public
//
//  Return:             void
//  Parameters:         double value
//
//  Preconditions:      none
//  Postconditions:     this->getString() == value
//  Exceptions:         none

    void
    CProtocolField::setValue(const string& value)
    {
        myLength = value.length();
        myValue = new char[myLength+1];
        if (myValue != 0)
        {
            memcpy(myValue, value.c_str(), myLength);
        }
        myValue[myLength] = '\0';
        myBuffer = NULL;
    }
//  --------------------------------------------------------------------------

Ignore the terrible formatting, variable names and call to memcpy(), Did you spot the lies?

The “Method Name” is quite correct but I didn’t need a comment to tell me that. I suspect that even if you’ve never seen C++ before, you’d be able to tell me what the name of the class and its method are. The “Definition”, hardly needed a resident of 221B Baker Street to reveal it to the unsuspecting public, it sets the value, hence its name. It also does some other things, but we’ll come to that later. “Qualification” is used (in case you were wondering) to explain to the unseasoned reader whether a particular method is static or const or even virtual. Given this method is used to set a member variable (or three!), it’s pretty obviously nether of the first two. According to the declaration in the header-file, it’s non-virtual but… CProtocolField is derived from IProtocolField, which declares setValue(const string&) as pure virtual, making this method virtual. A lie!

Helpfully we’re told that this set-function has a public access specifier. I’m not sure what use a private setter would be, but we’ll move right along to the blatant lie: “Parameters: double value”! Why would any programmer write that? This method clearly takes one argument and it’s a const string&!

Of course a programmer didn’t waste their time writing this lie. It was copied and pasted from the double overload 44 lines earlier in the file. (I submitted a patch to remove the duplication, of course).

The “Preconditions” also lie. Whomever assumed responsibility for allocating myBuffer (a raw char*, no less) also assumed the precondition that this buffer has not already been allocated. This same (incorrect) assumption is made in eight other places in the same source file! Removing the memory leak was the main reason for me submitting a patch (shared code ownership is a topic for another day).

The claim in “Postconditions” tells us nothing useful. What if I never call getString(), what is the state of my object? If there were accompanying unit-tests, I would have checked them. (Instead, I wrote them).

Unit-tests are of course the best documentation for any code. They are living documents that you can execute at any time.

I said that Uncle Bob was only "partially" right. The comments below (from the same code-base) tell the truth, the whole truth and nothing but the truth:

//////////////////////////////////////////////////////////////////////////////
//
// Function Name:   ExcCxlCached::~ExcCxlCached()
//
// Definition:      Destructor for the ExcCxlCached class
//
// Qualification:   virtual
//
// Export:          public
//
// Return:          none
//
// Parameters:      none
//
// Exceptions:      none
//
////////////////////////////////////////////////////////////////////////////////

ExcCxlCached::~ExcCxlCached()
{
// Nothing to do here
}

Eighteen lines of comments for a destructor (that is obviously a destructor for the ExcCxlCached class) that does nothing! No lies, just noise.

By writing this kind of cruft, you are creating noise for the poor person who has to come along and read (and understand) your intent. The signal-to-noise ratio here is criminally low.

We’re not paid by the line-of-code, people! Please stop! If you want to do more typing, please expend it on putting some vowels and meaning into your identifiers. Or writing unit tests.

For this reason I have the following mapping in my .vimrc:

nnoremap <Leader>ic :<C-U>highlight! link Comment Ignore<CR>

This makes it very easy for me to hide the lies and the noise and concentrate on the code!

Remember: Comments are a failure to express yourself clearly in your code. Clean Code: A Handbook of Agile Software Craftsmanship (Robert C. Martin) is recommended reading on the matter.