Design and code reviews in the age of the Internet

March 2008

Code reviews are one of the standard practices of software engineering. Or let's say that they are a standard practice of the software engineering literature. They are widely recommended; how widely practiced, I am not sure.

The original notion was "code inspection" as introduced by Michael Fagan from IBM in a 1976 article . Inspection or review, the general idea is to examine some element of code in a meeting of (typically) around eight people, with the aim of finding flaws or elements that should be improved. This is the only goal; assessing the programmer is in principle not part of the game, although this may be difficult to achieve, humans being humans, unless the manager is explicitly barred from the meeting - a decision that may not be justified. It is also not part of the review's purpose to correct deficiencies, only to uncover them. The code and any associated elements are circulated a few days in advance. The meeting typically lasts a few hours; it includes the author, a number of other developer competent to assess the code, a meeting chair who moderates the discussion (and should not be the manager), and a secretary who records it, producing a report with specific recommendations. Some time later, the author should respond to the report by describing whether and how the recommendations have been carried out.

Such is the basic idea of traditional reviews. It is often criticized on various grounds. Advocates of Extreme Programming point out that reviews may be superfluous if the project is already practicing pair programming. Others note that when it comes to finding serious flaws - a looming buffer overflow, for example - static analysis tools are more effective than human inspection. In any case, the process is highly time-consuming; most teams that apply it perform reviews not on the entire code but on samples. Still, code reviews remain one of the tools in a battery of accepted "best practices" for improving software quality.

All the descriptions I have seen in the literature talk of a review as a meeting; a physical one, with people sitting in the same room. This is hardly applicable to the model of software development that is increasingly dominant today: distributed teams, split over many locations and time zones. At Eiffel Software we were curious to see whether we could apply the model in such a setup; our first experience - still quite fresh, and subject to refinement - suggest that thanks to the Internet and modern communication mechanisms this setup is less a hindrance than a benefit, and that today's technology actually provides a strong incentive to revive and expand the idea of the review.

The EiffelStudio development team has members in California, Western Europe, Russia and China. In spite of this we manage to have a weekly technical meeting, with some members of the team agreeing to stay up late. (In the winter 8 to 9 AM California means 5 to 6 PM in Western Europe, 7 to 8 PM in Moscow, and midnight to 1 AM in Shanghai.) We are now devoting one out of three such meetings to a code review. Here is some of what we have learned and practiced.

First, scripta manent: the written word wins. We have found that a review works much better if it is organized around a shared document. Our current technology is Google Docs, which provides a primitive Microsoft-Word-like editing framework, but on the Web, so that several people can update a given document at the same time; conflicts happen only rarely - when you and I are changing exactly the same words.

Our meetings, whether ordinary ones or for code reviews, last one hour. We are strict on the time limit, obviously because it's very late for some of the participants, but also because it makes no sense to waste the time of a whole group of developers. This schedule constraint is an example of limitation that has turned out to be an advantage, forcing us to organize the reviews and other meetings seriously and professionally. The meetings use simple collaboration tools such as Skype conference calls (which we have discarded for voice because we found it to be not reliable enough at this stage, but of which we retain the chat window), X-Lite for voice, and Webex for sharing the host's desktop and driving the meeting.

The unit of review is a class, or sometimes a small number of closely related classes. A week in advance of the code review the code author provides a review page - currently a Google Docs document - with links to the actual code. It follows a standard structure made of 8 sections, dividing the set of aspects to be examined:

1. Choice of abstractions
2. Other aspects of API design
3. Other aspects of architecture, e.g. choice of client links and inheritance hierarchies
4. Implementation, in particular choice of data structures and algorithms
5. Programming style
6. Comments and documentation (including indexing/note clauses)
7. Global comments
8. Coding practices

This goes from more high-level to more implementation-oriented. From the presence of sections 1 to 3 and their importance it is clear that we are talking not just of a code review but really of a design and code review. Practitioners of Eiffel will know that with a full-fledged object-oriented approach design and code are closely intertwined.

One of the differences with a traditional review is a practice that we hadn't planned but quickly imposed itself through experience: most of the work occurs off-line, before the meeting. In fact the original impulse was to limit the amount of advance work and delay written comments to a couple of days before the meeting, to avoid reviewers influencing each other too much. This turned out to be a mistake; far from being harmful to the quality of the process, interaction between reviewers has turned out to be one its most effective parts.

