New Fun Blog – Scott Bilas

This is the fourth in a series of posts on our peer code review process at Loose Cannon.

Somewhere in the middle of the third post, I started to talk about the “make comments” part of the process, but it’s a big subject, deserving its own entire post. So here we go.

Comments in a review are where the real goods are delivered. This is where we get all the benefits that I had talked about way back in the first post.

What We Don’t Expect From Reviewers

First let’s talk about what reviewers aren’t expected to do when they’re reviewing a change.

Reviewers aren’t expected to catch everything.

It’s impractical and arguably a waste of time. Knowledge-spreading, mentoring, and so on are more of a “seeping” process than a hard core lesson plan. The idea is that, eventually, with enough reviews and shuffling of reviewers, the knowledge will spread throughout the entire team. There’s just no need to focus on catching every single thing in every single review.

Reviewers aren’t expected to catch deep or systemic design problems.

A changelist is a snapshot of a small part of the game. It’s really hard to try to see the big picture through a pinhole. Reviewers will often open up their editor and browse around in code outside of the change during a review, to get more context. FishEye’s browsers and search (and blame!), and Perforce’s time lapse view help here a lot too. But this only goes so far.

At some point, you’ve got to throw up your hands and say “we’ve got to talk, I can’t see what’s going on here”, head to the whiteboard, and discuss it in person.

The reviewer is not (necessarily) the boss.

Reviewers do not necessarily have the authority to enforce changes, nor should they have extra responsibility for the quality of code they didn’t write. The reviewee maintains responsibility for their own changes.

We are tapping into reviewers’ brains and schedules to help make the entire project better. This is a service they provide, not an opportunity for dictatorship. While it is a requirement of our process that all reviewers’ comments must be resolved, this does not necessarily mean “do what the reviewer says”. Ultimate responsibility and authority remains with the reviewee’s lead.

Now, it’s pretty easy to end up in dictator-speak mode when you’re in the zone, ripping through reviews, making comments. It helps to soften more subjective comments with phrases like “I suggest”, “are you sure this is the best way?”, and “this is totally optional and my opinion, but…”.

What Reviewers Seek

Ok, now on to what reviewers are actually commenting on. Reviewers are looking for the following kinds of things, in no particular order of priority.

Are There Architectural and Domain-Specific Issues?

Every project has experts in different problem domains. You want these people reviewing changes in areas in which they have expertise. Graphics, scripting, debugging, architecture, assembly, tuning, you name it.

Here are some examples I picked out at random from our reviews.

All we’re doing here is looking for “gotcha’s”: hidden rules in systems that you know well are especially important. Or perhaps better, more efficient ways to do things. Or how new code should fit into old code and interoperate with other systems.

This isn’t just for immediate course correction. With each review comment in a specific problem domain, the reviewee learns how to do things better in the future, so we don’t hit this again. And, often, the reviewee will run through their other code where the same mistake was made (but not caught yet) and fix it.

This is a wonderful way to spread knowledge while improving the code in the immediate changelist.

Are They Following Best Practices?

Here we are drawing heavily on the unique career experience of the reviewers, who are looking for things like:

Good comments, well-placed, relevant, etc. Good naming of variables – descriptive, not named after the type… Good flow in a function Avoiding code duplication Hoisting common code out to utilities/systems Calling out when someone is being lazy (in a bad way) Making unnecessary changes that are just personal taste General readability concerns

A lot of this can really subjective and often results in spirited debate. But this is a good thing – everybody learns something! And often we’ll agree to disagree and move on. However, if the same issue comes up again and again, then we can add the lead to the review and ask them to make a call to resolve it.

To the right is a sample clipping with a best practices discussion that will lead into an offline meeting.

Are There Any Opportunities to Mentor?

Teams are often made up of people with a wide range of experience levels. Review comments can be a great place to mentor a more junior engineer. If they’re sharp and fearless, they’ll challenge you on comments they don’t agree with or understand. Instead of getting mad and replying with a “just do it, this is the best way” – take advantage of the opportunity!

If you instead take the time to really give them a good explanation of why you made the comment, a couple things may happen. First, they will learn something. Great. But another possibility (this happens to me a lot) is that by forcing yourself to explain why it must be done that way, you find out that you actually don’t have a good reason. Maybe your reasoning is based on religion, or outdated techniques, or wasn’t completely thought-out. I like when this happens because it makes me a better engineer. Make sure the team knows that as a reviewer you expect to be challenged.

Comments in Crucible (our code review tool of choice) have permalinks as well, so it’s easy enough to link to the discussion from the team’s wiki for spreading the word.

This has been one of the more successful parts of our code review process. People will ask questions and say things in text form that would never happen in person. It just doesn’t come up as often in casual conversation to talk about a lot of seemingly minor things like why a particular naming convention exists. In text, when you’re directly reviewing code, it’s a natural part of the process to say “why does it need to be this way?” and can easily be done in a non-confrontational manner.

Are They Adhering to Our Coding Standards?

Not long after we started the new code review process at Loose Cannon, we sat down to hammer out an initial set of coding standards. We were planning on having coding standards anyway, but it became clear right away when we started reviews again that we needed standards immediately. It’s very hard to review code where every person has their own weird style and habits.

Comments about coding standards are simple and easy, and should be short. We have a Confluence doc with our standards, so as you notice things that don’t adhere, flag them. This is a good way to teach new engineers our existing standards, as well as updating everyone on the occasional new standard.

On our current game, we backed off from most of this when we were very close to shipping. At that point you’re writing a lot of junk (particularly certification compliance) just to get the game done that you don’t intend to carry forward to future games. More nitpicky stuff like coding standards is just not a priority in the final weeks.

Did They Write A Good Changelist Description?

This is a relatively new thing we added to reviews. It is really a best practice, but I wanted to call it out as a special item because good changelist descriptions are so important when debugging problems with unfamiliar code. And so many people do it wrong. [I need to write up a post on how to write good changelist notes. People always make the mistake of commenting on the ‘what’ they changed, and missing the ‘why’. Any fool can do a diff and find out what changed, but six months later remembering why it was changed? That needs good changelist notes.]

When our crucreate tool tells Crucible to create a review, it automatically sets the Objectives of the review to the contents of the Perforce changelist description. This is important for a couple reasons.

First, obviously, it helps guide the reviewers by answering questions in advance about the point of the change. Reviewers should always check the objectives to get background on what they’re about to review. Saves a lot of time asking questions in comments that are already answered in the objectives.

And second, as it is a part of the review, it can and should be commented on (with a general review comment). No more lazy, useless changelist descriptions!

It’s for this reason that we modified our Crucible installation to pre-expand the objectives (normally collapsed by default) any time a review is viewed. Otherwise people never saw the objectives because they never bothered to expand them. [Thanks to the nice Atlassian support folks for their help here.]

The “Final Comment”

The last comment a reviewer makes, as documented in the last post, is to say what should be done with the review. Here are some samples I picked at random.

The most common final comment is something like “looks good, check in”.

This one is a little more complicated. Obviously some in-person discussion has happened in between, as well as an incremental UPDATE 1 that was attached.

Sometimes the last comment is the first comment as well. I found a great example of sharing knowledge using reviews. Here, a reviewer got added specifically so they could learn about JSFL in Flash.

And that’s a wrap! Just in time too – I’m getting on a plane in a couple hours (the first of three) to leave Quito and head on over to Sydney.

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?