Running an Effective Code Review
Code reviews can be a frustrating waste of time, but they can be a worthwhile experience that contributes to team-building, improves the software, and oh yeah... is also fun.
Mon, December 22, 2008
CIO — "Oh, don't get me started on code reviews!" says Gary Heusner, client partner at custom software developer Geneca, in what can only be described as a Marvin the Android voice. "For many shops, code reviews are as prevalent as disaster recovery exercises."
To run a successful code review, your first step is to ensure that the code review happens. The code review process typically is among the first items jettisoned from a project, Heusner sighs, "Usually right before someone trims user acceptance testing to less than a week for a four-month e-commerce project." That can occur even in software development departments where the team personally cares about quality. Any mention of "code review" elicits comments like, "Wouldn't it be great to do them?" or "I heard someone did one last project" or "Is it worth the effort and money to hold code reviews since QA will still have to test everything anyway?"
If you got this far, you are already sold on the benefits of code reviews. (If you need extra encouragement, see 5 Reasons For Software Developers to Do Code Reviews (Even If You Think They're a Waste of Time.)
But it's one thing to say you're going to do code reviews. And it's another thing to know how to go about the process right, so that the end result is the best, most joyful application possible. Ideally, you also build a collaborative team environment, create a more responsive development process and, oh yeah, have more fun at work. In this article (and its accompaniments), I share the wisdom gathered from dozens of passionate software developers (oh boy, are they passionate!) about when and how to do code reviews, who should sit at the table and the dire consequences when code reviews are done wrong. And incidentally, they are done wrong a lot.
I've tried to make this a definitive guide to code reviews, which means that I've split the various issues into several articles. That lets you can focus on solving the problem that's getting your shorts twisted into a knot. But I like to think you'll want to read the entire thing; you don't have to do so in any particular order. Here's everything in this package:
Running an Effective Code Review (this article)
How to Lead a Code Review (let's set up and lead the review)
What Kind of Code Review Is This?
Before you go barreling into the conference room armed with a stack of printouts and the phone number for the local pizza delivery joint, make sure that you know exactly why you're getting the team together. Code reviews can have many purposes, and you will have a Very Bad Day if everyone has a different idea of what the purpose of this review is.
You may want to schedule different code reviews for each aspect of the project, such as one that looks at security issues and another that pays particular attention to the application's performance.
"The first step should be to determine why you are reviewing the code," suggests Micheal Lalande, director of technology at QLogitek, a SaaS supply chain solution provider. "This should come from your design-time discussions, where the core non-functional requirements have been made. These can include, but are not limited to, globalization, performance, security and supportability." Re-iterating the purpose at the beginning of the meeting helps the team put its attention on items that deliver the biggest ROI, Lalande says. "For instance, if you are looking at performance, you won't care about the procedure that is called in exception cases, so accepting the results of an automated code review will suffice."
Picking on one thing at a time also ensures that developers dive headlong into a single aspect of the software and don't try to do too much at once. "Too often, a poorly run code review has everyone focus on the same superficial issues," says Theron Welch, software mentor at the Microsoft Asia Center for Hardware, who is helping to build a team in China. "A code review checklist can help encourage a smaller group to focus deeply on a specific area, another group to focus on a different area, and so on. This helps the code review achieve depth."
The review's goal—both this specific review and the process in general—is informed by the business' needs, its institutional bias, the state of the team members and the role of the participants. For example, Jack Danahy, founder and CTO of Ounce Labs, says, "When you're in a bank, you're used to a vault mentality. Within financial services, there are two big concerns: privacy and integrity of data, and the non-reputability of that data." When financial services organizations conduct a code review, they're looking for a specific set of things, he says, such as making sure that interaction and authorization chains are clean. Danahy adds, "In the public sector, the purpose is much more around making sure the application worked the way they expected. There's not as much of a detailed focus but rather looking at the general characteristic, 'Does it work?'" Typical code reviews are about generic policy, such as making sure inputs are validated, as opposed to a more granular policy in which developers have to make sure private data is stored appropriately, he says.
It's important to keep the team's attention on the goal of this code review, and to avoid distraction with other issues. An extremely common mistake, says SOA specialist Mike Kavis, is for reviewers to challenge the design. "The design should have been ironed out in the design review and should not be part of the discussion in the code review," he says. "This is why it is critical that design reviews take place; otherwise the code review process can send the developer back to the drawing board."
Individuals' backgrounds also color the review, points out J. Schwan, managing partner of Solstice Consulting, a Chicago-based technology management consulting firm, depending on whether the developer is junior or senior. For example, Schwan says, in a code review for junior developers, goals should include adherence to the design or architecture defined during the design phase; ensuring that the code is written to perform as efficiently as possible; and use of common code modules. "For senior developers, a peer code review is often more effective," Schwan says. "The goal can then be more focused on ensuring utilization of common code modules and identifying other common code modules that can be reused by other parts of the system."
Next: What's the right time?