02 October 2008

Excess commenting considered harmful

With apologies to Dijkstra.

I'm a big fan of comments, provided they're in the right place and convey the correct information.

While the near-total lack of comments in my current codebase irritates me, I also come across stuff like this in some of our common code which is utterly pointless:

//----------------------------------------------
// File: spizzbar.cpp
// Author: Earnest Programmer
// Created: 8/05/2008
// Purpose: Implements an API that will allow...
//----------------------------------------------
// Changed: never
//----------------------------------------------


Let's start with the lines of dashes which separate the comment block into regions:

//----------------------------------------------

I'll admit to still occasionally doing such things, and I'd argue in the case where you have a small set of headers (2, in the case of our current SDK) and the headers are likely to be the documentation users of the SDK are most likely to see, you can get away with such cruft, but 99.9% of the time, it's pointless (and I really need to stop).


Next, we have a line which tells us the name of the file:

// File: spizzbar.cpp

Umm, if we're reading the code, it's already in a file and it already has a file name and it came from version control, which can tell us all the different names this file has ever had, so this is also pointless.


Next, we have a line which tells us the name of the original author of the file:

// Author: Earnest Programmer

Aw, your mom sure must be proud. However, this information is also in the version control history, so why are you cluttering up the code with it?


Next are a couple of lines which tell us when the file was created and last modified:

// Created: 8/05/2008
...

// Changed: never

You can guess where this is going, so repeat after me: "this information is also in the version control history, so why are you cluttering up the code with it?" Nicely done, thanks.


Finally, we come to some lines which purport to add value:

// Purpose: Implements an API that will allow...

So let's see:
  • it's a .cpp file, so I'm pretty sure it's implementing something.
  • the API is in the matching header (presumably spizzbar.h), so the purpose of the API should already be documented there

In conclusion, there's no value here: the entire comment block is useless.

"So what's the big deal?", you may ask.

Fair enough: let's go through this.

Comments are no different than code: they have to be maintained, and useless comments are like dead code. Linkers usually strip dead code, but programmers spend most of their time reading code, so we're optimizing for that scenario.

Also, the DRY principle says you want to abhor duplication, so if you document a function in the header, you don't need to copy the documentation to the implementation. Otherwise, you'll have to maintain comments in both locations.

Finally, like every line of code, you should look at every comment and ask yourself: If I ripped out this comment and I or someone else came back in six months to look at this code, could we still understand the code? If the answer is yes, strip the comment.

I believe part of our focus as software developers should be on writing as little code as possible - most of the cost of software development is maintenance, and the more you have to maintain, the more it costs - and this extends to comments.

As the phrase misattributed to Einstein goes:
Everything should be made as simple as possible, but no simpler.

No comments: