Comments are evil. The voice of your code must be clear and precise. I don’t get it. Never have. Why do so many experienced coders use so many comments? It’s not DRY. Every time you repeat yourself those repetitions will go out of sync. Comments on all your </div> tags to tie them to their opening tag? You’ve probably got div-itis. Simplify the markup or replace some with HTML5 semantic markup.
There’s nothing new under the sun. Black’s the new brown. Or is it grey that’s the new black now? Anyway, trends wax and wane and it seems that commenting code is frowned upon once again. In this case though, I think the underlying reasons for the advice are much more professional than they used to be when I was a lad. Back then, comments were considered by the alpha hackers to be nothing more than crutches for inferior noobs. After a decade or so of coding standards mandating at least 20% comments in an effort to rein in the bucking genius of uber-geekdom, a modicum of moderation seems to have returned and the code police now suggest commenting only sparingly. Well, I’ve got news for you sister, there’s a new SWAT team on the block and we’re gonna wipe out every last stinkin’ one of ’em.
I’ve grabbed the source for the Apache HTTP server and analysed a few of the comments contained therein. I picked Apache for several reasons:
- It’s an excellent product so I can’t be accused of picking on a dead duck to make my arguments easier.
- It’s open source so the code base should be of better than average quality.
- It’s complex and representative of real world products rather than contrived examples.
I opened the source up in my IDE and I started working through the source files in the order it presented them to me. I looked at each comment and determined what steps would be required to remove it without damaging, and preferably improving, the code base. It soon became apparent that they each fall into one of a number of groups which I’ll discuss in a series of posts over the next couple of weeks or so. There’s a lot to cover. A surprising amount for such an innocuous topic. The categories will be presented in no particular order so expect some posts to be reasonably obvious and some to be contentious and subtle.
The vast majority of comments are there to hide bad process, bad design and bad code; all of which can be avoided. The only comments that should remain in place are those that hide bad language features or bad third party interfaces. Essentially, anything that you have control over doesn’t need commenting.
Comments are a bad code smell, transparent to compilers, never executed, cannot be tested and can only be verified by manual inspection. If you’ve got the time and courage to mess with things that band-aid over bad code, resist all attempts at automation, create manual work, introduce significant risk to a project and provide no benefit, then comment merrily away. I think they’re evil myself.
Confusing Definition With Calling
Many comments are placed with the provider of functionality rather than the consumer. For example:
#if 'A' != 0x41
/* Hmmm... This source code isn't being compiled in ASCII.
* In order for data that flows over the network to make
* sense, we need to translate to/from ASCII.
*/
#define NOT_ASCII
#endifThis code says nothing about the use to which it will be put. It provides information about the environment. The comment, however, does not explain how that information is being provided. Rather, it explains the use to which that information will be put. But it’s quite possible that other uses will be found for that information. The comment should live in the client code; possibly in a function called TranslateToASCII() which checks for NOT_ASCII with a #ifdef for example. But putting a comment explaining that we’re translating to ASCII in a function called TranslateToASCII() seems a little redundant. Bin it.
Redundant Explanation
Looking again at the same comment we can see that we haven’t really removed the need for the first line. That’s easy to deal with. Instead of some woolly name like NOT_ASCII which could mean a variety of things, let’s be more explicit. Why not call it CODE_NOT_COMPILED_IN_ASCII. No need for the first line of the comment then. As an aside, we should also reverse the logic and call it CODE_COMPILED_IN_ASCII. Obviously the client code would need to change to reflect this. Avoid negatives where possible. And there’s no need for a #define in this day and age. Use a free function like IsCodeCompiledInASCII() at the least.
Bad Third-Party-Interfaces
There’s often nothing you can do about third party code so you may sometimes need to do things like this:
/* affects include files on Solaris */
#define BSD_COMPThis #define warrants a comment. The name of the #define is cryptic and presumably not amenable to change. In this instance the comment is pretty dire but the fact remains that a useful comment would have been appropriate. The fundamental point though is that the comment shouldn’t be required. If someone had seen fit to use a decent name instead of BSD_COMP we would have been OK. Long term solution: make sure your public interfaces are expressive and your clients won’t feel the need to turn to the dark side.
It’s Lint’s Job
This type of comment is often found in the context of #includes:
#include <unistd.h> /* for getpid() */Presumably it’s so a maintainer can come along and attempt to remove dependencies. If getpid() is no longer called then the #include can be removed. Assuming anyone else who decided they wanted to call something from unistd.h had the foresight to update the comment. Regardless, this is a job for a static analyser. This kind of thing is amenable to automation and you should use the right tools for the job. Lint will pick up unused headers as well as a thousand other things so you should use it.
Comments For Prettiness
Stuff like:
/* ------------------- DEFINITIONS -------------------------- */is just pointless fluff. The language has features to delimit structure within your code where it makes sense to do so. Use those rather than pretty ASCII art. This one was found in the middle of a file where declarations and definitions were intermingled so it was wrong anyway.
Bad Naming
/* maximum number of requests on a time limited test */
#define MAX_REQUESTS 50000Many, many comments are there to explain a badly chosen name. Instead of MAX_REQUESTS use TIME_LIMITED_TEST_MAX_REQUESTS. The former is unclear. The latter does exactly what it says on the tin. And it does so wherever it’s used rather than forcing those who follow you to look at the symbol’s definition to find the explanatory comment. And no, it doesn’t mean more typing. Either use an IDE that supports native code completion or get a plugin that does.
Use The Right Primitive
int keepalive; /* non-zero if a keep-alive request */Make it a bool and the comment can vanish. Or maybe it’s the keep alive time in seconds. In which case it’s a really bad name accompanied by only half a comment.
Don’t Use Primitives
The use of primitives (int, string etc.) to store data which has more significant semantics (days in month, URL etc.) is a bad idea.
Using primitives in this way often feels like a natural thing to do at the time because it’s either the first time this concept has cropped up or a precedent has already been set and the primitive is already being used to model this type.
Worse, the use of a primitive is usually forgivable at this stage since the concept being modelled is usually very straightforward. A first pass model of a password might well just implement it using a simple string. Soon it becomes apparent that it should have a minimum of 7 characters. Then, a little later, it must have at least one punctuation character. And so on.
Often these semantics end up being articulated through the use of explanatory comments and adherence to these semantics is left to client code, or worse, the end user. For example:
/* Note: this version string should start with \d+[\d\.]* and be a valid
* string for an HTTP Agent: header when prefixed with 'ApacheBench/'.
*/
#define AP_AB_BASEREVISION "2.0.40-dev"</code>The comment can easily be expressed in code. If the symbol needs to match the regex \d+[\d.]* then make sure it does so explicitly in the code. This type of validation is easy to implement in a class’s constructor. Likewise, adherence to the Agent: header format.
Address the real problem and substitute a class for the primitive type as soon as it becomes apparent that there are richer semantics involved than the primitive type is able to model.
Don’t Use Cryptic Encodings
int verbosity = 0; /* no verbosity by default */In the previous example a bool seemed a natural choice based on the comment. Here, the comment is incomplete and ambiguous. It’s possible that a bool would suffice but there may be differing levels of verbosity, from 0 to 3 for example. Use either enums or alternatively, a const int for each level, in this case. Again, expressive code removes the need for error prone commenting. You need to express yourself exactly, completely and minimally in this line of work. Code is designed to do that. Human language is not.
Don’t Use Globals
This a valid, if tired old line. The reasoning here is a little different to the conventional. A symbol at global scope does not have the advantage of a limited context in which to define itself. For example:
int tlimit = 0; /* time limit in secs */If this was defined at local scope in a function called WaitForNextCommand() then you could be pretty sure that, if the required command wasn’t received within tlimit seconds, some kind of error recovery would be called for. The semantics of the symbol are fairly clear. When defined at global scope, that context is gone and the symbol could mean many different things. At local scope, simply changing the name to timeLimitInSeconds would probably be enough to both remove the need for the comment and to make the code much clearer. At global scope you need to pretty much write an essay to describe its possible uses. The existing comment is both insufficient in its current context and superfluous in the correct context. Use the structural hierarchy of your code to limit the amount of explanation you need to do.
Don’t Explain Obvious Code
/* simple little function to write an error string and exit */
static void err(char *s)
{
fprintf(stderr, "%s\n", s);
if (done)
printf("Total of %ld requests completed\n" , done);
exit(1);
}Both the example and the advice are obvious and require no explanation.
Represent The Real World
This one’s a little specific but it highlights a general principle. It can be difficult to tease out that principle from the code in question at times but it’s worth bearing in mind. Sometimes we become bogged down in the detail of the code without thinking about how the internal bits relate to the real world.
/* avoid divide by zero */
if (timetaken)
{
printf("Requests per second: %.2f (#/sec) (mean)\n", (float) (done / timetaken));
}Firstly, this will never produce an exactly correct figure since timetaken is only an internally represented approximation of the real time taken to do something. The variable timetaken can never be accurate. Obviously, if done and timetaken are both ‘large’ then the result will be pretty close. So, the question is this; if timetaken is zero, how many requests per second have we processed? The most sensible figure to present to the user is done requests per second. This includes the case when done is also zero. So, the code should be:
if (timetaken == 0)
{
printf("Requests per second: %.2f (#/sec) (mean)\n", (float) (done));
}
else
{
printf("Requests per second: %.2f (#/sec) (mean)\n", (float) (done / timetaken));
}You might want to say ‘Approximate requests per second’ and you should change the condition to check for equality since ‘timetaken’ is not boolean. Regardless, producing a result for both zero and non-zero cases results in better code that more accurately reflects the problem domain and gives a more consistent user experience with a value being reported regardless of the mercurial nature of CPU clock cycles. It also removes the need for the comment since the code is much more explicit about what is happening. The comment had a clue in it. The word ‘avoid’ stinks of shoddy workmanship. If zero ‘happened’, deal with it.
Replace Code Block Comment With Function Call
if (requests)
{
/* work out connection times */
long i;
...
lots more code here
...
}If you have this kind of comment, it’s ripe for extracting the calculation to a separately called operation; WorkOutConnectionTimes() in this instance.
Explaining Algorithms
/* calculating the sample variance: the sum of the squared deviations, divided by n-1 */
for (i = 0; i < requests; i++)
{
...
}This is the most often cited ‘legitimate’ use for comments. Balderdash is what I say. The example above should obviously be in a separate function called CalculateSampleVariance(). And anyone can look up the formula for variance if they don’t know it already. No need for a comment. Ditto for something more complex like the DitterWibbleMeyerkovskiWizzySort(). Google is your friend here. If this is some complex algorithm that you’ve developed yourself then it will either be amenable to decomposition into smaller callable functions or you’ll have given it a name and written a paper that can be Googled. I’ve never come across anything that was too complex to understand purely by looking at the code and yet didn’t have any easily found external references explaining it.
History Block Comments
History blocks are frequently found at the top of source files. They give a detailed change history of the code and usually consist of a series of entries made up of date, author and change description. All this info is very worthy and useful but it only tells part of the story. You can’t use it to reconstruct the code at some point in the past and using it for anything else is pointless fluff. It’s very, very rare that history blocks are ever used for anything at all. Even when they are used I’ve never seen them be sufficient to complete a task on their own.
You always need to go back to your version control system. And guess what that stores. Ding. Dollar to the winner there. It stores the complete story. Date, author, change description and the deltas for the code change that was made. That’s what version control is for. It should store both the code changes and the config control info that goes with those changes. The two are inextricably linked and should be kept together. Source files can’t do that and that’s the main pragmatic reason history blocks are borked.
To Do Comments
ToDos and ‘known bugs’ comments are similar to history blocks in many ways. They often manifest themselves in less formal ways though. You’ll often find ‘Fix This Hack’ type comments littering the code. Sometimes you’ll find a list of known problems in a block at the start of the source file. The previous argument for history blocks applies equally well here.
Your todo list may take many forms, whether you use a set of index cards, a plain text file or something like FogBugz or Basecamp. The point is it needs to be external to the source code. In many ways todo comments scare me a lot more than history blocks. History blocks are in the past and that’s usually less dangerous than the future. It’s a fact that the vast majority of todo comments will never be addressed, especially the informal ones that just get dropped in the code whilst something else more interesting is happening. They just sit there lurking and waiting for the day when you have an especially important client exercising the dustier corners of your product.
There’s also an aesthetic argument why these comment types are not appropriate. I’ll keep banging on this drum without apology. Code should be beautifully architected and realised. They are nothing more than poor quality graffiti and allowing them to sully the fruits of your labour is selling yourself short.
Finally, they just don’t fit in a temporal sense. The purpose of code is to document the truth of the present. If you were to build and run your product now, then the code reflects completely what would happen. It says nothing about what would have happened six months ago. And it says nothing about what may or may not happen in the future.
Is this a ToDo or a rant? Either has no place in the codebase.
/*
* XXX: what is better; this hideous cast of the compradre function; or
* the four warnings during compile ? dirkx just does not know and
* hates both/
*/Code Comment Conflicts
/* calculate and output results in HTML */
static void output_html_results(void)So what does it do? Does it calculate and output or just output? Either get the symbol name right or… No. There is no ‘or’.
Design By Contract
DBC specs as commented annotations to a function signature are the closest real use I can think of. Some languages allow you to specify DBC structures explicitly and elegantly but none of the ‘mainstream’ languages do unfortunately. Asserts or exceptions, possibly wrapped as a Precondition(<condition>) macro or function etc., are probably better but cannot be defined as part of the signature in C++. On balance, I’ll always use explicit checks for preconditions, invariants and postconditions within the code itself where they can be tested. The loss in expressiveness of the function signature doesn’t warrant the duplication and confusion caused by inaccurate comments.
Complex Language Features
Some code is difficult to understand because the language features being used are complex. For example, many C++ developers are unfamiliar with templates. This often leads to developers explaining what the language feature does in the code. The code is no place for language tutorials. If maintenance programmers do not understand a language feature then either they shouldn’t be touching the code or the language feature is inappropriate for the skill level of the team. Either way ‘tutorial via comments’ is giving just enough knowledge to be dangerous.
Detailed Design Commenting
Using comments as a form of pseudo code during coding is common and sensible if it works for you. For example:
//for each widget that's in the reject bin
{
//dock x dollars from operator's pay packet
}What’s not acceptable is those comments remaining in place once the code has been written. Any temptation to leave them in can be refuted by applying one of the other comment ‘rules’. The best way to ensure they’re not left in for too long is to remove the // which leaves the code in a state that won’t compile.
Commenting On External Processes
Very often you’ll find comments that detail procedures for manually updating the code in light of external changes. It’s almost always a better solution to automate these processes.
For example dependencies on external libraries often require the presence of a minimum version. Storing this kind of info in comments is asking for trouble. It’s just far too easy to overlook. Besides which, one of the golden rules of software development is to catch errors as early as possible.
This kind of dependency detail can almost always be easily checked at compile time. So that’s where it should happen. If you find yourself writing comments in English to explain how to carry out procedures rather than writing the code that actually carries out the procedure, it’s often a sign that you don’t really understand what’s going on. That’s usually a bad thing.
Licence Info Comments
Quite often you’ll find a huge chunk of legalese garbage and disclaimers at the start of every code file detailing what can and can’t be done with the code.
These types of comments are usually duplicated in many files. As with any duplication, this results in repetitive work in the face of change and errors when some changes are missed. Over time this leads to divergence and hence possible misuse of the code due to differing interpretations of which is correct.
As with copyright warnings embedded at the start of DVDs, it’s annoying to have to scroll through irrelevant garbage to get to the good stuff, and it wastes developer time having to do so.
Besides any of the practical aspects, this is code and it’s supposed to be beautiful. Don’t allow it to be corrupted with lawyer speak. Just tell marketing all the extra comments required for their disclaimers will bloat the code and the boost in specs needed for the target platform will result in a 30% increase in TCO. They’ll understand this last bit and it will scare them. Tell them this even if you’re distributing compiled byte-code. In fact, just for fun, tell them this especially if you’re distributing byte-code.
The long term solution is for the industry to adopt the de-facto standard of placing this text in ‘licence.txt’ which is bundled with the code. In the short term, a very succinct reference can be made in each code file to point to ‘licence.txt’ e.g.:
//For licence info see licence.txt