New Fun Blog – Scott Bilas

This is the third in a series of posts on our peer code review process at Loose Cannon. In the first, I talked about what code reviews are and why we do them. The second documented our initial attempts at implementing a code review process.

In this post I’ll cover how our final process turned out. We’ve been doing this for perhaps the last year or so, through many milestones and the final product release, so it has been battle-tested!

Of course, as with all processes, what works depends on the team and culture already in place. It took us a while to settle on this one and I’m sure it would need adjusting at other studios.

Third Attempt: Crucible + Tool + Process

I kept watching Atlassian for updates, and it wasn’t long before they released a version of Crucible that supported patch-based reviews (I think it was 1.1 or 1.2). Yay!

Support for patches wasn’t great; changes coming from patches were not first class citizens like reviews created from a submitted changelist were (note that this is something they have been improving in recent versions). But it was good enough. You could take a patch file of local changes, upload it, and select it in as part of a new review. Awesome.

New Process Requirements

With everything we needed from a collaborative peer review tool in place, a few of the seniors on the team met and designed a new process for our rebooted code reviews. We had the following requirements:

• Every change to game or engine C++ code must be reviewed.
  Tools and game scripts were not included in this process yet. We wanted to get started slowly and narrowing the scope of what got reviewed, and expanding it later on.
• Code reviews must precede checkins.
  We would continue the previous process’s requirement of review-before-checkin, for the reasons stated earlier. I ran periodic queries at first to make sure of this until everyone got in the rhythm of the new process. People got it pretty quickly.
• Reviews must include a “primary reviewer”.
  There would be a core group of three primary reviewers (made up of Matt, Andy, and me to start), and one of us had to be part of every review. We would expand this group over time at Matt’s discretion.

Initial Concerns

We were at first a little worried about the review load among the three of us. I did a query of Perforce to get a feel for how frequent and large the checkins tended to be. We estimated that 20% of our time would go into reviewing code, and had to think hard about whether or not we felt the benefits were worth this cost. We also decided to bring new people into the core group as quickly as possible to help with the load, perhaps within a few weeks.

As it turns out, we didn’t have any trouble handling the load, which was far lower than we had expected. A review or two a day was probably what we each averaged, maybe 5-30 minutes total, depending on complexity. So we ended up keeping the 3-person core group for a while. We pretty much forgot about adding more people until a couple other members of the team asked to be included and we expanded the group.

Another concern was that the process of creating patches and uploading them by hand would be a pain in the ass and error prone. The technical part of the review process had to be really simple or it would be harder to get people on board with the whole thing. Clearly a tool was needed. Luckily, Crucible has a SOAP interface! Well, it had a SOAP interface. They recently switched to REST, so I lost my convenient WSDL and now have to maintain my .NET wrapper of their API by hand (ah well). But whatever.

To solve this, I spent a couple days adding a new crucreate command to my p4x tool to automate creation of reviews from pending changelists in Perforce. This tool has gone through many revisions since, and I will do a full post on what we’ve got now and how it works in a future post.

The New Review Process

Given our requirements, and the new tool, we created the following three-stage process for peer code reviews. All documented in Confluence of course!

Stage 1: Create Review

This part is pretty quick to do, rarely more than a few minutes and usually under 30 seconds:

1. Prep a pending changelist as if you were about to check in: isolate it to the minimum required for the change, give it a good changelist description, build all platforms and configurations, etc.
2. Right-click the changelist in P4Win/V and select “Create Crucible review from changelist…”. This runs p4x crucreate, which puts up a dialog box with available reviewers listed. You can also do this from a command line.
3. A primary reviewer is pre-selected at random but you can change it (*) if you want someone specific.
4. Select secondary reviewers (*), using your judgment. Who is the de facto owner of the code? Who else is affected by this change? Who has domain expertise that would be useful? Who will be upset if they see your checkin and weren’t part of the review?
5. Hit OK! A review is automatically created by p4x and comes up in the web browser in the Draft state, reviewers set up, patch uploaded, and so on.
6. Make any necessary comments on your own review (**). This is also a good time to review the diffs and look for bonehead mistakes (like temp/hack/test code you didn’t intend to check in).
7. Consider abandoning the review and going back to the code for another pass. (***)
8. Hit the Start Review button! All the reviewers will get notified by Crucible via email that they have a job to do…

Some notes:

(*) In choosing reviewers, it’s a good idea to instant-message people first to ask if they’re available to do a review, especially if you’re really itching for feedback or you want to check in ASAP. You never know what schedule other folks are on, mentally or physically! I usually turn around reviews fast when I’m looking for a distraction, but other times I need to focus and will put off reviews until the next day. It’s always good to check.

(**) For example: noting what your intention is in different areas, calling out chunks of code that need special attention, or asking questions like “this part is a mess, is there a better way to do this?” And if you’ve added people to a review for their domain expertise, you can save their time with a comment like “Joe – I added you just for the graphicsmgr changes – did I do this goofy GL state setting stuff right?”

