Holistic code reviews
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
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
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.
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.
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
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).
A week before the review meeting, the author should distribute the code.
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
-name '*.java' | sort-directory-order`, as done above, or in
some other way. The example uses
to sort files in a more sensible order than
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.
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
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.
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
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.