Before I started to work at Microsoft I worked with a firewall software. At that company the whole team reviewed code together after a new module was completed and made ready for deployment. Reviewing a module could take days and involved people who had not seen the code once before the review. I don’t think this was a very effective way to perform reviews, especially since design decisions that were bad often were only pointed out but not fixed since the module was “done”. only fixes regarding security were really fixed after these reviews.
I think you have to review code continuously in order to make them effective and valuable. And you should have a simple rule making sure all code is reviewed and the simplest rule you can have that works is “all code checked in should be reviewed“. This does not mean you have to review before check in or after. Reviewing before check in is advantageous since you get less crappy code check in. The drawback is that a change that others would benefit from takes longer to make it into the code base. Also you don’t get a history of the code since the “before check in code” is not checked in. If you on the other hand review code after a check in you can do it whenever you get the time. I think the major drawback with this approach is that it is easy to forget or ignore code reviews if that can be made at a later date.
The next option you have is who will perform the review. I know some people favor doing reviews alone when you have time. This way no one needs to be interrupted in what they do just to do a review. Another option is to have the reviewer sit down with the author of the code under review and look at the code together. I find this kind of review the most valuable and effective since the reviewer can ask questions directly to the author and discuss things directly rather than compiling a “review report”. And the “someone is interrupted” problem is no real problem in reality. In a team of developers it is rare (in my experience) that everybody is 100% occupied. Almost all the time you have somebody being “between tasks” or someone doing boring repetitive updates or just being stuck on some problem. Under all these circumstances the reviewer is happy to take a break from his (or her) current tasks and perform a review. If however nobody is available right away somebody is usually able to do the review within 15 minutes without being seriously interrupted.
In our team we do these face-to-face reviews prior to check in. Actually our process in very similar to the process described here. And here are my personal guidelines for effective, valuable check in reviews:
- Author of code prepares the review at his workstation.
- Author describes the purpose of the changes to the reviewer.
- The reviewer is in command of the review. This means the reviewer is in control of the keyboard and mouse and the reviewer asks questions.
- The author should not comment on anything unless asked by the reviewer.
- Reviewer decides if changes needed as a result of the review should be reviewed again or not.
Another common question is if there are reviews so simple that you don’t have to review them. Examples might include single line bug fixes or correcting spelling errors in comments. Personally I think it is dangerous to have rules where somebody has the opportunity to decide for them selfs. A good rule is unambiguous. So the rule should be “all check ins” since then you don’t have to decide where to draw the line. And really simple reviews will be over in seconds so why not have them…