What to Look for in a Code Review
You have all the right software developers in the conference room, ready to pore over the project's code. But now& what do you do? We share tips from experienced programmers.
CIO — Several other articles in this series on running effective code reviews have covered when you should schedule reviews, who should participate, special challenges for teams that aren't geographically collocated and the role of tools. But at some point, you eventually will draw several adults into a conference room and you'll shut the door. Now what? In this article, I give you explicit guidelines about the process and the things to look for.
First, set the ground rules, and get a workable process in place. That means ensuring that participants have the source code in time, establishing roles for the meeting participants and putting a process in place for follow-up after the code review.
For the review to be a success, says Jason Cohen, Smartbear Software's founder, eliminate the parts that make developers unwilling to participate. "Identify the parts of the review process which is busywork or dead time and eliminate them," he says. "The only activity that's clearly not a waste of time is 'critically looking at code and talking about it with others.' Everything else is extra."
For example, developer Adwait Ullal sends a notice out a week before the code review, ensuring that the meeting will have three peer reviewers, plus a scribe and the author. Reviewers are notified of the artifact under review and its location. Ullal sends a reminder three days before the review, in which he tells everyone that they can bring notes to the meeting (not the artifact source code); only the author can bring the source code. (That eliminates last minute reading.) The scribe takes notes, and sets a deadline for when the fixes must be made. After the code's author fixes the issues, he sends out one e-mail message to let the team know.
Be sure you budget enough time for the review, and for the reviewers to examine the code ahead of time. If the reviewers aren't prepared, says Ben Sweet, principal engineer at Lear Corporation, the experience is horrible. "The result is a 'walk through' in which somebody reads through the code line-by-line to the meeting participants. This is not very effective and extremely boring. In these types of reviews, people are likely to identify cosmetic flaws that don't really affect the quality of the product."
Theron Welch, a software mentor helping Microsoft build a team in China, suggests one fun way to ensure everyone arrives prepared. "We have a simple rule that says, 'If you have no red marks on your code printouts, you're not allowed into the review.'"
There are lots of variations on that process, and some developers get very passionate about them. (This will not surprise you. Is there anything in the development process about which programmers will not get passionate?)
For instance, Cohen's "extras"-reduction includes gathering files for review and sending to others: the source files, checklists, requirements documents, associated defects, test specs, whatever. "Integrate with version control to generate differences automatically, or use a commercial peer review tool that does the same," he advises. "Typical home-grown version is to e-mail diffs automatically upon check-in, but this assumes you want to review post-check-in instead of before, which most people don't like. Checklists and other docs should be hosted on an intranet system so they can be linked to, hopefully automatically."
Also, Cohen objects to the typical Scribe process of recording comments that requires lots of time writing, "On line 142 of file //depot/foo/bar/..." because both reader and writer need to look up the code in the IDE. Spreadsheets or tools help, he says; or develop a shorthand like "filename:linenumber," adding a path only when disambiguation needed.
Not everyone wants printouts. Jeff Benson, a technical lead at Geneca recommends you use a projector that's connected to the source control system. "That way, you can drill in and out into unanticipated parts of the system," he says. "It's also easier and more comfortable to keep a small group of developers focused if you're all looking at one place up on the wall instead of leafing through a sheaf of handouts, trying to keep up."
