New Fun Blog – Scott Bilas

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.