For me, code reviews have traditionally been one of those aspects of software development that fall into the “good idea; bad execution” category. It’s something that I know we’re supposed to do but when someone says, “let’s do a code review”, my first reaction is usually to make a cross with my fingers and start yelling “THE POWER OF CHRIST COMPELS YOU!” at them.

Unfortunately, our team is scattered across three time zones these days so that tactic is less effective than it has been in the past. Which means we’ve had to address the real problem: how do we make code reviews useful?

Our first attempt was to do them the tried-and-almost-true way. That is, we scheduled a code review meeting on Wednesdays and I’d send out notices to the other developers on Monday saying we’re reviewing these five classes. I sent the notes out on Monday to give people plenty of time because it wouldn’t be realistic to expect them to do the reviews an hour before the meeting starts now, would it…

Historically, code review meetings that I’ve attended/led have usually followed a familiar pattern. To summarize:

Meeting starts
Coding Hillbilly: Okay folks, I’m pumped to get started. There are some interesting things going on in this code and some awesome comments. Let’s take an easy one to start off: “This code is pretty coupled to the implementation. We should create an event bus to manage the communication between the various presenters.” I agree and let me tell you at length why I say that…

End of Hour One
CH: I’m going to have to cut off your overview of dependency injection frameworks, Arbuckle. You’re right, I think that will help us with the internationalization of our config files. But we’ve only covered three issues so far. The rest of them should go much quicker though. Let’s take a look at the next comment…”This should be split out into a REST service”…

End of Hour Two
CH: Moving on…”We shouldn’t need a separate interface for this. It’s just extra complexity. Just create a new instance of the implementation in the constructor.”………<sigh>…Agreed. Next…

End of Hour Three
CH: Next…”Move the hard-coded database connection string out of the vie---“…you know what? The rest of this code looks good to me. Any objections if we accept it as-is? Great. See you all next week.

In this little slice of life, I’ve also ignored an important aspect of code reviews with remote teams. In an attempt to keep comments as close to the code as possible, they are done as //TODOs inline with the code. And to avoid annoying merge issues, this means everyone needs to do the actual reviewing at different times. So there’s a lot of “Okay, I’m done. Have at ‘er” IMs thrown about.

As much fun as this is, I decided to give Rietveld a look. I stumbled across it when I noticed Philippe Beaudoin, the creator of gwt-platform, was using it for his project. It was easy to pick his brain about it because, y’know, he’s on our dev team.

Some notable tidbits about Rietveld. It’s based on Mondrian, the internal code review tool used by Google. And it was created by Guido van Rossum. As a general rule, if the creator of a piece of software has also written a widely used programming language, it’s worth a quick eval.

But it was the workflow of Rietveld that drew me in. This is our new code review process:

  • A developer initiates a code review by selecting a starting revision and an ending revision. The diff between the two is submitted for review to the selected reviewers
  • The reviewers look at the diffs (either as a side-by-side diff or, if you prefer a challenge, a unified one) and draft comments inline with the code on the web app (i.e. not in the code in the repository itself)
  • When a reviewer is done drafting comments, she publishes them to other reviewers and the initiator
  • The initiator reviews the comments, makes changes if necessary, and publishes updated code diffs in response to the comments (again, if necessary)
  • The initiator and other reviewers can keep adding comments back and forth. The comments are stored in a discussion format similar to how they appear in GMail.
  • Once all comments have been addressed (either through code or discussion), the code review is marked closed.

Here are the parts I love about this:

The reviewed code is a diff

We don’t need to review an entire class every time. We just look at what’s changed from one revision to the next. So we can focus on the functionality for a specific bug or user story. This makes it easier to implement a policy like “User stories must be code reviewed before they can be marked complete.”

At the beginning, this will be a pretty moot argument since diffs will include a lot of new classes. But even still, we can see *all* the changes that have been made for a particular story, including that “minor” change you made to the GetUsersTimeZone function in the global DateUtils class.

Review comments are an ongoing discussion

The comments are threaded in the interface and capture more of the discussion than our previous process, where one developer would add a //TODO and the rest of the discussion was done at a meeting. And speaking of the code review meeting…

NO CODE REVIEW MEETING!

This is by far, my favorite aspect of Rietveld. Once launched, code reviews are done asynchronously. Reviewers add comments whenever it is convenient (within a reasonable timeframe, of course). Other reviewers can chime in when they want. Once published, comments are immediately seen by other reviewers and can be commented upon further. For a disparate team like ours, this feature alone is pure 100 proof homemade moonshine. And finally:

The code review isn’t done after the meeting

In our previous process, the code review was all but forgotten after the meeting. There was no inherent follow-up process to make sure the changes had been made. Here, the code review remains active until it is explicitly closed. I.e. once all comments have been addressed to the team’s satisfaction.

sally-code-review.pngNow to implementation details. Rietveld is a Django app running on Google App Engine. There’s a publicly hosted implementation of it at http://codereview.appspot.com. There, you’ll find a truckload of OSS apps registered for it (and in fact, Rietveld is open source itself if you feel like looking at Python code written by the creator of Python). Nice thing about that is that you can see examples of code reviews in progress.

If, like us, you are leery about publishing your reviews publicly, you have at least two options that I know of to host it yourself. One is to create an application for yourself on App Engine and host it. There are instructions to do that and I’m sure they work. We didn’t try them because the other option is much easier. If you have a Google Apps For Your Domain account, you can add it directly from the Google Apps Marketplace. One click and it’s added to your account and only you and your domain users have access to it.

From what I’ve read, Rietveld works with Subversion, Mercurial, and Git repositories. From experience, they can be private repositories as well as public ones.

It’s probably too early in the process to be extolling the virtues of our new code review process. At this stage, Rietveld is kind of a rebound girlfriend for us because of the pain we felt with the old process. But early indications are that it will be a big hit.

If not, I suppose we could print out the code and fax comments back and forth…

Kyle the Encoded