Peer Code Reviews: First Attempts
In a previous posting, I talked about what a peer code review is, and why we want to do them. Now let’s start getting into specifics about how we actually do them at Loose Cannon Studios.
As I’m writing this I’m discovering it’s kind of a huge article, so I’m breaking it down into a few pieces.
First Attempt: In-Person Peer Reviews
When I joined the studio, the process was working roughly like this:
Simple, no? It was working well in the beginning, too. There weren’t many engineers, and no crazy deadlines. Life was good. In an ideal world, walking through code is probably the best way to do a code review. Nothing beats that in-person discussion.
Problems We Ran Into
Unfortunately, it slowly lost its effectiveness for us. Around the time that I joined, we started running into some big problems.
Problem: Mini-Cliques
People tended to pick the same person again and again to do reviews, often someone already sitting close by.
It’s just easier with the same person over and over. You already know each others’ styles, probably work in the same area, and so on.
And it’s just so much more of a pain to go get someone from across the room. Even with instant messaging. Does 20 feet really make that much of a difference? In practice, it sure does. This is a big reason I dislike individual offices.
Problem: Lack of Simultaneous Availability
Finding someone available to do a review at the same time you’re ready for it is surprisingly difficult. Especially when a deadline is coming up.
People are always busy, or at least on different mental schedules. Some people want to do their reviews in the morning when they’re waking up, some people do their best coding then and want a review in the afternoon.
And I can’t remember how many times I’d overhear (or participate in) discussions that went like “Can you do a review?” “Sure, oh wait, gimme five minutes” … “Ok I’m ready now” “Sorry, I just noticed some more changes I wanted to make, can we do it in an hour?” “Ok, but I’m going to lunch” and so on.
It seems like it ought to be easy to work this out, but we had a hard time with it. Few people are able to interrupt what they’re working on to do a review, then go back to their work without an expensive context switch. It’s really frustrating on both ends.
Problem: The Blind Leading the Blind
In many cases, we had junior engineers were reviewing other junior engineers’ work. This followed directly from the lack of availability. What else are you supposed to do when nobody more senior is available, anyway?
I saw a lot of bad code going in because of this. It was technically “reviewed”, but it should not have been checked in. Our codebase is still haunted by bad architectural decisions checked in during this time.
Eventually We Just Gave Up
People started checking in code without getting reviews because it was frustrating and there was no perceived value. And because nothing was tracked, nothing could be enforced. So of course people just slowly stopped asking for reviews. I even stopped getting reviews for my own code.
Ultimately, it got to the point where it felt like the review process was being done just for the sake of doing it. This is always a sign of a process that needs to be reexamined and possibly discarded. People start thinking the team leadership is out of touch and we start running into morale problems.
So we decided to do something about it.
Second Attempt: Crucible
Of all the problems with our process, I figured that the main problem was the side-by-side (in-person) review requirement. A lot of the above problems came straight out of that.
If a review can be done offline, you solve the simultaneous availability problem. It’s equally easy to have anybody in the studio do a review. And you can simply require that more senior people must do the reviewing. At least that’s what my thinking was.
Page 1 of 2 | Next page
