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!
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:
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
const or even
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
CProtocolField is derived from
IProtocolField, which declares
string&) as pure
virtual. A lie!
Helpfully we’re told that this set-function has a
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
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
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:
Eighteen lines of comments for a destructor (that is obviously a
destructor for the
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
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.