诗词大会视频第一期:Four Ways to a Practical Code Review

Four ways to a Practical Code Review

Jason Cohen, Smart Bear Software, http://www.smartbearsoftware.com/

How to almost get kicked out of a meeting

Two years ago I was not invited to a meeting with the CTO of a billion-dollar software development shop, but I didn't know that untilI walked in the room. I had been asked by the head of Software Process andMetrics to come and talk about a new type of lightweight code review that we hadsome successes with.

But the CTO made it clear that my presence was Not Appreciated.

"You see," he explained, "we already do codeinspections. Michael Fagan invented inspections in 1976 and his company isteaching us how to do it." His face completed the silent conclusion:"And you sir, are no Michael Fagan."

"Currently 1% of our code is inspected," offered the process/metrics advocate. "We believe by the end of the year we can getit up to 7%." Here Mr. Metrics stopped and shot a glance over to Mr. CTO.The latter's face fell. Whatever was coming, they obviously had had this discussion before.

"The problem is we can't inspect more than that. Given the number ofhours it takes to complete a Fagan inspection, we don't have thetime to inspect more than 7% of the new code we write."

My next question was obvious: "What are you going to do about the other 93%?" Their stares were equally obvious ? my role herewas to convince the CTO that we had the answer.

This story has a happy ending, but before we get there I have toexplain what it means to "inspect" code because this is what mostdevelopers, managers, and process engineers think of when they hear"codereview." It's the reason this company couldn't review 93% of their codeandwhy developers hate the idea. And changing this notion of what it meansto "review code" means liberating developers so they can get thebenefits of code review without the heavy-weight process of a formal inspection.

Michael Fagan ? father of a legacy

If you've ever read anything on peer code review you know thatMichael Fagan is credited with the first published, formalized system ofcode review. His technique, developed at IBM in the mid-1970's,demonstrablyremoved defects from any kind of document from design specs to OS/370assemblycode. To this day, any technique resembling his carries his moniker of"code inspection."

Take a deep breath...

I'm going to describe a "code inspection" in brief,but brace yourself. This is heavyweight process at its finest, so bear with me.It will all be over soon, I promise. A code inspection consists of seven phases.In the Planning Phase the author gathers Materials, ensures that they meet thepre-defined Entry Criteria, and determines who will participate in theinspection. There are four participants with four distinct roles: The Author,the Moderator, the Reviewer, and the Reader. Sometimes there is an Observer. Allparticipants need to be invited to the first of several meetings, and thismeeting must be scheduled with the various participants. This first meetingkicks off the Introduction Phase where the Author explains the background,motivation, and goals for the review. All participants get printed copies of theMaterials. (This important ? it's not a Fagan Inspection unless it's printedout.) The participants schedule the next meeting and leave. This starts theReading Phase where each person reads the Materials, but each role reads for adifferent purpose and ? this is very important ? no one identifies defects.When the next meeting convenes this starts the Inspection Phase. The Moderatorsets the pace of this meeting and makes sure everyone is performing their roleand not ruining anything with personal attacks. The Reader presents theMaterials because it was his job to "read for comprehension" sinceoften someone else's misunderstanding indicates a fault in the Materials. Duringthe meeting a Defect Log is kept so the Author will know what needs to be fixed.Before the meeting ends, they complete a rubric that will help with laterprocess improvement. If defects were found the inspection enters the ReworkPhase where the Author fixes the problems, and later there will be aVerification Phase to make sure the fixes were appropriate and didn't open newdefects. Finally the inspection can enter the Completed Phase.

Figure1: Typical workflow for a "formal"inspection. Not shown are the artifacts created by the review: The defect log,meeting notes, and metrics log. Some inspections also have a closingquestionnaire used in the follow-up meeting

The good news is, this works. It uncovers defects, it helpswhen training new hires, and the whole process can be measured for processinsight and improvement. If you have extra money laying around in your budget,Mr. Fagan himself will even come show you how to do it.

The bad news should be obvious in this day of AgileMethodologies. Studies show that the average inspection takes 9 man-hours per200 lines of code, so of course Mr. CTO couldn't do this for every codechange in the company.

Over the years there have been experiments, case studies, andbooks on this subject, almost always using some form of "codeinspection" as the basis. In our survey of published case studies andexperiments in the past 20 years, we found that 95% of them tried inspectionsonly in small pilot groups, and that in no case were they able to applythe technique to all their software development projects.

But surely there is another way. Fagan inspections weredesigned in the days when business logic was written in assembly language and"computer science" wasn't a major and dinosaurs roamed the earth.

Have we learned nothing since then? Don't we need differenttechniques when reading object-oriented code in a 3-tier application? Don't thechallenges of off-shore development require new processes? Hasn't the rise ofAgile Methodologies shown us that we can have process and metrics andmeasurement and improvement and happy developers all at the same time?

So finish the story already!

By now you can guess how the story ends. Using arguments notunlike those above, Mr. Metrics and I convinced Mr. CTO to at least try ourlightweight code review technique in a pilot program with a one developmentgroup that was already hopelessly opposed to Fagan inspections. The metrics thatcame out of that group demonstrated the effectiveness of the lightweight system,and within 18 months Code Collaborator was deployed across the entireorganization.

