Jump to content
 

What every embedded programmer should know about ...

  • entries
    29
  • comments
    15
  • views
    2,150

Contributors to this blog

Comments are a code smell

Orunmila

66 views

Comments

I was musing over a piece of code this week trying to figure out why it was doing something that seemed to not make any sense at first glance. The comments in this part of the code were of absolutely no help, they were simply describing what the code was doing.

Something like this:

// Add 5 to i
i += 5;

// Send the packet
sendPacket(&packet);

// Wait on the semaphore
sem_wait(&sem);

// Increment thread count
Threadcount++;

These comments just added to the noise in the file, made the code not fit on one page, harder to read and did not tell me anything that the code was not already telling me. What was missing was what I was grappling with. Why was it done this way, why not any other way?

I asked a colleague and to my frustration his answer was that he remembered that there was some discussion about this part of the code and that it was done this way for a very good reason! My first response was of course "well why is that not in the comments!?" 

I remember having conversations about comments being a code smell many times in the past. There is an excellent talk by Kevlin Henney about this on youtube. Just like all other code smells, comments are not universally bad, but whenever I see a comment in a piece of code my spider sense starts tingling and I immediate look a bit deeper to try and understand why comments were actually needed here. Is there not a more elegant way to do this which would not require comments to explain, where reading the code would make what it is doing obvious?

WHAT vs. WHY Comments

We all agree that good code is code which is properly documented, referring to the right amount of comments, but there is a terrible trap here that programmers seem to fall in all of the time. Instead of documenting WHY they are doing things a particular way, they instead put in the documentation WHAT the code is doing. As Henney explains English, or whatever written language for that matter, is not nearly as precise a language as the programming language used itself. The code is the best way to describe what the code is doing and we hope that someone trying to maintain the code is proficient in the language it is written in, so why all of the WHAT comments?

I quite like this Codemanship video, which shows how comments can be a code smell, and how we can use the comments to refactor our code to be more self-explanatory. The key insight here is that if you have to add a comment to a line or a couple of lines of code you can probably refactor the code into a function which has the comment as the name. If you have a line which only calls a function that means that the function is probably not named well enough to be obvious. Consider taking the comment and using it as the name of the function instead. 

This blog has a number of great examples of how NOT to comment your code, and comical as the examples are the scary part is how often I actually see these kinds of comments in production code! It has a good example of a "WHY" comment as follows.

/* don't use the global isFinite() because it returns true for null values */
Number.isFinite(value)

So what are we to do, how do we know if comments are good or bad?

I would suggest the golden rule must be to test your comment by asking whether is it explaining WHY the code is done this way or if it is stating WHAT the code is doing. If you are stating WHAT the code is doing then consider why you think the comment is necessary in the first place. First, consider deleting the comment altogether, the code is already explaining what is being done after all. Next try to rename things or refactor it into a well-named method or fix the problem in some other way. If the comment is adding context, explaining WHY it was done this way, what else was considered and what the trade-offs were that led to it being done this way, then it is probably a good comment.

Quite often we try more than one approach when designing and implementing a piece of code, weighing various metrics/properties of the code to settle finally on the preferred solution. The biggest mistake we make is not to capture any of this in the documentation of the code. This leads to newcomers re-doing all your analysis work, often re-writing the code before realizing something you learned when you wrote it the first time.

When you comment your code you should be capturing that kind of context. You should be documenting what was going on in your head when you were writing the code. Nobody should ever read a piece of your code and ask out loud "what were they thinking when they did this?". What you were thinking should be there in plain sight, documented in the comments. 

Conclusion

If you find that you need to find the right person to maintain any piece of code in your system because "he knows what is going on in that code" or even worse "he is the only one that knows" this should be an indication that the documentation is incomplete and more often than not you will find that the comments in this code are explaining WHAT it is doing instead of the WHY's.

When you comment your code avoid at all costs explaining WHAT the code is doing. Always test your comments against the golden rule of comments, and if it is explaining what is happening then delete that comment! Only keep the WHY comments and make sure they are complete.

And make especially sure that you document the things you considered and concluded would be the wrong thing to do in this piece of code and WHY that is the case.

 

 

  • I Agree 2


2 Comments


Recommended Comments

I completely agree with you!

I try to avoid comments, because they tend to de-synchronize with the actual code very easily.

#pragma config PWRTE = ON    // Power-up Timer Enable bit->Power up timer disabled

What does that mean? Is it a bugfix? Is it a bug? I find this so often in mature projects. Someone edited bit masks for a register, but forgot to update the comments.

Even the Hungarian notation as an extended version of commenting de-synchronizes eventually, because you tend to ignore the prefixes after a while.

uint32_t u16counter;

So my personal approach is to find identifiers and names to formulate a readable code, which says everything what is actually happening on that specific line of code. There is no need to write functions with many lines of code to make the compiling more efficient or save stack space, modern compilers will optimize that out again. Encapsulating a partial solution not only brings structure to your code, it makes the function actually readable. And with readable I mean to read it out loud in front of an audience (e.g. for a code review).

Comments should give the reader a general overview of the problem and an abstract strategy what has to be done in a function. If the purpose is obvious and easily derivable from the identifiers, why create a second meta layer which needs extra maintenance and creates a dependency?

This in turn often means that an over-commented code doesn't have a good structure or the author doesn't understand the problem well enough to abstract it. Although sometimes the project dictates code metrics, like a code to comment ratio that has to be satisfied. I see a lot of projects with Doxygen comments in it, but the actual content of that documentation is rather unhelpful.

  • Like 1

Share this comment


Link to comment

Absolutley, and nice examples!

Hungarian notation breaks the abstraction of having a variable name with unspecified underlying storage, so I think it is the worst way to leak implementation details!

 

Share this comment


Link to comment
Guest
Add a comment...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
×
  • Create New...