Peer Code Reviews: Success!
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:
- 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.
- 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.
- A primary reviewer is pre-selected at random but you can change it (*) if you want someone specific.
- 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?
- 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.
- 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).
- Consider abandoning the review and going back to the code for another pass. (***)
- 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?
