Peer Code Reviews: Good Commenting Practices
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.
Page 1 of 5 | Next page
