What to Look for in a Code Review

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

Running an Effective Code Review

5 Reasons for Software Developers to Do Code Reviews (Even If You Think They’re a Waste of Time

What to Look for in a Code Review

Making Code Review Software Tools Help, Not Hinder

How to Lead a Code Review

How Not to Run a Code Review

Doing Spot-On Code Reviews with Remote Teams

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.

• Avoid second-guessing.

• Don’t take more than 25 words to describe a process.

• Prevent interruptions.

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.

The code review should reflect the concern of the users or stakeholders, and these vary based on developer roles and experience. As longtime developer Chuck Brooks explains, a business owner (such as an independent software vendor) needs “the implementation of a storyboard or other block-level (and graphical) description of the requirement.” The issues to be addressed are whether the implementation is documented; whether the implementation reuses code from the object library; what’s new that didn’t come from a library; how the software is tested, and whether the test(s) be automated for subsequent regression testing when updates and point releases occur.

In contrast, Brooks points out, a contract consultant working on new development should look for code clarity. “For maintenance-centric contract development, the primary concern is understanding the politics before going into the meeting,” he adds. In that case, the main focus is the concerns of the champion, which include version control, testing, rollback procedures and documentation and training.

New developers should be cautioned to focus on the technology, Brooks says. “Coders aren’t supposed to know about business practices, and business analysts get really paranoid.”

Ensure you don’t duplicate functions, says Deakins. For example, the code review process should ask, “Are we doing the same thing multiple times without using a shared function?” Pattern recognition can be challenging sometimes, he cautions, because certain things need to be repeated when you’re looking at 10,000 lines of code. “At Deacom, we’re constantly going back and trying to simplify the code,” says Deakins. “When you’re dealing with a complicated matter like an ERP application, it’s hard to say ‘Wow, it’s done,’ and then go back through the code again and look for the patterns and see how we can make this simpler.” But it’s an important performance issue, as well as ensuring manageable code, he says, because you’ll live with this code for a very long time. “Therefore, the code review process shouldn’t be a big deal. It should be the last step of simplification,” he adds.

Many developers who responded to my queries about how to run effective code reviews put particular attention on the topics that are closest to their own hearts or their own domain expertise. For instance, Paddy Sreenivasan, the co-founder and vice president of engineering for Zmanda, an open-source backup and recovery software provider, tends to focus on security, error handling and user messages in the code. But all experienced developers have a wider view, too; Sreenivasan also checks to ensure any code changes are documented (i.e. code comments) and have unit tests.

This all assumes that the process works. But it doesn’t, always. For details (and to groan in understanding) see How Not to Run a Code Review. And 26 Ways To Know Your Software Development Project Is Doomed, while you’re at it.

Following-Up and Metrics

Don’t forget to have a process for what happens after the code review, or the entire exercise is wasted.

Lear’s Sweet suggests that at the end of the meeting, participants review and rank the items found as “change immediately,” “change by (some date or project milestone)” and “recommendation (change at developer’s discretion).” The leader should follow-up with the developer to make sure that the issues on the list are addressed,” Sweet adds.

Microsoft’s Welch suggests that you can calculate the effectiveness of your code reviews by tracking two pieces of information: The average bug fix rate for a team member (which he says usually is one or two per day), and the number of major issues the code review uncovered. “Assume you found five major issues in your last code review and your team’s major bug fix rate is one per day,” he says. “If there was no code review, it would have cost approximately five days to fix those bugs (the number of bugs times how long it takes to fix a major issue). If your code review had five participants, each prepared for one hour, and the review meeting lasted one hour, the total time is 10 hours. That’s well less than five days, so your code review was worthwhile!”

These aren’t the only things to keep in mind during a code review, of course. You’ll find several other suggestions in the other articles in this Effective Code Review series:

• Running an Effective Code Review

• 5 Reasons for Software Developers to Do Code Reviews (Even If You Think They’re a Waste of Time)

• What to Look for in a Code Review (this article)

• Making Code Review Software Tools Help, Not Hinder

• How to Lead a Code Review

• How Not to Run a Code Review

• Doing Spot-On Code Reviews with Remote Teams