Code Reviews as Relationship Builders - A Few Tips

I have code reviews on the brain as we build the Khan Academy team and prepare for our first class of summer interns. Code reviews are an important everyday tool for most tech teams, and I think they’re pretty much indispensable when absorbing the talents of a new developer.

Unfortunately, code reviews can also go very wrong. Taking a close, editorial look at the product of a developer’s focused thoughts is kind of an intimate affair. And unlike their liberal arts counterparts, computer science students are being left woefully unprepared for this experience. If you’re not careful, you run the risk of misusing one of the most powerful relationship-building practices available to a development team.

Below are some of the lessons we’re building into Khan Academy’s code review culture, with special focus on new developers:

#1: Point out the good stuff

I don’t care how obvious this sounds, it needs to be said. It’s really, really common to think that the sole purpose of a code review is to point out the problems in somebody else’s code. If you’re trying to build a solid group of devs that get along and learn from one another, this can be literally (in the “not actually literally, just really emphasizing the point” kinda way) poisonous.

Going under code review often means submitting your brain’s best solution for a complex problem to feedback that can range from, “AHEM you forgot to capitalize this variable,” to, “Your priorities are off, let’s throw this code out*.” If your team isn’t taking advantage of the chance to also acknowledge good work and the little sparks of genius that pop up here and there, you’re doing it wrong.

This gets to the core of most code review tools, which use language like ‘Issue’, ‘Bug’, and ‘Change’ to describe the actions you can take when discussing a piece of code. I’ve heard a few of Fog Creek’s discussions about this, and I don’t think it’s crazy to guess that Kiln may someday lead tools in the right direction.

#2: New developers should review experienced devs’ new code

A new team member should be reviewing code, not just having their code reviewed. You hired this developer because you believe they’re incredibly smart, so you might as well shut up and start learning.

More importantly, this balances the relationship while giving new devs a chance to learn unseen parts of the codebase in a nontrivial way.

#3: Review in a timely manner

Waiting a month to look at somebody’s code and then lobbing a bomb of critiques their way is a great way to really piss someone off. Besides being kinda stupid from a technical standpoint since the code has probably been accumulating users for the past month, this guarantees that the dev will have forgotten a lot of their work, and you’ll catch them on their heels.

It takes team-wide discipline to review in a timely manner, but it’s crucial for new devs.

#4: Encourage discussion during review — you’re not swooping in with the hammer of truth

Again, obvious, but needs saying. Blindly accepting all of a reviewer’s suggested changes isn’t healthy.

#5: Review in person every once in a while

I hesitated to add this one, because I don’t think it’s necessary if all of the above are handled very well. Still, it’s much easier to make sure you’re building a healthy code review relationship if you’re sitting next to the person and watching them react to your comments.

*#6: Devalue code on your team, people are worth way more

Freshly-written code can be thrown out; it happens, it should happen. Sometimes it’s tough, particularly on new team members, for obvious ego reasons. Having the wherewithal to decide that a working piece of code isn’t solving the right problem is one mark of a mature team.

So toss the code out if you have to. It’s ok. Maybe you’ll start using low-fidelity mockups more before writing new code. Building this “we’ll toss the (new) code out, we don’t care” attitude sets the right precedent for your team: your developers and their relationships matter most. Code reviews exist to help your team refine its problem-solving abilities, not to produce the perfect block of code.

3/20/11 — 7:24pm Permalink
 
  1. bjk5 posted this