multipart-mixed

Code Reviews

At some point your code will be the subject of a peer review. Many programmers loathe, detest, and double-plus unlike code reviews, but there’s really no reason to. In fact, experienced programmers look forward to code reviews—we’ll see why shortly.

Person and Perspective

The reason so many code reviews go bad is because the programmer makes a connection between the his code’s worth and his self worth. When reviewers point out problems in the code, the programmer takes insult, gets defensive, and things go downhill quickly.

Let’s be very clear about this: reviewers will find fault in your code. Gonna happen. I guarantee it. That does not mean you suck. There’s always room for improvement, or at least different perspectives, on how your code should be written. Treat the code review as an open discussion, not a trial where you’re the defendant.

“Faults” can range from bugs to issues of style. The bugs are easy, you’ve got to fix those. Everyone, novice and expert alike, screws up now and then, so nobody in the room thinks you’re an idiot. Just take a note and move on.

The more contentious problems arise with issues of style. Maybe you’re using a loop with a counter and a senior programmer reviewing your work suggests using an iterator instead. Is your code wrong and the suggestion right? No, matters of style are not so absolute.

Since this is an open discussion, go ahead and discuss the merits of the suggestion. Perhaps the reviewer will say, “using an iterator eliminates the possibility of an off-by-one problem.” Do not get defensive and argue “but my loop doesn’t have an off-by-one problem!” The reviewer knows that already. The point he’s trying to make is, it’s good style to eliminate that possibility with a different approach.

Once you understand the point the reviewer is making, thank him for the suggestion, take a note, and move on. Consider your course of action after you’ve had time to let the tension of the code review dissipate. Disagreements on style become contentious because they become personal; if you consider the merits of the disagreement without the other person right there, you may discover the reviewer had a valid point.

Perspective is essential: it’s not about you being right and the reviewer being wrong. It’s about good code and better code.

Formats

I’ll describe the formats I’ve seen for code reviews and give you some tips for each.

The Ad-Hoc Review

Often you’re puzzled by something and you just need someone to help you through it. Or perhaps you’ve found what you think is a good solution but you’re not sure. Go grab a more experienced programmer. Even the grumpy ones usually put their grumpiness on hold; the flattery of being asked for their opinion softens even the most surly.

Buddy System

Some projects will require a “buddy” to sign off on any code that’s checked into the source repository. You’ve made a change, tested it, and now you need someone to review it before check-in. Do not just go find your favorite pal that will green-light anything you write. Go find someone that’s an expert in the area you’re changing. Failing that, find someone that hasn’t buddied for you in a while.

Use the buddy system as a way to get more people familiar with your work. Especially when you’re “the new guy,” there’s no better way to build your credibility than with code. It doesn’t have to be brilliant, wicked fancy code—just solid code. Make sure people see it on a regular basis.

The High-Level Review

This is often a sit-down meeting with multiple people and a projector. You’ve often reviewing weeks worth of work, but at a high level. You explain the design, how it translates to code, and then review key portions of code. This is an opportunity for discussions on design and style. Be prepared for criticism and keep in mind the issues I discussed about people and perspective.

My favorite question, as a reviewer, is “let me see the automated tests.” I require automated tests for any project I lead, so if the response to this question is a blank stare, the review is over and we’ll schedule a new one for the next week. However, the experienced programmer will start the review by going over the tests. Nothing instills confidence in your code better than showing the tests.

The Line-By-Line Review

The most tedious, soul-crushing code review is where everyone walks through the code line by line. In practice, this kind of review tends to be held for code that’s already a disaster. (Better not be your code.) The worst problem with the line-by-line is that it assumes the design of the code is not up for review. You’re just trying to find bugs. Given that, ask of each line of code: what are the assumptions of this line? What ways could it fail? What happens in the failure case?

How do you avoid the line-by-line review of your code? Easy: get your code reviewed early and often. Ad-hoc, buddy system, or schedule your own group reviews. I started out saying that experienced programmers look forward to code reviews. That’s because they prefer to get feedback early and avoid getting into the mess that requires the line-by-line review.

The Audit

I’ve heard about this practice from others but not used it myself. In an audit, a senior programmer takes your entire project and drills down on specific topics of his choosing. And when I say drill down, I mean way down. Why did you choose such-and-such design? What data did you have to prove your assumptions? How do you prove (or test) that your implementation is correct? How much wood could it chuck per second if it could chuck wood? You get the idea.

Preparing for an audit is a big deal because you don’t know what the auditor is going to ask. You’ve got to be prepared for anything. My only advise here is to ask yourself the same kinds of questions as you program. If your program is reading data from a file, ask yourself, what assumptions am I making of that file’s format? How am I testing those assumptions? How big can the file be? Can I prove that?

Of course, you can only follow the rabbit hole down so far. At some point there’s a diminishing return on this line of thinking; that point will vary depending on the type of project and the phase of its lifecycle. Err on the side of caution with production code. Err on the side of git-er-done with trade show demos or proofs of concept.

Comments

When it comes to code reviews, it's important to major in the majors and minor in the minors. If you're invited to review the code of a peer, don't get too caught up on minor stylistic issues. Focus rather on big picture design decisions. Minor stylistic issues include things like what variable naming conventions and casing conventions are used, do you put your starting braces on a line of their own or at the end of a line, and -- my personal favorite -- are some lines of code longer than, say, 80 characters. Don't get me wrong, coding style guidelines make a difference in readability and should be consistent across a project. It's just that when someone reviews my code and all they do is nitpick on stylistic details, I usually decide that they didn't properly prepare for the review, so they're wasting my time on obvious nitpicks instead of going after the important design decisions that I want help thinking through.

Post a comment