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?

Now that the game is basically done (though weirdly still being kept under wraps by Konami), I plan to write about some of the things we did, both good and bad. I’ve learned a lot from my experience at Loose Cannon so far and need to write about this while it’s still fresh!

I’ll start out with something I think went really well, all things considered: Peer Code Reviews.

Now, we haven’t had our postmortem meetings yet to discuss things like this. And I know that people on the team have issues with the process and our standards, but I feel like overall this has been one of the best things we’ve done at our studio.

Before Loose Cannon, I had never worked anywhere with a review process of any kind. We never even had a coding standard in place at any of those. When I joined, Matt already had reviews going and wanted to add coding standards too. I was skeptical at first, mainly out of ignorance, but Matt convinced me to try it. After a little time optimizing the process and building and writing some tools, I became a believer and evangelist.

The next couple posts are about what I learned, what we implemented, and how it all works.

What Is A Code Review?

A code review is simple: get at least one other person to thoroughly review your changes before you check in. They need to (a) understand the code and (b) make sure anybody else could understand it too. Like, in six months when nobody remembers how it works any more.

Simple in concept. We’ll get to the details later.

Notice that I didn’t say “find bugs” above. Our reviewers usually do look for possible bugs, and they’ll often find some, but this isn’t an important goal for us. Bugs are simple things, really. Architectural problems, API design problems, lack of safety, and so on – those are the real dragons to slay.

What follows are the rough goals of our code review process, in descending order of importance.

Share Knowledge

If the graphics engineer gets hit by an SUV on their way to work (or quits the company, takes a sabbatical, etc.), you don’t want to be scrambling to figure out how all their stuff works.

Code reviews really help with this. In an individual review you won’t grok an entire system. It’s not really necessary to try, either, because over time, after many reviews, you not only get a feel for what’s going on in a system, but also how that particular engineer thinks. This is invaluable when you come back later to work on that system yourself.

I’ve experienced this directly recently as I’ve been bouncing around the project in the final hours, trying to make myself useful, investigating bugs in systems I’ve barely touched but have often reviewed. It’s a weird feeling, feeling things come into focus, wandering through familiar functions…

Anyway, this means that reviewers need to try to understand the code they’re reviewing. Not just skim through it and look for nitpicky easy things. If they don’t understand it, they need to comment with questions. If it’s too big for a simple answer, then they need to stop and find the reviewee, bring them over to a computer, and get them to explain it.

The more eyes on your code, the better.

Catch And Correct System Misuse

Every system tends to have a maintainer, even if you have a “no code owners” policy at the studio. There’s always that one person who works with it more than anybody else and knows all the hidden rules about it. Even if it’s heavily documented, there will always be knowledge that only exists in that persons head, and you definitely want them to be included if you modify “their” systems. That way they are made aware of what’s being changed, and can think about possible repercussions.

They can comment about possible issues, and in the process not only do we get corrections made, but everybody learns something. Not just about what’s being changed, but about rules in the system being modified or used. “Don’t assume that the game object coming back is valid, could be deferred-add” and so on.

In every nontrivial review, the reviewer and reviewee should learn something.

I especially like this aspect of code reviews. In past projects, I’ve subscribed to Perforce review emails for changes to systems I’ve worked with. And as I got the checkin notices I’d run a diff tool and review everything. All after the fact, all optional, all dropped when things got rough. And only in the areas I was paying attention. As a result, I’d miss a lot of things that I couldn’t afford to miss.

In our code review process, I’m often included on things that need my attention, and I have an opportunity to provide pre-checkin feedback to make course corrections on things I think are important. I love this aspect.

Raise The General Quality Of Code

This one is easy. When you know that somebody on the team will be reviewing your code, you’re a little more careful about what you write. You’re a little more deliberate in your choices, a little less hacky, a little more likely to think things through. Yes, we should all be professionals 100% of the time. It’s what we get paid to do. Well, code reviews help to reinforce that.

Peer pressure is a great tool for improving behavior. If you think someone will make fun of you for some embarrassing code that you’ve written, maybe you’ll think twice about doing it. And, well, when it turns out that you don’t think twice, and your ridiculous embarrassing code is caught in review, you’ll have to fix it anyway. Then we make fun of you.

