Thursday, October 4, 2012

Code reviews

Photo by Sebastian Bergmann
We started implementing regular code reviews at Orbeon a while back, and to our great satisfaction the practice has stuck!

How do we do code reviews, concretely? Some companies have very drastic processes in place. For example, they won't allow anything into their master branch without a code review. And there's nothing wrong with that.

At Orbeon we have implemented something a little bit more lightweight. We do reviews on weekly basis. Each Friday afternoon we stand around each other's computer (yes, stand!), and we review every commit pushed to github since the previous review.

The programmer who has written a given change is the driver and explains the rationale behind it, pointing to non-trivial aspects if any. This results in discussions and feedback such as:
  • Discussing whether there might have been another (maybe simpler) solution to a problem.
  • Improving naming (one of the hard things in computer science).
  • Requesting that non-obvious code be commented better.
  • Discussing and explaining programming patterns.
  • Catching obvious little errors.
We take note of changes required and address them individually after the review is over.

We don't review necessarily every single line of code. Some changes can be complex (hopefully for good reasons) and it would take too much effort to dig all the way down. But at least we get an idea of the intent of the code.

In the end, what do we gain? A better understanding of our code base, especially parts we are less familiar with; action items to improve code we have just written; and finally, we learn and, I think, become better programmers. It's probably worth the 1 to 2 hours a week we spend doing reviews!

2 comments:

  1. Nice article. I agree on the challenge of good naming. Good names, even if they are a bit long, can make reading code a lot easier.

    ReplyDelete
  2. Thanks. On a similar topic I recently liked @dhh's post "Clarity over brevity in variable and method names" http://37signals.com/svn/posts/3250-clarity-over-brevity-in-variable-and-method-names

    ReplyDelete