What is Well-Commented Code?

WannaBeGeekGirl queries: "What exactly is well-commented code anyway? Can anyone suggest resources with insight into writing better comments and making code more readable? After about six years in the software development industry I've seen my share of other people's code. I seem to spend a lot of time wishing the code had better (sometimes _any_) comments. The comments can be frustrating to me for different reasons: too vague, too specific, incoherent, pointing out the obvious while leaving the non-obvious to my imagination, or just plain incorrect. Poorly or mysteriously named variables and methods can be just as confusing. In a perfect world everyone would follow some sort of coding standards, and hopefully those standards would enforce useful comments. Until then, any suggestions for what you, as a programmer, consider to be good/useful/practical comments? Any suggestions for what to avoid? Also, I usually work with C++ so any resources/comments specific to that language would be too."

Comments

Code Complete

[kimba]

I can absolutely recommend a book called Code Complete. Yes, it is published by Microsoft, but it is an invaluable language-agnostic guide to writing software that includes heavy doses of common sense regarding commenting, coding styles etc.

Re:Code Complete

[NewtonsLaw]

I can absolutely recommend a book called Code Complete [amazon.com]. Yes, it is published by Microsoft

Yes, that's on my bookshelf -- but, given the fact that they go to great lengths to point out the importance of checking for buffer over/under-runs and fencepost errors, one can't help wondering if (in the wake of all those critical bugs in IE/Outlook/IIS) any of Microsoft's own programmers have read it.

More "do as we say, not as we do" from Microsoft?

Use plenty of expletives

[oingoboingo]

Well commented code should definitely contain a liberal smattering of four-letter expletives, eg:


// no fucking idea how this works
obj.doMagic();


or...


//bet those fucking lazy cunts in the QA team don't pick this up
fileSystem.delete();


When your code is released as open source and becomes famous, people can amuse themselves by searching through the source code to find all the hidden expletives, sort of like easter eggs. If you work for a commercial organisation, you can sit back and enjoy the panic as the QA and release teams sweat it out trying to track down every last filthy utterance before shipping to a fucker...errr..customer.

Doxygen, etc

[Stary]

Tools like javadoc, or maybe better in your case doxygen can really help when it comes to commenting code... the idea is pretty much that you place a documentation comment before each function, or class, and so on, which usually makes the entire thing much easier. Having done that, I've found that only a few more non-obvious parts have to be commented within the actual functions.

It's been a long time but..

[NewtonsLaw]

It's been quite a while since I wrote any significant amount of code but after spending far too many years cutting code too early in the development process I eventually woke up to the fact that coding is the *last* thing you do (apart from testing and debugging that is).

First-up you need a good spec -- and the spec should include the user-interface details to the extent that you could actually write the user-manual from that spec.

Indeed -- if you can't write the user-manual from the spec then the spec is incomplete.

From the spec the programmer should develop the structure of the code in another document.

That structure document is repeatedly refined in a top-down process until you (eventually) reach a point where you're actually cutting code.

I was always surprised just how much easier it was when the code was written as the lowest level of the structure documentation.

Not only could you comment out the program structure document so that the compiler would ignore it -- but you ended up with absolutely accurate and comprehensive documentation built into that source.

Project managers love this technique (and when I was in a project management role I demanded it of my team) -- it ensures that technical and end-user documentation are no longer the bits that get left until last and thus are either very shoddily thrown together or, if the project goes really over-budget, not produced at all.

Of course, as we all know, there's a huge amount of temptation to just leap into coding at the earliest possible stage and leave the documentation until later -- because some stupid managers use number of code-lines completed as a metric of project performance -- duh!

If you're smart and use good tools you can selectively collapse and expand the in-source documentation so that when you're trying to get familiar with a module that someone else has written, you can descend down the structure tree one level at a time without the meaning being diluted by stuff that is at a lower level.

Unlike the days of interpreted BASIC, there's very little overhead involved in integrating documentation and code these days -- so there's no excuse not to do it.

If required, the documentation can be automatically extracted from the source -- but by keeping the master copy in the code it becomes easier to ensure synchronization as changes and updates are made during the lifecycle of the project.

Document the function's contract

[IvyMike]

Take a look at this function, and tell me if there's a bug:

void foo(void) {
int* x = 0;
int y = *x;
}

Easy, the bug's the SEGV, right? Take a look at the same function, this time with comments:

// Function: cause_segv
// Description: Causes a SEGV for testing purposes
void cause_segv(void) {
int* x = 0;
int y = *x;
}