The reviewers provide their comments on the review page; the code author can then respond. The benefit of this approach is that it saves considerable time. Before we systematized it we were spending time, in the actual meeting, on non-controversial issues; in fact, our experience suggests that with a competent group most of the comments and criticism will be readily accepted by the code's author. We should instead spend the meeting on the remaining points of disagreement. Otherwise we end up replaying the typical company board meeting as described in the opening chapter of C. Northcote Parkinson's Parkinson's Law. (There are two items on the agenda: the color of bicycles for the mail messengers; and whether to build a nuclear plant. Everyone has an opinion on colors, so the first item takes 58 minutes ending with the decision to form a committee; the next decision is taken in two minutes, with a resolution to let the CEO handle the matter - does that sound familiar?) Instead, the verbal exchanges can target the issues that truly warrant discussion.

For an actual example of a document produced before and during one of our code reviews, see http://dev.eiffel.com/reviews/2008-02-sample.html which gives a good idea of the process.

This approach has several advantages:

• The group saves time. Precious personal interaction time is reserved for topics that require it.
• Discussing issues in writing makes it possible to have more thoughtful comments. Participants can take care to express their observations - criticism of design of code decisions, and the corresponding responses - properly. This is easier than in a verbal conversation.
• In a group with contentious personalities, one may expect that expressing comments in writing will help defuse tension. (I am writing "may expect"because I don't know from experience - our group is not contentious.)
• The written support allows editing and revision.
• There is a record. Indeed the review no longer needs a secretary or the tedious process of writing minutes: the review page in its final stage, after joint editing, is the minutes.
• The verbal discussion time is much more interesting since it addresses issues of real substance. The dirty secret of traditional code reviews is that most of the proceedings are boring to most of the participants, each of whom is typically interested in only a subset of the items discussed. With an electronic meeting each group member can take care of issues of specific concern in advance and in writing; the verbal discussion is then devoted to the controversial and hence interesting stuff.

Through all our meetings - not just code reviews - we have discovered another consequence, another example of how constraints can become benefits. Individually, most of us apart from piano players may be most effective when doing one thing at a time, but collectively a group of humans is pretty good at multiplexing. When was the last time you spent a one-hour meeting entirely and willingly focused at every minute on the issue under discussion? Even the most attentive among us, careful not to let their minds wander off topic, react at different speeds from others: you may be thinking deeper about the previous topic even when the agenda has moved on; you may be ahead of the game; or you may have something to say that complements the comments of the current speaker, whom you don't want to interrupt. But this requires multithreading and a traditional meeting is sequential. In our code reviews and other meetings we have quickly and naturally learned to practice a kind of organic multithreading: someone is talking; someone is writing a comment in the chat window (e.g. a program extract that illustrates a point under discussion, or a qualification of what is being said); a couple of people are updating the common document; someone else is preparing next week's document, or a Wiki page at http://dev.eiffel.com. It is amazing to see the dynamics of such meetings, with the threads progressing in parallel while everyone remains on target, and certainly not bored.

Our experience may not be fully transposable to an arbitrary context. Our group is small, we know each other well and have been working together for quite a while; we developed these techniques together, learning from our mistakes and benefiting from the advances of technology in the past years. But whatever the reservations I believe this is the way of the future. Even more so considering that the supporting technology is still in its infancy. Two years ago most of the communication tools we use did not exist; five years ago none did. (Funny to think of all the talks I heard over the years about "Computer-Supported Cooperative Work": fascinating, but remote from anything we could use. Suddenly come the Web, Voice Over IP solutions for the common folk, shared editing tools and a few other commercial advances, and the gates open without any fanfare.) The tools are still fragile; we still waste too much time on meta-communication ("Can you hear me? Did Peter just disconnect? Bill, remember to mute your microphone!"), calls get cut off, we don't have a really good equivalent of the shared whiteboard - but all this will be corrected.

Team distribution is a fact of life in today's software development, and as well as a challenge it can be a great opportunity to improve the engineering of software. Let me here put a plug for something that is happening on the academic side of my work. We may be the first at ETH Zurich to have offered a course specifically devoted to studying, in the controlled environment of an academic environment, the issues and techniques of distributed development. Our DOSE course (Distributed and Outsourced Software Engineering) involved in 2007, for the first time, a cooperative project performed in common by several universities. This was a trial run, and we are now expanding the experience; for details see http://se.ethz.ch/teaching/2008-H/dose/.

Participation is open to any interested university. If you are teaching I hope you will consider joining the next iteration of the course, which should be particularly exciting.

But this is not an academic issue. Eiffel Software's still recent experience of collaborative development - every meeting brings new insights - suggests that something fundamental has changed, mostly for the better, in the software development process. And as regards to code reviews I (for one) do not expect ever again to get stuck for three hours in a windowless room with half a dozen other programmers poring over some boring printouts.

Acknowledgments: I am grateful to the EiffelStudio development team for their creativity and team spirit, which enabled the team collectively to uncover and apply the techniques described here.

-- Bertrand Meyer