Want to know the pet peeve I’m thinking of right now? Code turds: when someone just comments out some code. Doesn’t explain why it’s commented out, just leaves it in. This has all but disappeared with this new review process. I love you, Mr. Review Process.

Mentoring Junior Engineers

At Loose Cannon, like most studios, we have a wide range of experience in our engineering staff. For me, at past jobs we’ve always thrown the junior folks to the wolves, hoped for the best, and forgotten about them until ship time. And of course, this usually leads to some bad results. Game Developer magazine postmortems are filled with stories about this. (By the way, does anyone read Game Developer any more? I totally forgot about that magazine until just now..)

The problem is that we hire the junior engineers with the best of intentions. They seem sharp but green, we figure we’ll mentor them, and they’ll learn and deliver. A good deal! But what always happens is the seniors get busy, and they just don’t put in the time. The juniors slowly create monstrosities and we manage to ship the game anyway.

With a code review process, more senior members of the team are regularly reviewing junior-level code. There’s just no way around it. They can make course corrections more often, as well as do some mentoring right there. “You should do it like X” “Why?” “Well because…” is such an easy a conversation to have in the context of a review. Outside of a review you have…brown bag lunch events? Regular meetings? Send them to training? Never happens. Code reviews are perfect places to spread knowledge and best practices.

And in some cases, reviews are where we’d identify people that just aren’t getting it, and need some more att
ention (or, possibly, to find another place to work). You’ll find out about it a lot sooner if you’ve got some people paying close attention to their code. You’d think that this would always be the case with junior level people assigned large tasks, but I have yet to see it in any of my jobs before Loose Cannon.

Educate About And Enforce Standards

We have a coding standard at Loose Cannon. Not everyone is happy with it, and most people have a problem with at least some part of it. We’ll be revising it. But warts and all, it is something that has helped. I remember how things were before the standards and it really was a mess.

But I don’t want to discuss coding standards here yet. Future post, perhaps. It’s a controversial topic, no doubt. How far to go with it? Tab/space settings? Curly brace placement? Naming of temporaries? Block headers in cpp/h files? Do we have a standard for Lua scripts too? What about batch files? And so on.

Suffice it to say I’m a big believer in coding standards as tools for increasing readability and comprehensibility. I also believe that even a bad one is better than none at all. It’s important to us that we check for standards compliance in our code reviews. As a small side effect, the consistency makes the code review process a ton easier.

As a simple example, consider a standard that bool-returning functions should “sound” like a boolean operation. So DebugMode() and LoadLevel() are incorrect (they sound like mutable actions, don’t they?), but IsDebugMode() and ShouldLoadLevel() are correct – they are clearly boolean queries. Well, this would get caught in a code review. This ends up helping to standardize API design. Anyone calling an API, looking at the functions involved, can quickly get a rough idea of how the thing works, based on the way the functions are named.

A review is also a good place to ask people to comment or rename things to be more clear. I tend to add a lot of “what’s this member variable for? needs better name” and “this load routine is complicated, needs an overview comment on how it works” type comments to reviews.

Yeah, ideally all code is self-documenting, yadda yadda. But when there’s a particular order of functions you have to call to get Bink to shut down and then the audio system in order to go to the Wii’s home menu, it needs a comment.

“Big Deal, We Do This In Agile”

Agile people are probably thinking “we already do this, what’s the big deal?” Pair programming inherently includes peer review, after all.

Well, we don’t do Agile development at Loose Cannon. Not yet, anyway. I haven’t really looked into it yet, actually. I was waiting for the hype to die down, and that now seems to be the case. Now that teams I respect a lot (like the good folks at Atlassian) are pushing it I’m starting to look into it more.

We don’t do a lot of things that maybe we should. We don’t do unit testing. We don’t have a continuous integration build server. We have information stored in 10 different places and it’s not searchable. So, basically the same as everywhere else I’ve worked.

We have a lot of things to improve. Agile development is just one more thing on the list of things to research and consider integrating into our process.

But overall, I’m very happy that we’ve gotten Peer Code Reviews as an integral part of our daily process.

Up Next: The Details

So I’ve talked about why we do this. Lots of generalities, fuzzy discussion. But how we do it, specifically? Next post.

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?