acm-header
Sign In

Communications of the ACM

Viewpoints

Kode Reviews 101


comical code review

Illustration by Andrew Stellman

Dear KV,

My company recently went through a round of layoffs, and at the same time a lot of people left the company voluntarily. While trying to pick up the pieces, we've discovered that there are just some components of our system that no one understands. Now management is considering hiring back some folks as "consultants" to try to fix this mess. It's not like the code is uncommented or spaghetti; it's just that there are bits of it that no one remaining with the company understands.

It all seems pretty stupid and wasteful, but perhaps I'm just a bit grumpy because I didn't get a nice farewell package and instead have to clean up the mess.

Holding the Bag

Dear Holding,

Maybe you should quit and see if you get hired back as a consultant; I hear they get really good rates! Maybe that's not the right advice here. I meant to say, "Welcome to the latest round of recession," wherein companies that grew bloated in good times try to grow lean in bad times, but realize they can't shed all the useless pounds they thought they could. In my career this is round three of this wheel of fortune, and I am sure it will not be the last.

The best way to make sure that everyone on a team or that enough people in a group or company are able to maintain a significant piece of software is to institute system code reviews and to beat senseless anyone who does not attend. Right now, that advice is a bit like closing the barn door after the train has left the station, or...whatever. It does bring me to a few things I would like to say about how to do a proper code review, which is something I don't think most people ever learn how to do—and certainly most programmers would not learn to do this if it were a choice.

There are three phases to any code review: preparation, the review, and afterward. One of the things most people miss when they call for—or in the case of managers, order—a code review is that it is unproductive just to shove a bunch of people in a room and show them unfamiliar code. When I say unproductive, what I mean is that it is a teeth-grinding, head-banging-on-the-desk, vein-pulsing-in-the-head kind of experience. I've stopped such code reviews after 10 minutes when I realized no one in the room had read a single line of the code before they showed up in the meeting room. Please, to preserve life and sanity, prepare for a code review.

Preparations for a code review include selecting a constrained section of the code to look at, informing everyone which piece of code you have picked, and telling them that if they don't read the code before the appointed meeting time, you will be very displeased. When I say constrained, I do not normally mean a single page of code, nor do I mean a set of 30 files. Balance, as in all things KV talks about, is important to the process. If you're reviewing a particularly nasty bit of code—for example, a job scheduler written by someone who is no longer with the company—then you're going to want to take smaller chunks for each review. The reason for the smaller chunks is that the person who wrote it is not present to explain it to you. A code review without a guide takes about two to three times as long as one conducted with the author of the code.

The next step is to schedule a time to review the code. Do not review code directly after lunch. KV once worked for someone who would schedule meetings at 2 P.M., when lunch was normally from noon to 1 P.M. I never failed to snore in his meetings. Code reviews should be done when people have plenty of brainpower and energy because reading someone else's code normally takes more energy than reading or writing your own: it's extremely difficult to get into someone else's mind-set. Trying to adjust your way of thinking to that of some other programmer takes quite an effort, if only to keep yourself from beating on that other programmer for being so apparently foolish. When I call a code review, it is either early in the day or two to three hours after lunch. Do not perform a code review for more than two hours. There is no such thing as a productive four-hour meeting, except for management types who equate the number of hours they've blathered on with the amount of work they've done.

Now for the review itself. Providing coffee and food is probably a good idea. Food has the effect of making sure people show up, and coffee has the effect of keeping them awake. The person leading the code review, who may or may not be the author of the code, should give a short (no more than 10- to 15-minute) introduction to the code to be reviewed. Make sure to keep the person focused on the code being reviewed. Letting a programmer wax poetic leads to poor poetry and to wishing that, like Ulysses, you had wax in your ears. Once the introduction is complete, you can walk through any header files or places where data structures, base classes, and other elements are defined. With the basics of the code covered, you can move on to the functions or methods and review those next.


One of the most difficult challenges in a code review is to avoid getting distracted by minutiae.


One of the most difficult challenges in a code review is to avoid getting distracted by minutiae. Many people like to point out every spelling and grammatical error, as if they're scoring points on a test. Such people are simply distracting the group from understanding the overall structure of the code and possibly digging for deeper problems. The same is true for language fascists, who feel they need to quote the language standard, chapter and verse, as if an ANSI standard is a holy book. Both of these types of issues should be noted, quickly, and then you should move on. Do not dwell here, for it is here that you will lose your way and be dashed upon the rocks by the Scylla of spelling and Charybdis of syntax.

