This came up in a code review yesterday... I thought I had already written about it, but cannae find the article anywhere so perhaps I imagined it. On the other hand I apologise if I have already written about this and it's a repeat. Point it out to me if so.
So, anyway, my contention today is that comments in code are grossly over-utilised, and in general one should avoid them. The code I was looking at yesterday had a case of stating the obvious. This is not the actual code, but an analogous example:
// send the request
$result = $httpService->sendRequest(requestObject);
Now I'm not having a go @ my colleague here, as we've all done this. But there's simply no need for the comment there: it's completely clear from the code that a request is being sent. What's more it's an HTTP request. So in a way the code here is actually more clear than the comment is. But don't write a comment that simply paraphrases what the code already clearly states. We can all read code.
But what if the code itself is not so clear?
$result = $obj->getRecords($id);
One might think a comment would make things more clear:
// get the customer details
$result = $obj->getRecords($id);
The problem here is not a lack of comment, it's the code is poor. Fix the code. Don't make it worse by cluttering it up with comments:
$customerAccountDetails = $customerService->getAccountDetails($customerId);
There's no need for a comment now, as the code is self-describing. Your aim should be for your code to describe itself, not to have to add narrative to your code so as to explain.
I spotted another example of comments being bad at work just today actually. This is a paraphrase to protect the author (it's old code, I dunno who wrote it):
// uncomment this to have it render as an accordion
IE: the code in question had been removed at some point, but not the bleedin' comment. The comment is lying. We've all seen variations of this.
And what about commented-out code? No. Just don't do it. We use source control for a reason: get rid of it. If you need it back: get it out of source control. Let's face it... almost none of the code we've ever commented out "just in case" ever gets re-activated. I've seen commented-out code in out codebase from 2007! (I deleted it. We have thusfar survived without it).
And how do I know it was from 2007? Because of this shit:
// (2007-010-03 Bob McUseless) fixing JIRA-1234
/*
commented-out code here
*/
Again: you've got source control! Don't put comments that ought to be in source control (although that'd be a shit comment for a source control commit too, that said) in the code. Just sort the code out and commit it with a sensible commit message.
Another example of something that should be handled by source control is this sort of junk:
<?php
/**
* Created by PhpStorm.
* User: adam.cameron
* Date: 03/10/2014
* Time: 18:52
*/
And this is what PhpStorm does by default. So, first things first: f*** off, PhpStorm: you're stupid. Second... what's the point of rubbish like this?
- When I commit it, it'll be against my name anyhow;
- as soon as someone else also works on this file, it really becomes irrelevant who first created the file.
What say you've got a tract of code that's a hundred or so lines long, has nested conditional and looping logic in it, and really needs careful explanation as to what's going on? Surely commenting that code is OK?
[I can't be bothered coming up with an example of this, but you know what I mean]
Nope. Your code probably (almost certainly) needs refactoring. 100 lines of code in one context generally too much, and it's down to the lack of refactoring that means the code is complex. If you shove each of the subtasks (and 100 lines of code might have a dozen subtasks) into their own - well-named - methods, and suddenly the code describes itself, and also can all fit inside one's head (and viewport) at one time. No need for comments.
Another sort of comment that should go is this sort of thing (which I see fairly often, and have been guilty of myself):
if (someCondition){
[more than a screenful of code]
} // end of someCondition
Two things to note here;
- most IDEs and even half-decent text editors do matching-brace or -tag highlighting. Or jumping between opening & closing elements.
- Your code is too long. Refactor it.
Those're all the situations that spring to mind in which comments are unnecessary or poor. There'll be more, so let me know and I'll update the article. Seriously... almost every situation one might think a comment makes sense: it's dealing with a symptom, not an actual issue.
There's a coupla situations in which a comment might actually be helpful. One I can think of is to break down a complex regular expression into something readable. I can quote myself here ("Regular expressions in ColdFusion (part 6: syntax - flags and the odds 'n' sods that are left )"):
Say I was wanting to match a UUID/GUID, I could just have this:This is valid because regex patterns have a very terse syntax, and there's no scope for clarifying it. So comments help.
^[0-9a-f]{8}(?:-[0-9a-f]{4}){3}-?[0-9a-f]{12}$
Which is gibberish. I could clarify that as:
(?x) ## allow whitespace and comments ## This is a regex which matches: ## XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX ## XXXXXXXX-XXXX-XXXX-XXXXXXXXXXXXXXXX ^ ## from the beginning [0-9a-f]{8} ## eight hex digits (?: ## just a grouping for repetition -[0-9a-f]{4} ## a dash followed by four hex digits ){3} ## so three groups of that -? ## an optional dash (GUID has one, UUID does not) [0-9a-f]{12} ## the last 12 hex digits $ ## right to the end
Similarly sometimes a piece of code is best-served being terse, and can stand a quick explanation. Or something is counterintuitive, cannot be clarified, so a comment along the lines of "
// this is actually correct, despite appearances" might be (but still probably isn't) appropriate.Bottom line? if one finds one's self starting to type a comment... one should stop to think if there's not a bigger picture afoot.
Thoughts?
--
Adam
