Most Popular Basic JUnit Tutorial Getting Started with Subversion JMatter-- Naked Objects that actually work! Test Driven Refactoring Blending C and Ruby Subversion Best Practices Nine Things Developers Want

The Archives 2005 2006

Wed, 14 Mar 2007

The Peer Code Review

Code reviews have been so abused in our field. So much so that I'd like to rename the idea I'm writing about today but I can't think of a better name... it is a code review, but not a traditional code review. So for now, I'll call it a Peer Code Review or PCR.

A PCR is simple, fast, and lightweight. When you finish adding one feature (or fixing one bug), you stop working, get your code, and go find a teammate. I like to print diffs, but the screen is fine too.

If someone is busy, don't interrupt. Don't break anyone's flow for this review. Ask someone if they have time for a review... if they say "No", then keep looking. Don't wait for (or ask for) a more detailed explanation. It would break their concentration. We can usually handle a minimal interruption without losing our place, but anything more than just a brief "Got time for a quick review?" is too much.

When you do find someone with time, first tell them what you were trying to do. Sometimes our intention is very different than our implementation. :) It's good to know what you're trying to do before looking at what our code actually does. Knowing our intentions helps the reviewer catch the mismatches.

Then walk your reviewer through the code, line by line and explain it to them. Remember I said to add one feature or fix one bug? The goal is to have a small amount of code to review. This helps make your automated tests more effective, avoid code integration issues, and let's you have small, fast code reviews.

As you explain your code, you're "Rubber Ducking" (also called "Talk to the Duck") from the The Pragmatic Programmers. By explaining your code to another person, you'll often find your own mistakes.

Another benefit of code reviews is raising your team's Truck Number or Bus Number. When we explain a part of your code to another person, we share a little bit of information with them. Over time each person on the team will have a very thorough understanding of the entire system.

Also, I find that I write my code differently when I know that someone else will look at my work and provide feedback. On more than one occasion I've been coding, gotten frustrated, and started adding hacks to the code... taking shortcuts, just trying to get something working. And then I've realized that Jim or Will is going to look at this code... and they'll call me on the sloppy work. So I take a step back and code something that's cleaner, more maintainable, more understandable, and less likely to be riddled with bugs. And that's before the review ever happens.

As a reviewer, don't get stuck on the insignificant details ("No, the brackets must go here!!!"), and only reject code that's actually doing something really bad. Bad data should always be rejected. But what about naming conventions? I once reviewed some code that had a hash table named mr_hashy... Mister Hashy was a bad enough naming convention that I rejected the code. :) Sometimes N or X is a fine naming convention (in a loop maybe), but if a variable is holding real data, X doesn't cut it. Remember, as a reviewer, your name is being added to this code as well. Be sure it measures up.

Why do I prefer Peer Code Reviews over a traditional code review? The five minute rule. After five minutes in a room with twenty of your teammates and three months worth of code, every eye glazes. Here's a new saying for you... "With enough code, all eyes are shallow". ;)

I try to keep my PCRs short enough to avoid glazed eyes. Five or ten minutes seems to be a safe time limit.

If you have a longer feature that requires days or weeks to write, then try a rolling code review. Every day, review that day's code with the same teammate. This will get you a lot of the benefits of a real PCR while still letting you tackle a larger problem. But try hard to keep commits small.

When I'm the tech lead, I require PCRs on any code before we check it in to the version control system. There's something about changing code that has already been checked in to the version control system that people resist, especially for "smaller" changes, like meaningful variable names and stylistic changes. So try to get the review, make your changes, and get re-reviewed before you check in your code.

And if your team isn't using PCRs? Then start asking your teammates to review your code. Ben Franklin said that to make a man your friend, ask for a favor. You'll complement your teammates, improve your own code, and learn from everyone else's experience all at the same time.

Here's a quick summary of a few Peer Code Review benefits.

• Crosstraining (raising your Truck Number)
• You catch your own bugs
• Others catch your bugs
• Mentoring "as you go"
• A second set of eyes looks at every line of code
• Lightweight
• Fast

Peer Code Reviews are much easier to introduce than pair programming, but still very effective. Like using a good source code management product, you will need to use it for a while before you settle into a comfortable groove (clear the Improvement Ravine), but I think you'll find it a great way to work.

Enjoy!