Reviewing code written by others is hard, and yet it is one of the fundamental aspects of the Mozilla code process. Matthew Gertner recently posted about how he instituted a review requirement as part of the AllPeers development process, and asks some interesting questions about how reviews should be done.
What are reviews for? The primary goal of code reviews is to ensure
code correctness and quality:
- “goal” review: is the issue being fixed actually a bug? Does the patch fix the fundamental problem? It is not uncommon for coders to provide a patch to fix a particular undesirable behavior, without understanding the relevant standards or compatibility requirements.
- API/design review. Because APIs define the interactions between modules, they need special care. Review is especially important to keep APIs balanced and targeted, and not too specific or overdesigned.
- Maintainability review. Code which is unreadable is impossible to maintain. If the reviewer has to ask questions about the purpose of a piece of code, then it is probably not documented well enough. The reviewer should enforce common style guidelines, with the help of automated tools when practical.
- Security review. This is mostly a subset of design review. Reviewers should ensure that the design uses security concepts such as input sanitizers, wrappers, and other techniques. It may also be appropriate to do a detailed review of the code which is exposed to public content.
- Integration review. Does this code work properly with other modules? Is it localized properly? Does it have server dependencies? Does it have user documentation?
- Testing review. The best time to develop a comprehensive test suite for code is when it is first developed. Automated and manual tests should be developed for all code modules. These tests should not only test correct function, but also test error conditions and improper inputs which could happen during operation.
For the most part, reviewers are not responsible for ensuring correct code function: unit tests are much better suited to that task. What reviewers are responsible for is much more “social”, and typically does not require a detailed line-by-line analysis of the code to perform a review. In many cases, important parts of the review process should happen before a coder starts working on a patch, or after APIs are designed but before implementation.
There are some important side effects of the review process that are also beneficial:
- More than one person knows every piece of code. Many Mozilla modules have grown a buddy system where two people know the code intimately. This is very helpful because it means that a single person going on vacation doesn’t imperil a release or schedule.
- Reviewing is mentoring. New hackers who are not familiar with a project can be guided and improved through code review. Initiall, this requires additional effort and patience from the reviewer. Code from inexperienced hackers deserves a much more detailed review.
- A public review log is a great historical resource for new and experienced hackers alike. Following CVS blame and log back to bug numbers can give lots of valuable historical information.
There can’t really be general guidelines on how much time to spend reviewing. Some experienced hackers may spend up to 50% of their time doing reviews (I typically spend two days a week doing design and code reviews and various planning tasks). This can be hard, because coding feels much more productive than reviewing.
