Required code reviews

This is the story of the growing Khan Academy team converting me into a passionate fan of requiring a code review for every single changeset.

Those who have worked with me know that it’s a surprising position for me to take. On the spectrum of “Follows good development practices even if it slows down the product” to “Just ship the thing, code doesn’t matter, only users matter,” I tend to fall…right about…[furiously scribbling]…here:

Even though I’ve long been a fan of code reviews at both Fog Creek and Khan, I never would’ve suggested requiring them for every single changeset, no matter how small. At first glance it appears all P̝̂R̫͙̼̽ͪ̽̋Ö̝̹̿ͬ́̐̆̈CÈ̝̱S̮̜͙̩̠S̹͍̳̖͍̆̐Y̩̟̥̟̘̺̠ͭͫ̔, this is web development, not rocket science, and Hey what if there’s an emergency and Wait are you serious, you want me to review my single-line change to a trivial #comment???

Luckily for everyone, we’ve been hiring smarter and smarter people at Khan who can save me from myself. (What’s that theory about smart people and some lake? Lake Okeechobee, I think. With the crocodiles.) We told our team that we’re requiring code reviews for all pushes a couple weeks ago. Spoiler alert: it’s not that processy, and even a Just Ship It clown like me is already seeing immense value from the experiment.

Croc! No, sorry, wait…log!…Or, no! Wait! Sorry…Croc!

What’s been so great about requiring reviews?

  1. For a team that values code reviews so highly, we were missing a lot of them. Turns out there are quite a few excuses you’ll tell yourself to explain why this changeset doesn’t need a review. By explicitly setting the expectation, everything’s much simpler. Just get it reviewed.

  2. Frequent, digestible reviews. By committing frequently and reviewing everything, we don’t let features build up to huge, threatening diffs that make your nose bleed like the emo kids from Chronicle. Any change that makes reviews more enjoyable will amplify all of the other well-understood benefits of code review that we’ve listed in our public Khan onboarding docs so I don’t have to make this sentence any longer by talking about them. Small reviews are just more fun.

  3. Those quick one-line reviews aren’t actually a problem. They take 30 seconds to review, see #1. If you have a legitimate emergency, push it and get it reviewed later, ain’t no thang. If I believed for a second that this decision would hurt our ability to Get Things Done, I’d be fighting it tooth and nail.

  4. In two weeks of doing this, I feel I’ve improved as a developer more than any other period in recent memory. All props to my reviewers, of course, and it probably doesn’t hurt that I’m actually writing code for the first time in a while. But…still.

All changesets reviewed.

It’s not for every group. I’m not convinced it would’ve been right when Jason and I were hacking together alone. But requiring code reviews has already made for a better product and a healthier team, two things that I personally care about far more than a healthy codebase. That’s just a nice side-effect.

P.S. When working on Kiln, I was adamantly opposed to building code review requirements directly into the source control product. I stand by that belief, and I’m almost certain the Kiln team still agrees. Reviews should be required by your team’s dynamics and strongly encouraged by your tooling, not the other way around.

2/28/12 — 9:07am Permalink
  1. psteffek reblogged this from bjk5
  2. tingletech reblogged this from bjk5 and added:
  3. syntatic reblogged this from bjk5
  4. bjk5 posted this