Holistic code reviews

by Michael Ernst

March, 2011
Last updated: May 23, 2016

Code reviews consist of others reading your code, then providing you with feedback (both at a high level and at a low level). Code reviews improve your code by revealing bugs, questionable coding practices, and locations where the documentation should be improved. The better the documentation (and the code itself) is, the more useful the comments you receive will be. Therefore, it is unacceptable to schedule a code review unless your code is documented.

Code reviews are interesting for the reviewers, who get to see how someone else solved a problem and how that person codes. The reviewers also become more familiar with the code, which enables them to assist with the code when the original author needs help or is unavailable.

There are two general approaches to code reviews: incremental or holistic. An incremental code review examines, comments on, and approves/rejects each proposed change, as soon as a developer proposes the change. A holistic code review examines an entire codebase or module. Each approach is appropriate in certain situations. This document discusses the holistic style: periodic review of an entire codebase. Such reviews ideally occur multiple times per year. It's also desirable to have a review before checking in code for the first time, to ensure that you haven't inadvertently made errors or written code that will be hard for others to understand or modify.

The process for improving code is very similar to the process for improving writing. (This is no coincidence!) The author ensures the document is ready for review and points out the parts that are most in need of review. The reviewer reads the document to provide detailed comments. At a meeting, the reviewer raises both the issues that are most important and also those that the reviewer is least confident about. The author makes changes based on the discussion and the written comments. Finally, there is an important followup meeting where the author raises any reviewer comments that the author does not agree with.

This document describes one successful approach to code reviews. Others are possible; adapt the instructions as appropriate for you.

1. The author ensures that the code is ready for review. At a minimum, every file/class/procedure must be documented. Each organization will have its own coding guidelines. An example is that (for Java code) javac -Xlint must issue no warnings.
   
   Don't write any extra introduction or overview just for the reviewers, such as a description of the code or how it fits into the rest of its system. It is better to let the reviewers discover that from your system documentation, just as anyone else reading your code would. The main goal of the code review is to ensure that others can understand and maintain it in the future, so the code review should simulate that environment.
   
   If the code is not ready, you should delay the review. That is better than having everyone spend a lot of time reviewing the code, only to tell you that the documentation is inadequate.
2. The author determines what code to review. You can specify specific files that the reviewers should focus on, or can give general guidelines about parts of a system, or can just tell the reviewers to examine whatever they like. It should be possible for the reviewers to read through the code in half a day or less. It is essential that the code contains overview documentation such as README files and Java package-info.java files.
   
   Especially if reviews are done frequently, a group of related modules comprising 7-15 pages is appropriate. However, it is better to err on the side of too much rather than too little code (perhaps putting the extra code at the end of the list of files). The reviewers should be able to figure out what parts are less interesting (if they cannot, that is valuable information for the author!). And the reviewers may be confused about something other than what you thought they would be. Programmers who are really reading the code (not participating in a code review) will have to read everything anyway, so you might as well find out during the code review what problems they will have. Finally, the extra files may be necessary to understand parts of the code that you are most interested in getting feedback on (such as a base class or a class that is called by or uses the main classes).
3. A week before the review meeting, the author should distribute the code. Some reviewers prefer to work with the original source code in electronic format. Others find hardcopy better for reading and for writing notes. Optionally, you can provide a PDF for reviewers who wish to work with hardcopy. (The reviewers can always make their own hardcopy, but if you provide one, the page numbers are consistent for all reviewers.)
   
   If you wish to print the code, this command creates PDF files suitable for printing.

     enscript --landscape --font Courier6 -T 4 -E -2 -A 2 --toc -p - `find * -type f -name GLOB -print | sort-directory-order` | ps2pdf - outfile.pdf
     % An alternate way to get page numbers, if "--toc" causes enscript to core-dump
     enscript --landscape --font Courier6 -T 4 -E -2 -A 2 -p - files | pspage -l -x .42 -y 10 - | ps2pdf - outfile.pdf
     % If many lines are longer than about 95 characters, print rotated
     enscript --landscape --font Courier6 -T 4 -E -r -A 2 --toc -p - `find * -type f -name GLOB -print | sort-directory-order` | ps2pdf - outfile.pdf
   
   

   The list of files can be specified as something like `find * -name '*.java' | sort-directory-order`, as done above, or in some other way. The example uses sort-directory-order to sort files in a more sensible order than find itself does.
   
   The file name that you specify will be printed in the page headers. Therefore, use a path relative to the common ancestor, so that it is apparent which directory (or Java package) a file is in. Do not use an absolute pathname, which contains irrelevant information and may even be too long for the page number (within the file) to fit in the header.
4. The reviewers mark up the code in detail (either electronically or on the hardcopy) for the author to read after the meeting.
   
   The reviewers prepare, for the meeting, a prioritized list of their 5 most important comments/suggestions/questions about the code. Reviewers are encouraged to send as many comments as possible to the author in advance of the meeting when possible. This gives the author time to prepare for the meeting, or even to revise the code before other reviewers read it, so that they have an easier time focusing on other issues.
5. During the meeting, go around the table with each reviewer raising his or her highest-priority issue (that has not yet been discussed), and everyone discussing that issue. The author takes notes during the discussion.
6. After the review, it is the author's responsibility to address both all comments that came up in discussion, and also all comments that the reviewers wrote about the code.
   
   Most of the reviewers' comments will be noncontroversial, and the author can apply the changes. However, the author may disagree with others. In this case, the author should arrange a meeting with the reviewer (or, in some cases, even reconvene the code review meeting) to discuss each of these. This is one of the most important parts of the code review, and should not be skipped. Whenever there is a difference of opinion about the structure of the code, then someone is sure to learn something: about programming (from design to testing), about the structure of the program under review, or about deficiencies in the documentation. Either the author or the reviewer may learn from this process.
   
   Finally, it's often a good idea to let the reviewers know when the changes are finished, so they can take another look. The code improvements may permit the reviewer to make even more and better suggestions about other parts of the code. As with any engineering activity, feedback is critical to producing a great product, and iterations improve the quality.

Back to Advice compiled by Michael Ernst.