Sunday, July 7, 2013

Code reviews

After reading Aras' Reviewing ALL the CODE entry, I was going to reply describing our process, but it was getting long so I decided to write it up here.


Our process is a little more lo-fi but effective. We use perforce's review daemon, and as part of programmer orientation we set new programmers up to subscribe to the source code folder and set up an outlook filter. That's right, every programmer on the team has a stream of emails for every changelist.

The emails are set up with the first line of the changelist description in the subject and the email of the changelist author in the reply-to field. The body of the email contains the full changelist comment and diffs of the change up to a certain size to avoid flooding our email system.

Reviews are handled by replying to the email, and cc'ing a code reviews email list which goes to everyone and is archived. This is so everyone gets the benefit of subsequent discussion.

We have a few senior engineers who at least read every changelist comment. Personally, I find it is something useful to do while waiting the couple minutes for a big compile to finish. But looking at our code reviews email list, quite a few programmers scan at least some of the changelists, usually looking for changes in code they are most familiar with.

This only works if you enforce meaningful changelist comments. "Fixed bug in renderer" would not be an acceptable changelist comment and would garner a review email to make a better one. A changelist comment should describe the problem being solved and how it was solved. It doesn't need to be a novel but it should be enough information so someone going back to this changelist 6 months from now can understand what was done and why.

I've worked on teams where this was the primary code review process, although currently we use it as a second line of defense - each changelist requires a primary reviewer.

We catch all sorts of issues with this review process - minor issues such as code that is unclear all the way to major bugs that were uncaught by both the author and the primary reviewer. This is a good method for orienting new programmers to the code base, teaching "code base lore", or pointing out bad naming. One of the things I particularly look for is badly named functions or variables - code without short, concise and meaningful names is usually an indicator of a larger problem. I could do a whole entry on names.

Beyond the day to day issues, I also find it is a useful toward improving the quality of the code base as a whole. If you see the same mistakes being made or people having trouble with a particular system, it gets you thinking about ways to prevent those mistakes, or make a system easier to use.

All in all, I find it useful for getting a feel for how the team as a whole is operating and also learning about parts of the code base you might not normally delve into. It's difficult to give people advice on how to solve problems if you don't have at least a cursory understanding of what they are working on. It is also low ceremony and a way to communicate what's going on across largish teams. If you're not doing it, give it a try.