Coding for easier and quicker code reviews

If your company doesn't do code reviews, you most certainly should. If you're bound and determined to ship crappy software by not doing reviews, this post won't do much for you.

Code is meant to be read

Many people are under the mistaken impression that their code is meant for the computer's consumption. While it's true that a computer will consume your code, it is not who you're writing for. Programmers are your intended audience. That includes you. Coding a new project is the easy and short part of the software's lifecycle compared to the time code spends in the maintenance and upkeep phase.

So, assuming that you want to write code that's easier to read as well as easier to review, here's a few tips. All of them come from dealing with real coworkers. If you've worked with me and recognize something that you still do, you should feel bad. You're probably the reason I wrote this.

Descriptive variable names

Yeah, that's just good software development practice, but it can really help during code reviews as well. A short variable name that doesn't describe what it does is much harder to figure out during a code review since most review tools (at least that I've used) don't give a huge amount of context around the change. So while someone going in to edit the code might quickly figure out what the variable is supposed to contain, a reviewer has to dig.

Exceptions: Obviously loop control variables should be short ($i, $j, $k) as well as variables that use a single letter commonly, like representing coordinates ($x, $y, $z).

Useless commit messages

"Fixed a bug" or "Made change suggested in review #1234" are nearly useless. While they may be factually correct, they don't really tell the review what and why you're changing something. In the first case, the reviewer has to figure out what the bug was before they can decide that you've fixed it (and fixed it in the best way). In the second case, they have to go back to the original review to figure out what the suggested change was (and whether it was a good idea in the first place) before they can determine whether you'd fix the original problem.

Exceptions: If you've got a code sniffer (PHPCS, for example) that complains about things, having a commit message of "PHPCS" is plenty clear that you're appeasing PHPCS.

Trailing commas

This one is a bit controversial, especially if you frequently switch between PHP and javascript. PHP allows trailing commas for arrays while certain paste-eating browsers don't allow it in their javascript parsers. Creating an array with the trailing comma makes later reviews easy to read. For example, if you have an array like:

    $letters = array(
        'a',
        'b',
        'c'
    );

and you need to add a new letter:

    $letters = array(
        'a',
        'b',
        'c',
        'd'
    );

code review packages will show two changed lines:

    $letters = array(
        'a',
        'b',
        'c',
        'd'
    );

If you always include the trailing comma it makes it obvious during the review that you're only intending to add a new element. This is a trivial example, but it helps to show your change's intent better, which helps a reviewer better understand what you're doing.

Code reviews are meant to help you

Don't get your feelings hurt when someone points out mistakes that you've made. Everyone is trying to get better. If you send shorter review requests that are easy to read, it's much more likely that you'll get responses quickly and with less confusion.

Consistency is the key

http://www.flickr.com/photos/richard-g/3549285383/

In my last post, Keeping it simple, I wrote about a few things that can make you a better coder, or at least a more valuable member of a coding team. This is the next step down the path of coding nerdvana.



Style isn't just for the stylish


Every coder has their own preferred style. Left to our own devices, we tend to write code our own way. As long as you're the only one looking at the code, this isn't a problem, but consistency should still be valued.

If you're part of a team, you hopefully have a well documented style guide that everyone follows. Hopefully it covers the gritty details so that developers don't get into fist fights with each other about style differences. The last thing that a growing code base needs is for you to be able to tell who wrote a piece of code without checking your source control's blame log.

But what if it doesn't cover a style point?

Stay consistent!


Perhaps it's my upbringing as a military brat or my overly-logical thought process, but I just can't handle disorder in code. It seems so simple to me, but I see code like this entirely too often:

function foo($bar, $mitz) {
    if ( 0 == $bar ) {
        doSomething();
    }
    if($mitz == 0){
        doSomethingElse();
    }
}

There are some very valid reasons to write your if conditions one way or the other (0 == $bar instead of $bar == 0). I'm sure there are people that will make arguments about whether to put spaces outside of the if condition parenthesis, or extra spaces inside them. But doing it two different ways in a single method is just crazy.

Mixing styles in your code makes it an order of magnitude more difficult to read. Code that is hard to read is hard to maintain. Your code will spend more time in maintenance than in development, so why wouldn't you do everything you could to make it easier to maintain?