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:
- Write and test code.
- Find an available engineer.
- Sit down side by side and walk through changes, discuss, make small fixes as you go.
- Check in if everything is ok, or redo code and go back to step 1.
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.
So I started looking around for tools to help us out. Maybe with the right tool and some changes to our process we could solve this thing…
Enter the Crucible
The first place I looked for tools was Atlassian. I’m a big fan of those folks, having used Jira since 2003, and Confluence since it first came out. Great support and community involvement. I watched them buy Cenqua a couple years ago and knew they had this Crucible tool for assisting code reviews. So I gave it a try.
At first glance it appeared to be almost perfect. I looked around at some competitors and found them either insanely overpriced or simply weak. Crucible’s method of managing reviews and making comments and replies was slick and fast. Its conversation notification system was poor (email-only with limited controls) but I figured we could work around it.
Note: in Crucible v2.0 which was just released, they’ve done some serious work in conversation updates and timelines and so on that looks great. I haven’t tried it yet though, but soon. After we ship!
Unfortunately, Crucible out of the box just could not do what we needed.
Requirement: Pre-Checkin Reviews
In coming up with a replacement for our old process, we obviously wanted to keep the parts that worked. Matt was very firm in requiring that, no matter what we did, it had to remain a pre-checkin process. Code must always be reviewed before committing it to the depot.
Why? Once code is committed, you’ve suddenly got a huge barrier to making meaningful changes to it. There are a few reasons for this:
- The incentive to do a good review is diminished. It’s easier for reviewers to fall back on “Well, if it’s in there, and it works…I guess it’s ok…” and do a poor review.
- When we get near a milestone, the first thing to get dropped will be those reviews. They’ll just pile up in the inbox as “stuff to review after I fix this bug”. Eventually, we’ll be putting off all reviews till after the milestone. A dangerous time to put the blinders on.
- People often start using code immediately after it’s checked in, especially if they’re actively waiting for some change (“can you export that class for me?). So now if a reviewer is requiring significant changes to be made post-commit, particularly when involving architecture, you have to go fix all the code that uses the code you want to change as well. That’s a big barrier.
At the time we were implementing this process (mid-2007), this is where Crucible sadly failed us. It was designed for post-commit reviews only! I went back and took another look at Smart Bear’s tools, which did support pre-commit reviews, but the price was just out of our range. I wonder who their target market is.
Anyway, we put our code review process on ice. I saw on an Atlassian forum somewhere a posting about how they were going to implement patch-based reviews, and figured we’d come back to it then. After all, they were only on 1.0!
So we continued with our unreviewed checkins for a couple more months. It was getting really bad.
Series
My full series on code reviews:
- Part 1: Peer Code Reviews At Loose Cannon
- Part 2: Peer Code Reviews: First Attempts
- Part 3: Peer Code Reviews: Success!
- Part 4: Peer Code Reviews: Good Commenting Practices
- Part 5: About Our Crucible-Perforce Bridge
- Part 6: Peer Code Reviews: How Did We Do?