As in any other engineering endeavor, someone will need to take notes on what problems or issues were found in the review. It is infuriating in the extreme to review the same piece of code twice in six months and to find the same issues, all because everyone was too lazy to write down the issues. A whiteboard or flip chart is fine for this, and in a pinch you might be able to trust the author of the code to do this for you. I would follow the latter path only if I trusted the author of the code, because programmers are generally lazy and will subconsciously avoid work. It's not even that they'll know they left off something to fix, but three months later you'll say to them, "Wait, we told to you to fix this. Why isn't this fixed?!" To which you will receive curious looks accompanied by "What? This? Oh, you meant this?" If it's an issue, write it down while the group is thinking about it, and go over the list at the end of the review.


A compiler reads your code, but it doesn't understand its purpose or design; for the moment, only a person can do that.


When the review is over, there is still work to be done. Of course, someone has to fix all the spelling, grammatical, and language conformance issues, as well as the genuine bugs, but that's not all. Copies of the notes should be distributed to all participants, just in case something happens to the author of the code, and the marked-up copies of the code should be kept somewhere for future reference. There are now some tools that will handle this electronically. Google has a code-review tool called Rietveld for code kept in the subversion source-code control system. Although an electronic system for recording and acting on code-review issues is an excellent tool, it is not a substitute for formal code reviews where you discuss the design, as well as the implementation, of a piece of code. A compiler reads your code, but it doesn't understand its purpose or design; for the moment, only a person can do that.

KV

q stamp of ACM QueueRelated articles
on queue.acm.org

A conversation with Steve Bourne, Eric Allman, and Bryan Cantrill
http://queue.acm.org/detail.cfm?id=1454460

Kode Vicious: The Return
http://queue.acm.org/detail.cfm?id=1039521

The Yin and Yang of Software Development
http://queue.acm.org/detail.cfm?id=1388787

Back to Top

Author

George V. Neville-Neil (kv@acm.org) is the proprietor of Neville-Neil Consulting and a member of the ACM Queue editorial board. He works on networking and operating systems code for fun and profit, teaches courses on various programming-related subjects, and encourages your comments, quips, and code snips pertaining to his Communications column.

Back to Top

Footnotes

DOI: http://doi.acm.org/10.1145/1562764.1562778


Copyright held by author.

The Digital Library is published by the Association for Computing Machinery. Copyright © 2009 ACM, Inc.


Comments


Olivier Scalbert

Hello,

I have read your interesting article on code review.
Here are some points (based on real life) I would like to add ...

Scope and timings
Doing a code review for a nuclear plant or a medical device context is not the same thing as doing a code review for Flash animation actionscript code !
So scope and timing should be very well defined and understood by everyone.

Coding rules and style checking tools
These tools can do a lot of work for you, automatically.
They can be seen as a prerequisite before a formal code review.
After code review, new rules can be added to such tools.

Unit tests
It is much more comfortable to do a code review for a code fully covered by unit tests.
If you need to understand a very complex piece of code it is perhaps easier to try to play with this code by writing some unit tests.
If you can not write unit tests for this code, I have serious doubts you can do a code review on it !
Code coverage tool can be your friend there.
Also unit tests can be seen a assets. They can be run again later automatically (which is not the case of a code review).
They can help to clarify missing specs.
Having unit tests prove at least that the code under review can be compiled ! Which is far from obvious with only code review.

Missing specs
Sometimes (read often!) specs are missing or not enough formal. Having a domain expert can help.
Specs should then be fixed.

Fix after code review
Treat the code review results as a problem report, so it can be prioritized, assigned, fixed, validated and closed.
The fix should be done in separate branch to help code reviewers.

Human factors
Code review is not a trial. Code author is not guilty. Code reviews without a little of psychology could lead to disasters ... Egos ...
There are several way to avoid this:
- start code review process early in the project (cultural aspect and avoid black box or even black hole);
- do a little of peer programming which can be seen as a small real-time code review.

Best regards,

Olivier Scalbert


Displaying 1 comment

Sign In for Full Access
» Forgot Password? » Create an ACM Web Account
Article Contents: