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.

Building rock solid software in the real world

http://www.flickr.com/photos/preef/32995286/

Recently (2011-11-08) I gave a talk at the Dallas PHP meetup about building rock solid software as a team. For my first experience talking in front of a crowd since high school, I thought it went pretty well. Several people have asked for me to post my slides (which I did), but they were made in a way that doesn't really help people out if they didn't see the talk. The talk was recorded and is available on Ustream, but I thought it might be helpful to do a blog post on the topics I covered as well.

This post is mainly meant to aggregate links to the topics that I talked about.


Tools


I covered several tools. All of these should be available to the developers as build targets and run in your continuous build. Lint and your unit tests should be run as part of your submission process.

• lint - The bare minimum, it just detects syntax errors in your scripts. Code that doesn't pass the lint test won't pass any other tests or manual QA.
• PHPUnit - Standard unit testing framework. There is plenty of information about it elsewhere.
• PHP Code Sniffer - Detects code smells that should be fixed. Many bad programming practices can be written as "sniffs" along with most rules from your smile guide.
• PHP Mess Detector - Statically analyzes your code for possible bugs or coding practices that tends to hide bugs.
• PHP Copy Paste Detector - Scans your code to find large similar blocks which can be factored out to a common method.
• PHP Dead Code Detector - Scans your code to find code that can not be reached. For example, code after a return statement.
• Code coverage - Adding the xdebug extension to your system allows PHPUnit to calculate how much of your code is run by unit tests.

Code reviews

I talked about two different code review packages:

• Review Board - You install it on your server and host it yourself.
• Codereview.appspot - Use Google's code review tool.

And I talked about three different ways of doing reviews:

• Pre-review - Code doesn't get submitted until a peer reviews it. Keeps bad stuff out of your code base.
• Post-review - Code gets submitted, then gets peer reviewed. Comments made about the code may never get resolved, but code reviews don't slow down getting code into production.
• Public shaming - Put the code up on a projector and discuss as a team. Great way to destroy programmer morale.

I mentioned a few points about what to look for in a code review:

• anything the tools couldn't catch
• logic errors (like ifs that don't make sense)
• loops with off-by-one errors
• performance problems (SQL in a loop)
• things to refactor (large methods)
• or things they missed
• Style problems (not really wrong, but you know, wrong)
• Typos (variable names, documentation)
• Tests that don't have assertions
• Methods without tests

Style guides

There's two ways to choose a style guide:

1. Roll your own - Look at your existing code and build the style guide from what you're already doing.
2. Use existing - Such as Zend or Pear.

Integrating Subversion and Review Board

At Google, all production code must be peer reviewed before it is allowed to be submitted to their source control system. Since I'm no longer a Googler, I don't have access to their awesome infrastructure so I had to roll my own. Review Board is a pretty good code review system, and unlike the open source implementation of Google's internal one, you can host Review Board on your own hardware.

We use Subversion as our SCM. Out of the box, Subversion doesn't force code to be reviewed before submission, and I couldn't find any documentation about forcing code reviews through pre-submit hooks. Subversion's hooks are pretty powerful, and Review Board has a decent API, so I wrote my own. Hopefully this is useful to someone out there. It's written in PHP5. Put this in your subversion hooks directory, call it pre-submit, and make it executable.