The Case for Code Review

[originally posted on 7/10 to "Dev Blog"]

Code Review is a fun topic, and much like unit testing, it garners a lot of interest from dev teams -- and a lot of promises from dev managers -- but in the end it rarely sees the light of day. Code reviews just seem to take up too much time, and they can be difficult to organize and standardize.

But let us not forget: Code reviews are important. There are all sorts of studies and success stories that tell us just how important code reviews are. The stats suggest that code reviews generally catch between 20 and 70 percent of the bugs in the code being reviewed. That's a pretty good success rate!

Of course there are different methods for code review, and the specific development methodology that applies to your project will often dictate what types of code review are possible. For example, if you are working on a waterfall project (such as one using the CMMI methodologies), you may have time set aside in the schedule for design, then construction, then testing. The testing portion of the project would then be the best time to perform code reviews. The reviews should be built right into the schedule. The time you have allocated for code review may dictate which types of review you choose (I'll introduce a few types below).

If you're using an iterative development methodology (Agile, Scrum, eXtreme Programming, etc.), you'll want to build code reviews into each of your iterations. It is important in these iterative methodologies to find a code review technique that fits well into the tight iteration schedule. In any methodology it's important to make sure your code review technique is a good fit for your team's development style.

Whatever code review technique your team uses, it's important to have specific goals for the code reviews. In general, I've found that it's important to look for two things in a code review: Does the code do what it is supposed to do, and does the code meet the team's coding standards. Of course, it's important that the team actually have a set of coding standards to which the code is compared before that standard can be applied. I'll try to cover that in another article.

Here are some of the approaches to code review that I've been introduced to:

• Formal Inspections: Formal inspections are a highly structured form of review that uses a team of reviewers in specific roles, following focused checklists in their search for defects in your code. They require a certain commitment to a review team, and that can mean a lot of overhead. But the return is also significant: Generally formal inspections find 45%-70% of the defects in the code reviewed.
• Walk-Throughs: This is a very informal style of review, usually structured as a team meeting in which code is discussed in a matter-of-fact, highly technical manner. Experts on the team offer guidance, while other team members point out possible issues and ask pertinent questions about the code under review. While they can be quite educational, the return is low: only 20%-40% of defects are found using this technique.
• Code Reading: Code reading is a more independant style of review, in which the code being reviewed is delivered to the reviewers, who then return their findings (defects, suggestions, etc.) to the code's author, who then makes the proper changes. A meeting to go over the results is optional, and often of little use for finding new defects. The return for code reading is a bit higher than walk-throughs: 25%-65% of defects.
• Pair Programming: While not generally thought of as a code review technique, pair programming warrants mention here because it has review built right into the process of development, and because it is a great way to reveal defects in code. The typical return is 40%-60% of defects found.

My personal preference is strongly rooted in my preference for an iterative development methodology. Given a typical iteration length of one month, code reviews have to fit nicely within that month, and their time on the schedule has to compete with time for design, construction, testing, and deployment. Meetings can prove to be a burden for developers who are trying to maintain momentum on their development tasks, so I prefer a review process that is asynchronous. Of course some code needs more experts reviewing it than other code might, especially if it's going to provide any sort of interface to other systems.

I think a healthy combination of formal inspection and code reading fits the iterative development style well. For code that interfaces with other systems, formal inspection is important before the code is released, and should involve team members from the projects with which the code will be interfacing. For all internal code, the code should be delivered upon completion to all of the other developers on the team, who will then review it, returning their findings to the code author. All code should be reviewed in this way, and code should not be marked "complete" until each developer on the project has had a chance to review it. In any given iteration the amount of code reviewed should not prove to be overwhelming. It's important not to ignore the reviews in an iteration, simply because it will cause defects to pile up, and future reviews will take much longer.

That is, of course, just my preference. We'll need to find the perfect fit for each of our projects and methodologies.

It's important that we find a way to work code review, in one or more of its many forms, into the work we do every day. Whether it's highly formal and performed at regular intervals, or informal and part of our iterative process, our code needs to be reviewed before it goes live. Why leave our bug-finding to the testers? Why wait for our users to find defects, when our teammates can find them more quickly, more efficiently, and before they go live?

Besides: it's fun to find your friends' bugs.

Ss.