Assuming you抳e bought into the argument that code reviewis good but heavyweight inspection process is not practical, the next questionis: How do we make reviews practical?

We抣l explore four lightweight techniques:

1. Over-the-shoulder: One developer looks over the author's shoulder as the latter walks through the code.
2. Email pass-around: The author (or SCM system) emails code to reviewers
3. Pair Programming: Two authors develop code together at the same workstation.
4. Tool-assisted: Authors and reviewers use specialized tools designed for peer code review.

This is the most common and informal (and easiest!) of codereview. An "over-the-shoulder" review is just that ? a developerstanding over the author's workstation while the author walks the reviewerthrough a set of code changes.

Figure 1: A typical Over-the-shoulder code walk-throughprocess. Typically, no review artifacts are created.

Typically, the author "drives" the review bysitting at the keyboard and mouse, opening various files, pointing out thechanges and explaining what he did. The author can present the changes usingvarious tools and even go back and forth between changes and other files in theproject. If the reviewer sees something amiss, they can engage in a little"spot pair-programming" as the author writes the fix while thereviewer hovers. Bigger changes where the reviewer doesn't need to be involvedare taken off-line.

With modern desktop-sharing software a so-called"over-the-shoulder" review can be made to work over long distances,although this can complicate the process because you need to schedule thesesharing meetings and communicate over the phone.

The most obvious advantage of over-the-shoulder reviews issimplicity in execution. Anyone can do it, any time, without training. It canalso be deployed whenever you need it most ? an especially complicated changeor an alteration to a "stable" code branch.

Before I list out the pros and cons, I'd like you to considera certain effect that only this type of review exhibits. Because the author iscontrolling the pace of the review, often the reviewer doesn't get a chance todo a good job. The reviewer might not be given enough time to ponder a complexportion of code. The reviewer doesn't get a chance to poke around other sourcefiles to check for side-effects or verify that API's are being used correctly.

The author might explain something that clarifies the code tothe reviewer, but the next developer who reads that code won't have theadvantage of that explanation unless it is encoded as a comment in the code.It's difficult for a reviewer to be objective and aware of these issues whilebeing driven through the code with an expectant developer peering up at him.

So:

• Pro: Easy to implement
• Pro: Fast to complete
• Pro: Might work remotely with desktop-sharing and conference calls
• Con: Reviewer led through code at author's pace
• Con: Usually no verification that defects are really fixed
• Con: Easy to accidentally skip over a changed file
• Con: Impossible to enforce the process
• Con: No metrics or process measurement/improvement

This is the second-most common form of lightweight codereview, and the technique preferred by most open-source projects. Here, wholefiles or changes are packaged up by the author and sent to reviewers via email.Reviewers examine the files, ask questions and discuss with the author and otherdevelopers, and suggest changes.

Figure 3: Typical process for an e-mail pass-around reviewfor code already checked into a version control system. These phases are notthis distinct in reality because there抯 no tangible "review"object.

The hardest part of the email pass-around is in finding andcollecting the files under review. On the author's end, he has to figure out howto gather the files together. For example, if this is a review of changes beingproposed to check into version control, the user has to identify all the filesadded, deleted, and modified, copy them somewhere, then download the previousversions of those files (so reviewers can see what was changed), and organizethe files so the reviewers know which files should be compared with whichothers. On the reviewing end, reviewers have to extract those files from theemail and generate differences between each.

The version control system can assist the process by sendingthe emails out automatically. The automation is helpful, but for many codereview processes you want to require reviews before check-in, not after.Like over-the-shoulder reviews, email pass-arounds are fairly easy to implement.They also work just as well across the hall or across an ocean.

A unique advantage of email-based review is the ease in whichother people can be brought into conversations, whether for expert advice orcomplete deferral. And unlike over-the-shoulder, emails don't break developersout of "the zone" as they are working; reviews can be done wheneverthe reviewer has a chance.

The biggest drawback to email-based reviews is that they canquickly become an unreadable mass of comments, replies, and code snippets,especially when others are invited to talk and with several discussions indifferent parts of the code. It's also hard to manage multiple reviews at thesame time. Imagine a developer in Hyderabad opening Outlook to discover 25emails from different people discussing aspects of three different code changeshe's made over the last few days. It will take a while just to dig though thatbefore any real work can begin.

Another problem is that there's no indication that the review is"done." Emails can fly around for any length of time. The review isdone when everyone stops talking.

So:

• Pro: Fairly easy to implement
• Pro: Works with remote developers
• Pro: SCM system can initiate reviews automatically
• Pro: Easy to involve other people
• Pro: Doesn't interrupt reviewers
• Con: Usually no verification that defects are really fixed
• Con: How do you know when the review is "complete?"
• Con: Impossible to know if reviewers are just deleting those emails
• Con: No metrics or process measurement/improvement

Pair-programming (review)

