Frictionless Code Reviews

Having a second set of eyes on your code has multiple benefits.

Besides increasing the odds of catching bugs earlier in the process when they are less expensive to fix, code reviews tend to yield higher quality code that is more maintainable because you get feedback from someone who has not been immersed in the code and thus does not suffer from the blind spots that can be caused by working on something for too long.

It also leads to greater cross training in the code base (the hit by a bus factor) as well as knowledge sharing about coding practices in general. I’ve learned a lot of new techniques by simply reading code written by my co-workers.

Unfortunately, regular code reviews seem to be a rare practice in the industry because they are expensive in terms of time and difficult to manage due to egos. Most developers are very sensitive about the code they write and therefore it’s difficult to offer criticism without it being interpreted as a personal attack.

Even if reviews are built into a software shop’s SDLC, code reviews are often viewed as an ancillary activity and thus are usually first to be cut once a deadline looms (which is pretty much all the time in my experience).

Pair programming is one popular way to tackle this problem. It has all the benefits of code review, but bypasses the emotional issues by turning the review into a collaborative exercise. It also makes reviews more likely to happen because it becomes intertwined with the development process.

Although I clearly see the conceptual benefits of pair programming, I must admit that I am never very excited to participate in pair programming sessions on a regular basis. It’s probably just due to my introverted nature, but I feel most productive when I am in “the zone” and I can never seem to attain that state unless I am alone and in an environment with few distractions.

An alternative approach to pair programming that still makes the code review process inexpensive, stress free, and an integral part of the development process is to make it a required step of the development workflow and then adopt tools that make the process quick and painless to perform.

If code review happens all the time, then it ceases to be a stressful activity. If review is a required step before testing can occur (e.g. a step in the deployment script queries the issue tracking system and stops the deployment process if any unreviewed issues are returned), then it is no longer ancillary and avoidable. If tools help make it a frictionless process then it won’t be an undue burden that kills productivity.

I think we’ve achieved a relatively frictionless review process at StoreFinancial with the help of JIRA, Fisheye, TortoiseSVN, and a SVN hook.

First we ensure that a valid Jira key is entered into the comment field of the TortoiseSVN commit window with the help of a custom pre-commit hook that queries Jira to verify that the issue exists and is in an opened or resolved state before allowing the commit. I’ll describe how I did the hook in my next post and provide the source code.

If a developer forgets to enter the key or fat fingers the entry, then the code commit fails.

We originally purchased Fisheye to allow fast and sophisticated source control searching, but by far the biggest benefit we gained was the automatic association that happens between source code files and JIRA issues and the ability to view code directly in the browser. Within seconds of doing a commit, links to source control files will show up on a tab of the issue.

This allows the reviewer (in our case whoever finishes their developer tasks first) to grab an issue and quickly review it by simply clicking on the diff links of the Fisheye tab. The color coding scheme makes changes easy to see and the fact that it is within the browser minimizes the number of clicks required to do the reviews.

After reviewing all the files associated with the issue, the review can either mark the issue as reviewed or reopen it with comments and reassign it to the original developer to make the changes.

There are still parts of the code review process that I would like to streamline more.

In particular, I don’t like being a code formatting nazi so I’d like to move to shared ReSharper templates that automatically enforce formatting and syntax standards.

Otherwise, I’m quite pleased with the current state of our process and tools. With the addition of some simple tools it went from being expensive drudgery to being an relatively painless and valuable part of the development process.

Popularity: 1% [?]

8 Comments so far

  1. Nathan on August 28th, 2009

    Nice post. These look like interesting tools. Out of curiosity have you tried integrating Review Board (http://www.review-board.org/) into your process? It make code-reviews virtually painless.

  2. marshal on August 28th, 2009

    Have you checked out crucible? http://www.atlassian.com/software/crucible/ integrates into fisheye and jira

  3. Russell Ball on August 29th, 2009

    @Marshal – We actually did evaluate Crucible back in the Spring. I was enamored with the GUI magic that allows you to add comments inline and associate them directly with lines of code. However, we ultimately decided against it because it seemed more geared towards a formal review process with multiple people, complex workflow, and heavy collaboration needs. We were looking to streamline our review process but that would have ended up adding a minimum of a half dozen clicks for each issue. It was a shame because we really wanted to make it work for us.

  4. Russell Ball on August 29th, 2009

    @Nathan – I’ll give it a try. Thanks for the recommendation!

  5. dru on August 31st, 2009

    @Nathan: you beat me to the question about http://www.review-board.org/

    -d

  6. Jesse Gibbs on August 31st, 2009

    Hey Russell, you might want to take a look at Crucible again, as we’ve just released version 2.0. Atlassian’s dev teams use agile methods (pick n’ pull elements of Scrum/Kanban/XP/TDD), and code review is a key part of our process. Crucible does not force you into a heavy-weight process nor does it make you particularly agile. It’s just a tool for collaborating, similar to email, wikis, etc.

  7. [...] We use it to ensure that developers enter a valid Jira key into the commit notes, which is a crucial step in making our code review process as painless as possible. [...]

  8. Shane on September 17th, 2009

    Hey I just found this look also might be useful for everyone
    http://confluence.atlassian.co.....RAEXT/JIRA Commit Acceptance Plugin

    Now if i could only figure out how to make jira automatically create crucible reviews after an issue is closed.

Leave a reply