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.
Page 1 of 4 | Next page