(***) Well this is a little odd, eh? Perhaps it’s just me, but prepping a review always makes me think about the problem from a different perspective, and I often realize I screwed up something deep, or didn’t solve the problem fully, or forgot to test a few things, and so on. So I’ll abandon the review and head back to the code for another pass. It’s a lot less embarrassing to figure this out when a review is in Draft and not started.

Stage 2: Perform Review

This stage is very fluid and could take minutes to days of time to get through, depending on the type of change involved. It has these main tasks:

• Reviewer Task: Make Comments
  Here we go line by line through the diffs and make comments and ask questions. This is where the big win is in doing reviews! Right here, in the middle of this giant post! Unfortunately, what to comment on and how is itself a really big topic, so I have to cover that in a later post.
• General Task: Discuss
  Questions need answers obviously. And comments that the reviewee doesn’t agree with or understand will need some discussion. Most discussions we’ve had are very short, but some can get large and end up needing in-person discussion or possibly escalation to the team lead for a call to resolve.
• Reviewee Task: Address Comments
  ”Address” does not mean “Do What Reviewer Says”. The reviewer is not the boss. “Address” means that the reviewee needs to do something with every comment a reviewer makes. So that means: making the requested change, answering the question, arguing the point, raising new questions and so on. Note that ignoring the comment is not a valid choice.
• Reviewer Task: Mark Complete
  Once a reviewer has made all their comments and has had their questions answered to their satisfaction, they (a) mark it complete in Crucible, and (b) add one final comment. It’s a general comment saying if they’re ok with things going forward. For example, “check in after addressing comments”, “send back with an incremental review”, “this whole change needs to be reverted, I’ll come talk with you” (perhaps by a team lead), and so on.
• Reviewer Task: Talk In Person
  Sometimes it’s just not going to work through Crucible or any tool, and you need to sit down with the reviewee and have them explain how it all works to you. Especially if it’s a huge or weird change that just doesn’t work well in diff form. Afterwards, the reviewer can go back and make informed comments, or maybe the reviewee will be able to bypass the rest of the review entirely if it all came out in the 1-on-1 discussion.

Now, while a change is going through this review process, what is a reviewee to do besides replying to comments and making fixes as they are addressed? They should be working on other things, by using a separate Perforce client, a private branch, or by working on files that do not conflict. Whatever works. They shouldn’t be sitting on their hands waiting for the review to be done so they can check in.

Of course, sometimes you have a high priority fix that needs to go in. Maybe some people are waiting on the edge of their seats for your change. And sometimes people who can’t manage their inbox don’t see or ignore the review notification email. Nagging via instant messaging does a good job of solving this problem.

Stage 3: Close Review

This final stage takes just a few seconds. Finished reviews go to one of several places:

• Submit And Close!
  This happens when everybody is satisfied. It is the result of most of our reviews, depending on the stage of the project and scale, risk, impact, etc. of the change. We’ll go here if all the comments result in trivial changes and the reviewers have said they don’t need to see a new review. So check in the code, take the Perforce changelist number, and paste it into the Crucible review (like “checked in as #37132”) when summarizing it, then close.
• Add Incremental Review
  This happens when reviewers want to review new code changes based on their comments. Crucreate can “diff the diff” and add incremental updates to existing reviews with only what has changed since the comments (more on how this works in a future post). After doing this, notify the reviewers that more info is available and they’ll go back and do another pass on the update. Repeat as necessary. We’ll rarely have more than one or two incrementals tacked onto a review.
• Create New Review
  A totally new review is required when the changes based on comments are really hard to review incrementally. No big deal – just crucreate a new review, then summarize the old review with the ID of the new one, and we start the process fresh. Now, if a change goes through more than a few separate reviews, we probably need to have a whiteboard discussion to sort it all out before the next review is created.
• Abandon!
  Head back to the drawing board. This happens when there is just too much work to do to make things right. Create a new review when the code is ready again. Perhaps 5% of our reviews end up this way.

And that’s the step-by-step of how we do reviews at Loose Cannon!

I’ve gone into a lot of detail that may make it all sound pretty laborious, but that’s the level I like to write at. In practice, our reviews tend to go fast and smooth. It really feels like a natural part of our process now.

Coming Up

Well, it turns out I have a lot more to say about our review process than I thought, so I’ve had to continue to break this down into multiple topics. Maybe I should have a list of what I intend to hit in future posts, and hope it won’t break down further:

• How reviewers make comments. What we comment on and why!
• How this whole process worked out for us. Did we solve the problems we set out to solve? What new ones arose? What does the future hold?
• How the crucreate tool works in detail. With action screenshots!

And with our visas expiring soon, Ally and I are nomads again, so it’s been really hard to find time outside of work to write. But I’m on an 18 hour bus ride right now, so I’m going to see if I can queue up some stuff to post when we get to Máncora. We’ll be there for a week, then it’s on to Quito, Sydney, Brisbane, and back home-home in Seattle on August 9th. I’ve heard we’re missing a really nice summer. To be honest, what I’m really missing is Boar’s Head pickles.

Until next time!

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?

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:

1. Write and test code.
2. Find an available engineer.
3. Sit down side by side and walk through changes, discuss, make small fixes as you go.
4. 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?