ReviewBoard: Indispensable and Rather Spiffy

Code inspection is demonstrably one of the most powerful tools for preventing defects in software. For our own part, we who produce Saros have an inspection policy, whereby all but the most trivial changes committed to the version control system (VCS) must gather a minimum of two approving votes before being allowed. But how do we manage this?

In the past, patches were passed freely around on the project's mailing-list. Team-mates could take each other's patch files, apply them manually and conversations proceeded in mailing-list threads. While it did the job, it was somewhat cumbersome and only grew worse when the team grew in size. To improve matters, we chose to use ReviewBoard.

ReviewBoard is a web application that links to a project on your VCS. When you make local changes and produce a patch file, you can then upload your patch to ReviewBoard, which will compare the patch to the repository version and manage the diff for you. By "manage the diff", I mean things like:

  • Produce a two-pane diff view, showing what has changed
  • Allow you to attach both general and line-by-line comments on the code
  • Make multiple versions of the same patch
  • Aggregate all feedback into a single thread
  • and so on...

In our time using ReviewBoard, it has become an extremely helpful tool indeed. I think it would cause us to go cold turkey if it were taken away (although perhaps not as powerfully as losing code completion would -- I've been there, it wasn't pretty) . This post is simply to express my own support and gratitude to the makers and to point out that any sizeable team of software engineers could do a lot worse than use such a tool to manage their review process (you do have a review process, right?)

I also thought about passing on some advice about using ReviewBoard, which we at Saros have gathered, but I've been beaten to it by others on the Interweb, most notably by KDE's Aaron Seigo. Otherwise, read a good software engineering handbook for the more general inspection stuff.

There are however a couple of additional tips I could pass on:

  • Due to one of the unusual quirks in the way Saros team functions, we typically have a only single team-member producing a new feature. Something like a new feature means a lot of new code and therefore a big patch. (Reviewing a two-thousand line patch makes me yearn for something less painful, like root canal work.) This is where ReviewBoard's multi-version helps.
      1. Our team's policy is that an in-progress feature should be broken up into small milestones and a permanent review request is opened for that feature.
      2. A patch is posted at each milestone and reviewed. It can still be updated at this point, but as soon as it is approved (at, say, version N) , all changes made thus far are therefore approved up to version N.
      3. At the next milestone, when the same review request is updated, you don't have to review everything, you just ask ReviewBoard for the differences between N+1 and N.
    This way, the effort required for reviewing a monster feature is kept under control.
  • ReviewBoard does a lot of work dynamically that requires it talks to your VCS. If, like us, your VCS is kept on a less than speedy server, ReviewBoard can be slow. We solved this by connecting ReviewBoard to a self-hosted, read-only mirror repository that is much quicker and makes for a much more enjoyable experience.
 

karl

 

3 thoughts on “ReviewBoard: Indispensable and Rather Spiffy

  1. How does this cope with the problem of domain experts? For example, in SeqAn there are a lot of disjunct modules with (usually) one “expert” working on each. The algorithms implemented there are not obvious so I’m wondering if approval-based commits could really work here. Any thoughts?

    1. @Konrad: That question gets us more towards code review in general rather than the tool. Nevertheless, my thoughts: Code reviewing has a number of different purposes, many of which are still useful when parts of it are expert-written, including:

      – catching defects (could be caused by anything unrelated to the domain, e.g. pointer exceptions)
      – readability/maintainability of code
      – architectural correctness
      – compliance to project’s “best practice”

      Code changes need approving according to such factors and really should be.

      Of course, one of the aims of good software engineering is to avoid having “gurus”, individuals who are the only people to know how a module works. Code review could be a way to fight against this tendency in “expert-oriented” systems and spread knowledge through a team.

Leave a Reply

Your email address will not be published. Required fields are marked *