Code Reviews

We recently decided to review all code produced by our team. By our team I mean the team which I’m currently coaching. I’m really happy that it wasn’t actually me who suggested this but it was one of the latest additions to the team. This senior developer had in his previous job used a tool called Review Board with good results. Good thing was that one of the other teams in our company had actually taken this tool in use already earlier so only things we needed to do was for all team members to signup for an account and ask Review Board admins to create a group for us.

I was really amazed, it took less than two hours to get all developers (and two TA engineers) to start using the Review Board. First reviews were made already during the same day.

We use Review Board with SVN, so the process goes like this:

  1. Developer does the changes to the codebase locally
  2. Developer runs svn diff >changes.diff in his local working directory
  3. Developer opens Review Board front page and clicks a button to create a new review request
  4. Developer writes a short explanation and uploads the diff of changes to Review Board
  5. All other developers get e-mail that there’s a pending review
  6. Developers go and read through the review request and give comments
  7. When enough “Ship It!” comments received, original developer commits the changes in and closes the review request

The process is still fairly light so it gets followed. In the beginning we had some problems, one developer didn’t put all his changes in RB, but committed some “obvious” changes directly. Also in the beginning the review requests were rather large so it was really laborious to review the changes. Soon people learned to not only put smaller review requests but also develop in smaller pieces.

After a while, the developer who originally came up with the idea of taking Review Board in use, started disapproving all review requests where there was code without unit tests, unless there was a very good reason for those missing. As a result, we now get decent size changes covered with unit tests. And this is good.

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s