Kode Reviews 101

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.