Let's talk about comments.
There are a lot of varying philosophies about comments and when and how often to write them. Some people think comments are good, some people think comments don't belong in code at all. I don't think I'm telling anybody anything they don't already know when I say: the right approach is somewhere in the middle.
But that's not fun to write about. I present to you:
Top 10 Problematic Comments
I started writing this article with a serious tone. I have now abandoned that. Most comments are reprehensible. These comments are the worst. They'll rot your code, infect your repository, and keep your company from competing in the marketplace. If you see comments like this in your code, delete them immediately, then make a pull request. And then cite this article when the reviewer complains.
10. The Rotten Comment
This is a comment that persists despite its original purpose having been lost. The code around it changes, making it obsolete, sometimes these comments are obvious.
// Run the status check 5 times just to be safe:
for(int i = 0; i < 16; i++)
runStatusCheck();
And sometimes it's not as obvious.
// It is very important to call these functions in this order:
initResources();
initNetworkComm();
Is it? Maybe that used to be important, but then somebody repaired initResources() and initNetworkComm() so that they no longer irrationally depend on each other. Who knows? Only the code knows.
9. The Commented-Out Old Method
/*
// We don't do it this way any more
if( argv.size() < 3 )
{
LOG( "Not enough arguments given. See docs." );
exit(0);
}
*/
checkArgumentsFailGracefully(argv);
"We don't do it this way anymore" Thanks, asshole. What kind of weird cowardice is this? Just delete it, revision control will keep track of the way you used to do things.
8. The Code-Parroting Comment
Every so often I see code like this:
// This function initializes the AES event handler
void initAESEventHandler()
{
// Allocates AES handler with a size of 20
mHndlr = allocateAESHandler(20);
// Set the ticks per second of the handler to 60
setAESHandlerParameter( mHndlr, AES_TICKS_PER_SECOND, 60);
// Set the sample-rate of the handler to 44k.
setAESHandlerParameter( mHndlr, AES_SAMPLE_RATE, 44100);
}
Just imagine for a second, that you don't know what AES stands for. Obviously these comments don't help you. In fact, they don't help anybody, and yet, I see stuff like this all the time: incomprehensible code surrounded by equally incomprehensible comments.
I can only imagine some jerk, in a fit of mentorship, said
At this company, we comment our code
And a young, impressionable engineer thought they needed to write all this crap to fit in. Thanks for nothing.
The whole idea of commenting every line dates back to a time when languages were inherently hard to read. We don't live in that world now, languages are better, they've evolved to a nice middle ground where they are readable by computers and humans. Get with the times.
7. The Naively Optimistic Comment
This is a comment that says that the code works, when it actually does not. Suppose a server request that is meant to get sent after a 5 second delay, but instead it's getting sent immediately. You trace the bug to this code:
// After 5 seconds...
dispatch_after(
dispatch_time(DISPATCH_TIME_NOW, 5e6),
dispatch_get_main_queue(), ^(void)
{
// ... send a request to the server.
[WebClient sendRequest];
});
Now, unless you know that dispatch_time takes a duration measured in nanoseconds, you probably will be very distracted by a comment that assumes the code does what it's supposed to.
6. The Standard Header
I open a file and the first thing I see, at the top of the file is this horseshit:
/*****************************************************/
/*****************************************************/
/*** Everhard Corporation ****************************/
/*** Standard Source Header Comment ******************/
/*** This file is called utils.c *********************/
/*** Written By Jeff Hanson **************************/
/*** September 20, 2009 ******************************/
/*****************************************************/
/*****************************************************/
Who the fuck is Jeff Hanson? Does he even work here anymore?
We all use revision control now. What is the point of putting creator and created-at date at the top of a file? All that information and more is recorded in revision control.
Sometimes these goddamn things take up a window's worth of lines, and they're completely irrelevant. Seriously. What bug do they fix?
If there's a name at the top of the file, I like to immediately email that person whenever something even-a-little-bit goes wrong with the code in that file. They could be a tech lead on another team, at another company, I don't care. "It says here, you wrote this code..."
If you are a potential employer reading my blog to learn something about my personality as an engineer before hiring me on, you should know that I'm going to write a script to remove all of these from your codebase.
5. The File Name at the Top of the File
At the top of a file called ZucchiniSlicer.cpp you'll see:
//////////////////////////////
// File: ZucchiniSlicer.cpp //
//////////////////////////////
And then, later
class ZucchiniSlicer {
// ...
};
This is just stupid. It's so stupid that it deserves special mention even though it's usually part of a bigger problematic comment: The Standard Header.
I've never seen a text editor in my life that didn't display the name of the file prominently at the top of the window. We don't need a comment at the top of the file. Who does that help? Also, every so often somebody will make a new file by duplicating an existing file, forget to change the comment, and then it's wrong.
4. The Content-Free Warning
Maybe you've got some Android Java code designed to detect if a particular class is present in your game engine that you're using.
// DO NOT remove this temporary variable.
String className = "com.engine.Object";
Class<?> theClass = Class.forName(className);
What! Why?!?!! It's only used in that one place. I want so badly to remove it, and you didn't give a reason. How about:
// Putting the class name in a temporary variable
// works around a warning in the obfuscator.
String className = "com.engine.Object";
Class<?> theClass = Class.forName(className);
At least maybe I can figure out what that's about.
3. The TODO
def treat_files(pathlist):
for path in pathlist:
f = open(path, 'r')
s = f.read()
f.close()
if not s.endswith('\n'):
f = open(path, 'w')
f.write(s + '\n') ## TODO: use append-mode instead.
f.close()
Fuck you. I've never gone back and done a TODO in my life, and neither have you, you prick. Stop showing me all the places you failed to follow through on some fleeting idea for an optimization.
2. The Code Snippet Label
Okay, this one is actually important, because it's an anti-pattern. A function gets long and complex, and instead of dividing it further into functions, you label each block inside with a comment that says what that block does.
void Game::initEverything()
{
// First, set up the graphics engine
glBlabityBlah();
glSomeMore();
glEtCetera();
// Then, prepare the audio stuff.
alCreateWhatever();
alMakeChannelSomething();
alConnectPieces();
// Then put up the splashscreen for the game and
// play a sound effect.
glCreateSomeGeometry();
glDrawStuff();
alSproing();
}
FOR CHRISSAKE!
void Game::initEverything()
{
initGraphics();
initSound();
initFirstScene();
}
void Game::initGraphics()
{
glBlabityBlah();
glSomeMore();
glEtCetera();
}
void Game::initSound()
{
alCreateWhatever();
alMakeChannelSomething();
alConnectPieces();
}
void Game::initFirstScene()
{
glCreateSomeGeometry();
glDrawStuff();
alSproing();
}
Which brings me to the final category of problematic comments.
1. Any Comment Explaining How Code Works
If you find yourself writing comments that explain the code around them, it probably means your code doesn't connect well enough to your mental model. Instead of trying to bridge the gap with words, try reorganizing your code so that it reflects how you naturally think.
I'm not saying all comments are bad. There are rare circumstances where a comment is necessary, like the Android Java example above. But the distinction there is: the code does something weird to work around a systemic issue. Mostly, I think you'll find you can convey more meaning by organizing the code and choosing sensical variable and function names.
I'm for documenting at the public function level. If other people's code calls a function you write, write a description of what that function does, not how it works. That's what I think.