Most people associate pair-programming with XP and agile developmentin general. Among other things, it's a development process thatincorporates continuous code review. Pair-programming is two developerswritingcode at a single workstation with only one developer typing at a timeand continuous free-form discussion and review.

Studies of pair-programming have shown it to be very effective at both finding bugs and promoting knowledge transfer. And somedevelopers really enjoy doing it. (Or did you forget that making your developershappy is important?)

There's a controversial issue about whether pair-programmingreviews are better, worse, or complementary to more standard reviews. Thereviewing developer is deeply involved in the code, giving great thought to theissues and consequences arising from different implementations. On the one hand,this gives the reviewer lots of inspection time and a deep insight into theproblem at hand, so perhaps this means the review is more effective. On theother hand, this closeness is exactly what you don't want in a reviewer; just asno author can see all typos in his own writing, a reviewer too close to the codecannot step back and critique it from a fresh and unbiased position. Some peoplesuggest using both techniques ? pair-programming for the deep review and afollow-up standard review for fresh eyes. Although this takes a lot of developertime to implement, it would seem that this technique would find the greatestnumber of defects. We've never seen anyone do this in practice.

The single biggest complaint about pair-programming is that it takestoo much time. Rather than having a reviewer spend 15-30 minutesreviewing a change that took one developer a few days to make, inpair-programming you have two developers on the task the entire time.

Of course pair-programming has other benefits, but a full discussion of this is beyond the scope of this article.

So:

• Pro: Shown to be effective at finding bugs and promoting knowledge-transfer
• Pro: Reviewer is "up close" to the code so can provide detailed review
• Pro: Some developers like it
• Con: Some developers don't like it
• Con: Reviewer is "too close" to the code to step back and see problems
• Con: Consumes a lot of up-front time
• Con: Doesn't work with remote developers
• Con: No metrics or process measurement/improvement

Tool-assisted review

This refers to any process where specialized tools are used in allaspects of the review: collecting files, transmitting and displayingfiles, commentary, and defects among all participants, collectingmetrics, andgiving product managers and administrators some control over theworkflow.

"Tool-assisted" can refer to open-source projects, commercial software, or home-grown scripts. Either way, this means money ?you're either paying for the tool or paying your own folks to create andmaintain it. Plus you have to make sure the tool matches your desired workflow, and not the other way around.

Therefore, the tool had better provide many advantages if it is to beworthwhile. Specifically, it needs to fix the major problems of theforegoing types of review with:

Automated File-Gathering: As we discussed in email pass-around, developers shouldn't be wasting their time collecting "filesI've changed" and all the differences. Ideally, the tool should be able to collect changes before they are checked into version control or after.

Combined Display: Differences, Comments, Defects: One ofthe biggest time-sinks with any type of review is in reviewers anddevelopershaving to associate each sub-conversation with a particular file andlinenumber. The tool must be able to display files and before/after filedifferencesin such a manner that conversations are threaded and no one has to spendtime cross-referencing comments, defects, and source code.

Automated Metrics Collection: On one hand, accurate metrics are the only way to understand your process and the only way to measurethe changes that occur when you change the process. On the other hand, nodeveloper wants to review code while holding a stopwatch and wieldingline-counting tools. A tool that automates the collection of key metrics is theonly way to keep developers happy (i.e., no extra work for them) and getmeaningful metrics on your process. A full discussion of review metrics and whatthey mean (and don't mean) will appear in another article, but your tool shouldat least collect these three things: kLOC/hour (inspection rate), defects/hour (defect rate), and defects/kLOC (defect density).

Workflow Enforcement: Almost all other types of review sufferfrom the problem of product managers not knowing whether developers arereviewing all code changes or whether reviewers are verifying thatdefects areindeed fixed and didn't cause new defects. A tool should be able toenforce thisworkflow at least at a reporting level (for passive workflowenforcement) and at best at the version control level (with server-sidetriggers).

Clients and Integration: Some developers like command-line tools. Others need integration with IDE's and version control GUIclients. Administrators like zero-installation web clients and Web ServicesAPI's. It's important that a tool supports many ways to read and write data in the system.

If your tool satisfies this list of requirements, you'll have thebenefits of email pass-around reviews (works with multiple,possibly-remotedevelopers, minimizes interruptions) but without the problems of noworkflowenforcement, no metrics, and wasting time with file/differencepackaging, delivery, and inspection.

It's impossible to give a proper list of pros and cons fortool-assisted reviews because it depends on the tool's features. But ifthe toolsatisfies all the requirements above, it should be able to combat allthe "cons" above.

So what do I do?

All of the techniques above are useful and will result in better code than you would otherwise have.

To pick the right one for you, start with the top of the list andwork your way down. The first few are the simplest, so if you抮e willingto live with the downsides, stop there. Tool-assisted review has themostpotential to remove downside, but you抣l have to commit to a trialperiod,competitive analysis, and possibly some budget allocation.

No matter what you pick, your developers will find that code reviewis a great way to find bugs, mentor new hires, and share information.Just make sure you implement a technique that doesn抰 aggravate them somuch that they revolt.