On Code Reviews

May 31, 2001

One of the best ways to improve the quality of your software is also possibly at the same time the most overlooked and the most effective use of your time. I'm talking about performing reviews on your work products: requirements, designs, and code. Below I'll discuss doing a code review. Similar principles apply to reviews of other work products.

According to Watts Humphrey, "Doing reviews is the most important step you can take to improve your software engineering performance." [Hu95] At the core, reviews are about manually inspecting a work product for defects. We generally think of other people inspecting our work for defects, but it is also possible (and highly effective) to review your own work for defects before (or instead of) presenting it to others for review.

Several different activities come under the heading of reviews. These include code inspections (sometimes called Fagan Inspections, after Michael Fagan's seminal article on the topic [Fa76]), walkthroughs, and code reading. [Mc93] The main variation among these is their level of formality, although they vary slightly in emphasis and also in effectiveness. All three of these activities are performed in a group setting. The solitary review that you perform on your own code most resembles what I'm calling code reading.

In a code inspection, the most formal of the three techniques listed here, participants have well defined roles. A meeting is called whose purpose is to inspect a certain piece of code. Participants are given printouts of the code to be reviewed one or two days before the meeting. This is important as the reviewers need time to prepare. At the meeting, there is a moderator who has (should have) training in how to properly run the meeting, there is the author of the code, there is someone to record the defects that are found, and there are reviewers. The meeting could have as few as three people, and could probably be effective with up to six people. If there are more people, it will likely end up as a waste of time for some. Note that the author of the code should not be the moderator or the recorder. During the meeting, the code is inspected line by line with participants pointing out defects as they are found. The recorder keeps track of where defects are found and possibly the type and/or severity of the defect.

For an effective inspection, participants should use a checklist to ensure that common defect types are more easily caught. Also, be sure to keep the meetings to under two hours. Attention and energy fades after this. Don't invite management to the meetings as they can stifle frank, open discussions. Inspection results should never be used as a basis for performance reviews.

Code inspections have been found to be highly effective at discovering defects. Much more effective than testing. In various case studies, it has been found that inspections save time (this includes the time spent performing the inspections) and result in far fewer defects. [Fa76 and Mc93]

I'm not going to discuss walkthroughs, I don't think they're really worth the effort. Code inspections were developed as an improvement to walkthroughs. [See McConnell and Fagan if you really need to know.] Note that Fagan found inspections 38% more effective than walkthroughs in his study.

Code reading is my personal favorite for performing reviews. In my personal experience (I am in no way talking about any kind of scientific study here), code reading is more effective than a formal inspection meeting. When using code reading, no meeting is even really needed. Here's how it goes. The author drops off code printouts to each reviewer (he could also email the code, but I prefer to work from a printout). The reviewers go over the code line by line individually, using a review checklist. When a defect is found, the reviewer marks the defect with a pen. If performing "paperless" reviews, you should agree upon a marker to be placed in the file prior to the review (e.g. "FIXME"). I have found working from a printout slightly easier, but it is mostly just a matter of preference. When finished, each reviewer turns in his results to the author and the author fixes the defects. It is possible to have a meeting to turn in the marked-up printout and discuss each defect, but this just seems like a waste of time to me. A group at AT&T had the same experience. [Mc93]

It is probably obvious at this point that code reading could be done by a group of just one person: the author reviews his own code. This is highly effective and is used heavily by the Personal Software Process (sm) described in [Hu95]. The principle is the same: using a checklist, you read over the code looking for defects. In his book, Humphrey presents loads of data from students that have taken his course. Nearly all of his test subjects show dramatic improvements in the level of defects in their code after they begin performing reviews. His book also provides a very detailed method for performing reviews. I highly recommend either purchasing a copy or borrowing the book from the library at your local university or larger library (or get your boss to buy a copy for your group, it is a great investment).

The point of performing reviews is to find and fix defects. The "and fix" part is important! After getting feedback from a review, be sure to fix all of the defects that were found. If you don't, you're wasting your reviewers' time. If your organization is instituting a review procedure as part of the development process, be sure that verification is part of the procedure. It is important to be sure that defects are fixed, and fixed properly. It is possible to inject a new defect while fixing another one. When major defects are found that affect large sections of code, it is usually a good idea to have that section of code re-reviewed.

I've mentioned the idea of a review checklist a couple of times. This is important to performing a highly effective review. It helps the reviewers to look for the types of defects which have been effective in the past. Which of course implies that you have to keep up your review checklist. If, during the course of a review, you discover a defect that looks familiar, add it to the checklist. Odds are that you'll come across it again. The opposite is true too. You have to prune the checklist occasionally. If all of your coders (because of the checklist, possibly) have disciplined themselves to stop making == vs = errors, or off-by-one errors, and you no longer see this defects during reviews, then you should take these items off the checklist. They no longer serve a purpose and you can't get the checklist get too long or it becomes ineffective. One to two pages is a reasonable length (both sides of one page is probably best, but I have no hard evidence to back this up). Humphrey goes into more detail on the maintenance on review checklists. [Hu95]

It takes a fair amount of discipline to keep doing reviews. Especially at first. It will take some time for people in your group to become top-notch reviewers. Like anything, it takes practice. So it may feel for a while like you should instead be debugging and unit testing instead of "wasting time" reading code. If your organization does any kind of measurements, though, I'm sure you'll be very pleasantly surprised with the results. Later, after you get out of the habit of code/compile/test/debug and into the habit of code/review/fix/compile/test/debug, you'll realize how much less time you spend doing compile/test/debug and how much less time you spend in total developing programs.

Finally, a bit about automated code inspection. These kinds of tools have been around for a long time. Lint is a good example. This program is used to check C programs for common errors, and has been around for quite a long time. Most modern C compilers will provide warnings for common coding errors if you turn the warning level up high enough. If you have access to a version of lint, or your compiler supports this kind of behavior, be sure to use it. Any extra "free" inspection is worth it. I say free because you have to compile your code anyway, so you might as well have the compiler be strict about it. The same with lint: once you get it set up, it costs very little to run it, and it has the benefit of an extra pair of "eyes" to inspect your code. For best results, make this a part of your build process (i.e. build lint into your makefiles).

Over the past few years, a new breed of more powerful code inspectors has been developed. [Co99] These systems analyze code at various levels, finding both real defects and areas which are likely to be defect prone and should be targetted for further review (either manually or with other automated tools). These tools may work on a number of principles, depending on the information they are given. The number of defects that have been found in a given section of code is a good indicator that more defects are probably lurking in that same section. The complexity of a piece of code is also correlated to the number of defects that it probably contains. A number of different factors contribute to code complexity, but things like deeply nested loops or if statements, the use of gotos, multi-level switch statements, and very long functions all increase code complexity. I've also heard rumors about code inspectors that bombard your code with gamma rays and report which lines sprout defects. Other than lint and various compilers, I haven't used any automatic code inspectors, so I can't say much about how well they work. The new stuff that is mentioned here is (of course) new, and I haven't read much about its effectiveness, so I can't say much about whether they're worth the money. (These tools and services tend to be pricey, so I don't know whether you're better off performing inspections and testing in-house or not.)

The bit about gamma ray bombardment was a joke. But if you invent such a thing, give me a yell.

References