The point? A bug is unwanted behaviorm, but that only makes sense if you've defined what the correct behavior is. My example is trivial, but often this is a real concern. Function "bar(int,int)" returns null whenever one of the arguments is negative--is that a bug or a feature? Your function has a goal in life, a contractual obligation to do something; make sure it's clear what that something is.

Note that if you choose good function and good variable names, a simple one or two line comment at the beginning is usually sufficient to document whe function's intended behavior.

I also find that an "assert()" or two on the arguments at the top of the function makes it clear what values the function accepts, and which one the function doesn't handle. It's an easy way to document the contractual obligations of the function.

Stuff not to put in comments is stuff that's easily devised from the code. Check this out:

// Function: square
// Inputs: int x
// Outputs: int
// Used by: pythagorean(int,int)
// Description: returns x squared
int square(int x) { return x*x; }

Did the "Inputs" or "Outputs" add any value? That information appears again, two lines below in the function definition, and it's guaranteed to be correct there (unlike the comment which will be out-of-date and wrong when we change "square" to work on longs). The "Used by" might have added some value, if it was correct, but as it turns out it's out of date, and 15 other functions now use "square". Any information better derived looking at the code should be left off. Any information which can be better found using "grep" or "find in files" should be left off. Any information that will probably be out of date at some point should be left off. Heck, in this situation even the description is probably extra verbiage, since it doesn't really help anyone. (I'd probably put it in out of habit anyway, though...so sue me:)

Examples..

[popeyethesailor]

# all of these will also get moved elsewhere

# this is the worst damned warning ever, so SHUT UP ALREADY!

# Keep your friends close but your enemies closer.
# Or ignore them, we don't care.

# You know, we do assume comments are linear -Brian

Refer here for further details:)

Re:Variable Names

[emag]

On one of the last projects I worked on, the specs we received from the customer were horrendous. Actually, it wasn't the customer themselves who had done the specs, but another contracting firm. Spending 5 months on the project, and finding repeated errors in the "data maps" (it was apparently too bloody difficult for us to be supplied with a schema for the DBs we were supposed to be accessing and updating), I'd finally had enough.

Querying the DBs directly showed that the data maps were works of pure fantasy in several spots, or would lead to outright data loss if followed precisely. In a fit of pure...creativity...I ended up setting a "$workAroundFuckups" variable, and in the sections where it was needed, had a false evaluation do precisely what thee datamaps said, which would corrupt data. If the variable was true (ie, non-zero), it would work correctly, which meant ignoring the data maps and doing what was needed to have the data be entered correctly.

I ended up getting moved to another customer (due to the limited resources *we* had, not because of my creativity), so I don't know if the remaining folks on the project removed it after I left. When I added it, I explained to them precisely why I'd added it, and since they'd had similar experiences with what we were given to work with, were behind me 100%.

This wasn't even the *only* part of the project which was FUBARed, but it was unfortunately what I spent many a 15+ hour day dealing with, so I was rather familiar with it. Had I access to the server that *read* the data and used it, I probably would have just gone in and redesigned everything "for free", just to avoid having to deal with such a horrible layout.

This is also the client where, after a few months of an irksomely out of sync clock (off by 12 hours...made figuring out when something happened a bit of a PITA), I finally went in and set the damned clock to the proper time. Not surprisingly, the same folks who made that wonderful novel for us were the ones admining the dev server we were working on. AFAIK, no one ever noticed that the time suddenly became "correct" either.

Re:type* var is evil

[emag]

Oh, you've really touched on a sore spot. At a company I worked for once, there was a group of managers and developers who were working on coding standards for the entire division. Somehow, since *my* manager knew I was a fairly proficient coder, and wanted to make sure our group had input, I ended up on the panel. I remember telling a manager for another project point blank that he was an idiot for insisting that:

char* foo, bar;

was good coding practice, while

char *foo, bar;

wasn't, because the code was declaring two pointers, and so the * should be with the type and not the variable name.

Even pulling out K&R, and writing sample code showing the sizeof(foo); vs the sizeof(bar); wouldn't convince him that he was wrong.

Unfortunately, I don't think it was ever "officially" settled. Nor were several of the other corrections that I immediately made to his "proposed" coding standards document he handed out at the first meeting.

Thankfully, my manager at the time listened to me (and also, helpfully, knew C and C++), so when we got the coding standards, they were filed with the rest of the useless paperwork we got, and we kept on writing things properly, including:

• comment blocks before each function describing usage, parameters, expected range of return values, and error conditions
  
• comments describing thee amount and type of testing done to verify things worked
  
• comments about who had done what with what code and when
  
• comments preceeding anything non-obvious about the code itself
  

