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.
THE ULTIMATE GUIDE TO CODE REVIEWS
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."
Set Ground Rules
I've already covered some of the "attitude" issues in other articles in this series (see Running an Effective Code Review), but a few items are worth mentioning during the course of every review.
Engage in advocacy to encourage code reviews to become part of the culture, says Oliver Cole, president at OC Systems and also the lead for the open-source Eclipse Test and Performance Tools Platform project. "Make sure that you continually say things like 'We all know that this is the right thing to do' and 'Let's be egoless.'" He adds, "Once in a while throw in a 'Wow, these code reviews really are increasing the quality of our work' (even if it hasn't yet). (Heck, quality is hard enough to measure anyway.)"
One important suggestion is that the developer whose work is being examined be asked to explain her choices. Ask, don't tell.
Advises Benny Czarny, founder and CEO of OPSWAT, which makes development tools and data services for managing security features of endpoint applications, "Ask questions that are productive. Don't say things like, 'That isn't right' or 'Why'd you do that this way?' Ask for an explanation: 'Can you explain what this piece of code does?' or 'What brought you to this implementation?'" As the author answers your questions, Czarny says, have her insert comments into the code detailing the explanations.
"A reviewer should never demand code be changed without explaining why they believe there is an issue," says Czarny, and not just for ego reasons. The code author may gain development skills and draw her own conclusions about something the reviewer is highlighting. For example, let's say the code has missed a scenario that could become a vulnerability. The reviewer who notices this potential vulnerability asks insightful questions. As the discussion progresses, the developer has a 'light bulb moment' and suddenly recognizes the issues missed. "These are the types of experiences you learn from and draw from in future code development and review sessions," Czarny points out.
Senior software engineer Michael Doubez suggests that you make it clear that every reviewer will get one of his pieces of code reviewed. "It makes people more kind, knowing that one day they will be on the other side of the desk," he says.
Still, don't be afraid to criticize the work under review. Senior software engineer Alexei Zheglov once ran a review of code that was quite sloppy, had lots of copy-and-paste and just screamed out for refactoring. "I had an idea about how it came to be that way and it was far from being entirely the author's fault," he says. There was no point in criticizing the developer or saying he was stupid. Instead, though, "Simply say that the code is not as good as it should be, and here are the ways to fix that."
A few more bullet points before we get into specifics:
Critique the code, not the person.
Point out good things, not just weaknesses.
Don't focus on solutions to the issues you discover. No more than five minutes should be devoted to possible resolutions.
Don't take more than 25 words to describe a process.
Look For: Adherence to Style and Standards
Several experienced developers and IT managers point out that a code review should ensure that the software follows accepted guidelines for coding style.
"The most important aspect about the software code review process is first, clearly communicating the company standards for writing code: Here is how we develop, test and lay out code," says Jay Deakins, the founder and president of Deacom, Inc., an ERP software producer for the building component and batch process industries. "You should never be able to tell the difference between two developers' code."
But for your code review to ensure that the software meets company standards, then you have to have those standards—documented.
"To eliminate the silly code review comments like spaces or capitalization, you need a good style guide," says Microsoft's Welch. A style guide describes how the team prefers to comment; to name classes, variables and methods; and how to format. "It should not attempt to convey software best practices, like how best to deal with memory that can't be allocated, or tracking threads, and so on. It should only focus on style." The style guide should be short enough so that it can be easily grasped by everyone on the team, without covering every nauseating detail, explains Welch. Team members still need freedom to write the code the way they see fit. But with a style guide, he says, "Developers are then held accountable that they will have no style issues in their code. If the team has established good style guidelines which are readily accessible and used frequently, this ought to be easy."
A style guide can solve a lot of team problems and prevent silly fights—assuming they don't cause more of them. As software contractor Steve Silberberg points out, "No two people will ever agree on variable names, module formatting or coding conventions, nor will they ever come to an acceptable compromise." In Silverberg's view, code reviews that instill standards can cause resentment (sometimes severe) among those who must change their naming conventions. For some developers, the change markedly inhibits their productivity by making them re-program themselves to start thinking differently. "Coding standards arguments are fierce and territorial and will rip through the fabric of any team unity developed—all for modest (at best) gains of ease of coding maintenance," he says.
Look For: Everything Other Than Style
It's common to jump on style issues during a code review, but truly, there are better things for you to do, say many experienced developers. "It is easy to get distracted by the style of the code," says James Pitts, VP of development and program management at Embarcadero Technologies. "The wrong style won't break the product; it is just harder to read."
"The key to a code review is not to spend more time reviewing then it took to implement," says Pitts. "Fast and effective is the name of the game." Among his guidelines:
Pay attention to the programming language. "People often make mistakes similar to typos around pointer misuse, missing breaks in a switch, etc."
Take the time to understand the impact of the code. If it is very localized you should be able to review quickly. If it has larger impact you need to take more time.
If you are having a hard time understanding the code, stop and ask for help. Don't waste a lot of time trying to figure it out.
E. William Horne, systems architect at William Warren Consulting suggests that the code review concentrate on reusability, easy conversion to other locales, use of standard procedures and functions whenever possible, well defined interfaces, system calls and file formats.
Focus on catching items that the compiler, linter and other automated tools miss, says Eric W. Brown, president of Saugus.net. "This may sound basic, but most new code reviewers don't follow it, and time spent on such items is mostly wasted," Brown says. It's far better for code reviewers to look for logic errors, he says; the chief mandate is to find those sorts of glitches where the code doesn't do exactly what it's supposed to (even if it is clean and appears to work in most cases).
Others agree. J Schwan, managing partner of Solstice Consulting, points out that code reviews are not meant to be a replacement for unit testing or functional testing—there are more efficient and automated ways for doing that. Nor should it be a be a formatting exercise. "Some of the worst code reviews I've sat through have involved developers measuring indent spaces or trying to walk through code logic that spans across several modules in their head attempting to find bugs in the logic. Both of these exercises are something that can be performed much quicker by a computer than a human." (For more on tool use in the code review process, see Making Code Review Software Tools Help, Not Hinder.)
Reviewers should hunt down potential problems where working code could break, says Brown. He once performed a code review during which he spotted a bit of C code that happened to work with the current compiler but which might not work with other C compilers. "Fixing it was a simple matter of moving a few lines, but a couple of years later our company did change compilers, and if it hadn't been fixed back at the time of the review it would have led to a really difficult bug," he says.