2014-02-13

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.

No comments:

Post a Comment