Three guesses as to which project was ahead of schedule. (Of course, not entirely fair, since we also didn't force code generation via Rational Rose. We instead reverse-engineered all of our final UML from the code we'd written and tested, and knew worked the way it was supposed to...)

My favorite comments

[bentini]

I personally despise comments such as:

i++; //increments the variable i

I think that they are unclear and do not properly explain the situation. Remember, you're writing so people can UNDERSTAND the code, not so that you can impress them with how smart you are. Instead, strive for a comment like this:

i++; /*changes the value stored in the space referred to by i to be the sum of the old value stored in the space referred to by i and the constant 1. Note: In C, this may cause what is known as a "silent overflow" if the value is too large, and go so far as to make a large positive value into a larger negative one. Oh my!

This way, people who read your code not only understand your program, but all programs. I really think that each function you write should repeat a semester's worth of computer science theory and programming practice, so that anyone who reads your code will learn from it. Remember, not everyone knows idioms, and why should they? And since we all write open source on slashdot, many novices are going to have their introduction to any computing environment by looking at the code you write at any point.

Your most humble and obedient servant,
Dan

Re:Variable Names

[Grab]

Sometimes, from other ppl. If I see it, it goes right back in review, and I won't pass the review until the fuckwit responsible has removed them. If you're writing code for yourself, then fine, please yourself. If you're writing code that anyone else will see, *especially* the customer, then hell no.

Thing is, there's two essential things that a reviewer/maintainer has to understand about a program: what it does; and why it does it. It should be possible to work out the first one of these just from the code, so long as the variables and functions are named sensibly. The second can be worked out from code with some effort, or the coder can add comments to explain why they're doing things that way and make it easier for maintainers.

But if someone has deliberately given all the variables names which don't reflect what they do, then it's utterly impossible to work out what the code is doing, and it's therefore also impossible to work out why it's doing it. So the code is unmaintainable - it isn't possible for anyone else to pick it up and work out what it does, except with massive work. If in 6 months time your company says "oh, we've got this code we can use with slight modifications, let's quote 1 month to do this contract" and then they find out you've made the code utterly obscure, then they'll crash and burn. And if that happens, the company *will* fire (or at least formally discipline) the person who wrote the original code, bcos they've been grossly negligence in doing their job. And you can kiss goodbye to any reference from them, so you'll be SOL in finding your next job.

Grab.

Things to bear in mind

[Rogerborg]

• All modern compilers that I know of can handle symbol names of at least 256 characters, not the old 31 character limit.
• The most widely understood naming convention is the English language.
• If you feel that you have to comment the purpose of a method, function or variable when you declare/define it, why isn't it necessary to comment every use of it as well?
• The time spend typing or reading characters of code is insignificant compared to the time spent comprehending it.
• Whenever you write code that requires any interpretation at all, you cost yourself developer time, and that's a precious resource.
• If you comment something that the language supports, you're not using the language.
• People who slate you for using over-verbose naming are really saying "You shouldn't need long names to understand my code." That's a solipsistic ego trip, as the target audience isn't or your peers or anyone in a code review or with white box knowledge of the code or system. It's the poor contractor shmuck five, ten or fifteen years down the line who has to come to your code stone cold on to fix a critical bug with a deadline breathing down his neck and a hankering to get the hell out of the office and have some semblance of a life. Write for the benefit of that guy, because one day you'll probably become him.
• Every time you write a comment, you introduce a potential headache for the maintainers. Ask yourself when the last time you updated a comment in production was, even when it contradicted the code.

Here's the rules I use:

• A function/method name is too long when it doesn't fit on a typical screen. 80 characters is about my limit.
• If I find myself thinking that I'd better comment the purpose of a variable, I incorporate the comment in the variable name. As a side effect, that also tends to give a good feel for how important a variable is.
• Yes, we all know that "i" is a counter, but what is it counting? It costs me perhaps five seconds to use a variable that describes what is being counted. Then it costs a reader an extra tenth of a second to read it, but that saves a quarter of a second to translate to what it actually means. Let people read your code, don't keep making them stutter and recap.
• Describing the function and purpose of "input" and "output" parameters in a function description comment is a hell of a long winded way of typing "const WhatTheParameterIsActuallyUsedFor". You only have to type it once; that's what copy and paste is for. Don't comment expected values, assert(them).
• Completely self commenting code is an unrealistic ideal. But get as close to that ideal as possible, and don't be afraid to change comments when you change the code during maintenance. If you're sure what the code deos, you should have no problems doing that. If you're not sure what the code does, then find out.

Favor Code Clarity Over Comments

[Bob9113]

In any situation where I see the need for code commentary, I try first to find a way to make the code clearer. If the source code is sufficiently clear, comments are unnecessary. This also avoids the risk that the comments will diverge from the code - making claims that were once true, but no longer reflect the code's actual logic.

This is poorly commented code (despite the fact that the comment is clear and accurate):

aClassName = aTask.getUiInitializerClassName();
// empty or null uiInitializerClassName means this task is not
// defined for use in this interface. Skip it.
if( aClassName != null && ! aClassName.equals( "" ) ) {
... do something ...
}


This is well commented code (despite the fact that there are no comments at all):

initializerName = aTask.getUiInitializerClassName();
boolean isNotNull = initializerName != null;
boolean isNotEmpty = ! initializerName.equals( "" );
boolean definedForThisUi = isNotNull && isNotEmpty;
if( definedForThisUi ) {
... do something ...
}

Of course, this doesn't work in all situations, but I find that I can improve the clarity and accuracy of seventy to eighty percent of my commentary this way.

Re:Good Comments

[MadAndy]

Changes have to be done twice? That's right, when they change the code, they must change the comment.

I'll repeat that: they MUST change the comment. And it must make complete sense when they're done or they'll be out of a job!

Why is this important? When you change the comment, you must think about the comment. You must think about the change you've done and how it fits in with the rest of the code, and what the rest of the code is trying to do. If a comment isn't up to date or doesn't make sense, that's a bug in the code, as bad as any other, and it needs to be fixed.

It's not difficult to spot when the comments don't line up, so they're fairly easy to fix. While you're there fixing the comments you need to check the code, 'cos whoever the idiot was that wrote it, they obviously haven't checked it properly. Go and hit them with a Very Big Stick.

Certainly you shouldn't whine about the extra typing. A little extra typing shouldn't hurt - and you should be able to type faster than you can think, so it shouldn't really slow you down. If it does, go take a typing class.

And if your lame excuse is that you're in too much of a hurry to maintain comments, just make sure you're not in too much of a hurry to deal with the bug reports that come back because you haven't checked your code properly.

Re:Make the variable names mean something!

[Mr. Slippery]

If a method has more than a screen full of code (i.e. about 20 lines), split the method into multiple methods

I strongly disagree. The proper delineation of a function or method is the operation that it abstracts, not how long it is.

If a subroutine is only called once, and doesn't cleanly abstract some idea (i.e., if you can't tell me what it does in one simple sentance), it should not be in a separate subroutine.

I've seen too much code written in the manner you suggest, that makes the reader bounce around from function to function to function for no reason other than "otherwise that function would be more than 30 lines".

void foo()
{
foo_part_1();
foo_part_2();
foo_part_3();
}

If I have to maintain such code I always refactor it into one subroutine.

It's sophomores like you...

[Pollux]

...who make reviewers like me stare at computer screens for endless hours trying to figure out how the hell your computer code is supposed to work.

Comment sparsely. Do not sprinkle your code with comments. Especially do not use comments like

Yea, I can already picture your programming style. You'd make a 200-line function with the only comment being " // Creates hash table ". Question: Where does that leave me? When I find out that there's some problem in the hash algorithm, I have to dig through 200 lines of code to find some freakin' bug that is described only by "Creates hash table." Your example of why comments don't need to be made is a poor one:

// increment loop counter
loopCounter++;

That is adding zero value.

Yes, because it's one line of code, and the code is described through the variable. But when sifting through lines of code, you often find beautiful works like iHateMyJob++; or fuckMyBoss--; to name a few. And needless to say, they're uncommented in the code. Until computer code can be written bug free in complete English sentences (aka Never), the rest of your team of workers needs to understand what your code does.

Personally, I make sure every function says what goes into it, what comes out of it, and what setup (variables, etc.) need to be made for it to be called. I do not comment every single line of code, but I do make sure that every line is accounted for by descriptive sentences, explaining the task that I wish to accomplish as well as what variables / registers / actions I take to accomplish the task.

Every time someone has to change some code, you've just forced them to double their workload, and change some comments too.

Okay, this just pisses me off. You didn't mean what you said. Here's what you meant to say:

Every time I have to change some code, you've just forced me to double my workload, and change some comments too.

I can assure you, from a reviewer's point of view, comments SAVE my time from trying to understand what each piece of code is trying to accomplish. Commented code may make you work extra time to detail the lines of code (I do admit, some programmers are quite tallented at keeping track of every single line of code in their head as they work on it on the computer), but it saves tremendous amounts of time once that chunk of code needs to be integrated with other chunks of code into the final product.