Comments inside the body of a function should apply to the state of the system at the point the comment “executes”.
Here’s an example of poor comment placement, taken from an actual code sample I was reviewing (though I’ve done some anonymization):
// Widget is already vibrating, so we update the waveform in place.
// Else the waveform parameters will be set when we start vibrating.
if (waveformParameters != null) {
waveformParameters.Shape = WaveformShape.Square;
widget.UpdateWaveformParameters(waveformParameters);
}
When I encountered this comment, I read it as telling me that the widget is already vibrating. I’m thinking, “How do I know that it is vibrating? Shouldn’t we be checking for that first?”
And then I see the “else” part of the comment, and I get more confused, because why are we talking about what we do if the widget is not vibrating, if the previous sentence told us that we (somehow) already know that that it is vibrating?
Next, I see the if statement, and now it’s checking whether something is null, which I guess tells us whether the widget is vibrating. But the first sentence of the comment said that we knew that it was vibrating.
Oh, I see. The comment is really describing what we know to be true once we are inside the if block.
Here’s a less confusing way of writing the comment.
if (waveformParameters != null) {
// Widget is already vibrating, so we update the waveform in place.
waveformParameters.Shape = WaveformShape.Square;
widget.UpdateWaveformParameters(waveformParameters);
} else {
// Nothing to update right now. We will set the parameters
// the next time we start vibrating.
}
Each comment describes what happens when execution reaches the block of code it is in. I even created a dummy else block to hold the explanatory comment about why it’s okay to do nothing.
If you really want to put the comment prior to the “if” statement, you need to structure it to match the state of the program prior to the “if” statement.
// If the widget is already vibrating, then update the waveform in place. // Else the waveform parameters will be set when we start vibrating. if (waveformParameters != null) { waveformParameters.Shape = WaveformShape.Square; widget.UpdateWaveformParameters(waveformParameters); }
FInally, an article I can understand! Hi Hi.
Keep them coming.
The only hard and fast rule about comments is that everybody else does them wrong. But statically, even I did them wrong, in the past, since my code probably functions differently now than it did then.
Solution: no more comments.
Seriously though, better variable names would make this at least as clear as the actual comment attached, esp. if there was a clearer API to check for ongoing vibration than this != null approach shown here, which seems to be overly dependent in implementation details that could change.
Have an upvote.
I also write code without comments and strive to make it readable by using proper naming and not doing more than one thing per function.
I cringe whenever I see code peppered with comments where you first need to spend considerable time to verify whether the code and the comments match before actually being able to analyze the code itself. It is just doubling the workload for no benefit to anyone. If someone can't write readable code then there's a high probability they won't be able to write a coherrent comment either and in most cases it turns into...
Now with AI assisted programming, comment placement is even more important, since if you place the comment in the right block, you have much better chance that the code that it generates/suggests at that spot will be correct.
This is why you find the best comments in branches that should never execute. At least the authors of such expletive lines stuck to this rule.
Not sure I agree. I very much prefer
;*** Begin Configuration UI ***
drawscreenanew:
to
drawscreenanew:
;*** Begin Configuration UI ***
The same applies to higher level languages; comment before function decl not after it.
But sometimes it really is better inline (had to look around for a case without angle brackets to hose the post)
do {
buflen *= 2;
results = new byte[buflen];
IOException exception;
do {
result = NativeMethods.readlink(blinkpath, results, results.Length);
} while (IsEIntrSyscallReturnOrException(result, path, path, false, 0, false, out exception));
if (exception is object) throw exception;
// Documentation suggests we should never go around this loop but …
} while (result == buflen);
You mean like:
// Should never get here but good luck proving it
followed by code that handles the case
I settled on this reasoning a few months ago; glad to see my decision validated!
not a huge fan of any of those comments because it doesn't explain the relationship between "already vibrating" and "waveformParameters != null" .. a better comment would be "if we have waveformParameters (therefore the widget is already vibrating), we update in place; otherwise it is done later when vibration starts" ... comment language should be fluent, using "if then else" is a bit robotic, so I dont see an issue with mixing it up to "if, otherwise"... in rare cases if the logic is a bit complicated (negated), using "unless" instead of "if" works too... a comment should only exist...
I agree that the relationship between “already vibrating” and “waveformParameters != null” is implicit. The issue I was trying to illustrate was the comment placement, not the comment wording (which also could use work).