A Peer Code Review Process

For Java Development

Change Summary	Author	Date
First Draft	Thomas J. Clancy	5/17/02
Simple revisions.	Thomas J. Clancy	11/24/03

Table of Contents

1 Introduction and Overview_ 3

2 Steps of the Code Review Process 4

3 Code Review Scenario Walk-Through_ 7

4 Pre-Review Checklist 8

1 Introduction and Overview

Code review is a very important part of the development process in that it helps to not only determine if the code itself follows the established coding guidelines, but also to help uncover any problems or bugs within the code.

Peer review of code is of course invaluable in this regard because not only does it directly benefit the review process by having programmers themselves review the code (thus helping to ferret out bugs more quickly) but also indirectly—benefiting the company as a whole—since these same programmers tend to review code with which they are unfamiliar; this, of course, helps to distribute knowledge of the code base among the teams rather than have it remain in isolation, worked on and improved by only one or two developers.

Although this document discusses the peer review process in more detail below, it should be noted by the reader that peer code reviews are not intended as a lynching party, where some unsuspecting developer is tied up by the thumbs and held dangling over the proverbial coals while all his or her mistakes are laid bare in front of him for all to mock and ridicule—remember your sensitivity training. No, peer code reviews are simply a way of improving the quality of the code by helping to ensure that the code is written according to the coding standards, that any bugs or problems are discovered and squashed, and that any fundamental mistakes in logic are found before they can cause harm.

This document, then, provides the guidelines and describes the process for holding effective and, I hope, painless code reviews. It also provides a simple, but complete code review checklist based on the established coding standards, that can (and should) be used before actually commencing the peer code review process.

With the advent of more sophisticated tools being designed to help ensure code robustness (e.g. code coverage analysis and style checkers), manages may wish to consider purchasing and investing time in such tools to help reduce the amount and time that developers need to spend doing all of this by hand. For example, a code style checker (e.g. JStyle), could be used to greatly reduce or even eliminate the pre-review process (discussed below). Thus a section is devoted to a simple introduction to the various tools that are available for such purposes.

2 Steps of the Code Review Process

There are five steps to the code review process that should be relatively simple to follow, and they are briefly described here. They are intended to keep the peer review process as short as possible because a developer’s time is money, and although code review is invaluable, developers need to be doing what they do best: writing code.

Here the steps are briefly defined:

Acceptance. Simply, this step is needed to nominate and accept a piece or pieces of code that are in need of a code review.
Pre-Review. This step is to be carried out by either the team lead or a designated developer to initially screen the code for review and ensure that it conforms to the checklist (provided below).
Peer-Review. Once pre-reviewed, copies are handed out to a select group of developers and a meeting is scheduled with a moderator to discuss the code being reviewed and to come to a consensus about the exact nature of any problems that need to be addressed.
Revision. This step is used to address and fix the problems found in the code during the peer-review.
Post-Review. This step is taken by the team lead or manager or an appointed developer to ensure that the revisions made were adequate to address and fix all of the problems (within reason) found through peer-review.

The following sub-sections go into more detail concerning each step.

2.1 Acceptance

Acceptance is the means by which code is selected and accepted for review. Usually the team leader or technical manager of a project team selects a portion of code (say a module or several related modules) to be reviewed. If necessary, the code in question is discussed with the team in a team meeting and acceptance of the code is either granted or denied for review.

2.2 Pre-Review

Once a selection of code has been accepted for review, either the team leader or an appointed developer provides a cursory examination (15 to 20 minutes, for example) of the code to make sure it conforms to within 90% of the checklist that is provided. If it does conform, then the okay is given and the team leader selects, usually randomly, three or four developers and appoints a moderator to perform the peer review. A meeting is then usually scheduled a week from the time the code passes the pre-review process, to actually perform the peer review. Note that it is probably wise to mix both junior and senior level developers within a peer-review team so that junior developers can learn by observing as well as contributing.

Copies of the code to be reviewed are printed out with line and page numbers and distributed to the selected peer-reviewers and the moderator. The developers who are peer-reviewing the code are then encouraged to examine the code in detail, no more than an hour a day, and at least an hour or so in all before the peer-review meeting, so that each developer can make notes as to what he/she believes is wrong with the code, and also to find any further problems in conformance with the coding standards. Doing this kind of review before the actual meeting helps to minimize the amount of time spent within it.

If the code, however, does not conform to within 90% of the checklist provided, this needs to be brought to the team lead’s attention, or if the team lead is doing the pre-review, he needs to bring it to the attention of the developer responsible for the code, at which point the developer will need to revisit the code and sufficiently edit the code to make sure that it does conform to the checklist. This of course would entail actually fixing or rewriting pieces of the code, adding proper comment documentation, etc.

The reason for this pre-review, really, is so that during that actual peer review, very few observations made by the review team will be about code conformance, and more observations will deal with real concerns regarding logic, potential bugs, efficiency problems and perhaps even simple design flaws; but note that the peer review process is not a design review and so more emphasis should be placed on problems and potential bugs within the code, or code improvement.

2.3 Peer Review

The purpose of the peer review is for the reviewers to come together and discuss the code being reviewed. These meetings, which are usually an hour to an hour and a half long, are where a consensus is made and a melding of the collective intellect occurs.

A moderator is put in place to make sure that the discussion stays casual and on-target. Which is to say that the discussions between the developers needs to remain focused on bugs or logic problems within the code, or those parts of the code that might have been missed during pre-review that do not conform to the coding standards and guidelines. These meetings are not for reviewing the design of the code or for simply beating to death the coding style—coding style, in fact, should have been caught at the pre-review stage anyway. These meetings are held simply to help catch bugs and logic errors within the code. This is not to say that some comments can’t be made regarding the design and that useful and constructive criticism couldn’t be made, but this should be done after the peer review and probably via email or within a separate meeting (a design review meeting, perhaps).

Often, one of the reviewers or the moderator him/herself is chosen as the scribe for the purpose of taking rigorous notes that reflect the problems encountered and agreed upon by the peer review team (e.g. class name, page number of print out, line number or range of line numbers on the page or pages spanned, method name, and a specific and detailed description of the problem—note that having a notebook computer and a fast typist in these sorts of meetings can be a great advantage over someone who simply has a pen and note paper). These notes are then added afterwards into the bug-tracking tool and assigned to a developer or to the team lead or manager of the group who will then delegate to one or more developers on the team the task of fixing the code.

2.4 Revision

The revision step is the step where the designated developer or developers fix the bugs that were generated from the peer-review step. Once fixed the bugs are then set as fixed within the bug-tracking tool at which point the post-review can then take place. Note that it isn’t necessary for all bugs to be fixed before a post-review. In fact the team lead or appointed post-reviewer could be reviewing the bugs fixes as they occur in parallel with the revision step as bugs are being fixed and checked in.

2.5 Post-Review

Post-review, which may be optional, is where the team lead or a designated developer checks to make sure that the bug fixes and general revisions conform to the problems discovered through the peer review process. This doesn’t necessarily mean that all bugs were found and fixed, but this should ensure that the ones that had been found during the peer review step were resolved correctly, which might entail a simplified version of peer review or even via pair programming as described in the Extreme Programming software development process.

3 Code Review Scenario Walk-Through

This section shows a walk-through of a typical code-review scenario.

Team lead or manager selects classes A, B, and C for review
Classes for review are accepted by the team (or by the team lead alone if he/she is responsible for solely making the decision as to what code gets to be reviewed)
Team lead hands a printout of classes with a printout of the checklist for each class being reviewed (3, in this case) to developer A
Developer A spends 15 to 20 minutes scanning code and filling out a checklist for each class in review
If each class reviewed is within 90% conformance with its checklist it’s then handed back to the team lead with a seal of approval
If one or more classes are not within 90% of its checklist, a developer is designated (either the originator of the code or someone else) to bring the problem code up to spec in as much as the coding guidelines are concerned
Once the code is up to spec. the team lead selects three or four developers at random as well as a moderator/scribe and then schedules a meeting from several days to a week from that point
The team lead produces printed, line and page numbered copies and hands these out to the selected developers of the peer-review team
Peer-reviewers are encouraged to spend a few hours between the time the code was handed out until the time of the meeting to review the code for bugs and to take notes, which will obviously expedite the entire peer review meeting
During the one hour peer-review meeting a focused and moderated discussion ensues and detailed notes are taken by the scribe
After the meeting the final notes, if they contain bugs and/or revision issues, are entered into the bug tracking software and assigned to the team lead or manager of the group
The team lead or manager then re-assigns the bugs to one ore more developers
When all bugs have been fixed and issues resolved a post-review is made by the team lead or a designated developer and the process continues until all the bugs and issues found in the peer-review meeting are adequately resolved

4 Pre-Review Checklist

Class Name:_______________________________________

Reviewer:_________________________________________ Date:________________

Code Format Conformance:

If the class is a standard java class, does it conform to the StandardFileTemplate.java template class? Yes [ ] No [ ] N/A[ ]
If the class is a TestCase class, does it conform to the TestStandardFileTemplate.java class? Yes [ ] No [ ] N/A[ ]
Are member variable naming conventions being followed? Yes [ ] No [ ] N/A[ ]
Are class and method naming conventions being followed? Yes [ ] No [ ] N/A[ ]
Are there any lines longer than 80 characters (see EJS[1] 6)? Yes [ ] No [ ]
Are all indentation 4 spaces (see EJS 8)? Yes [ ] No [ ]
Are all control statements and control statement alternatives surrounded by curly braces? Yes [ ] No [ ]
Are there spaces between operators and after commas (see EJS 7)? Yes [ ] No [ ] N/A[ ]
Do the curly braces conform to either the K&R standard or the vertical alignment standard and not both (e.g. no mixed styles within code)? Yes [ ] No [ ]

Coding Standard Conformance:

Are there pre-condition, post-condition and assertion checks in each method (see EJS 89)? Yes [ ] No [ ] N/A [ ]
Are all member variables private? Yes [ ] No [ ] N/A[ ]
Are there proper accessors and setters for member variables variables, if applicable? Yes [ ] No [ ] N/A[ ]

Javadoc and General Documentation Conformance:

Has the class as hole been documented with proper Javadoc comments and notations? Yes [ ] No [ ]
Have any static initialization blocks been properly documented? Yes [ ] No [ ] N/A[ ]
Have all public methods been properly documented (e.g. @param for all params, @return for all return values and @exception or @throws for all exceptions thrown by public method)? Yes [ ] No [ ] N/A[ ]
Have all package-protected methods (if any) been properly documented? Yes [ ] No [ ] N/A[ ]
Have all protected methods (if any) been properly documented? Yes [ ] No [ ] N/A[ ]
Is there proper documentation for all private methods? Yes [ ] No [ ] N/A[ ]
Are all incomplete or tricky portions of code tagged with a special doc flag (see EJS 63)? Yes [ ] No [ ] N/A[ ]

Exception Conformance:

Are all caught and ignored exceptions (if there are any) properly documented in the empty catch clause as to why the exception was caught and ignored? Yes [ ] No [ ] N/A[ ]
Do all public methods that throw exceptions define these exceptions within the same package? Yes [ ] No [ ] N/A[ ]
Do the catches of all the try…catch blocks catch the exact type of exceptions being thrown by the methods within the try…catch blocks (which is to say that the catch statement better not be catching base class exception classes of the exceptions being thrown)? Yes [ ] No [ ] N/A[ ]

To score, treat all Yes answers as 1, all No answers as -1 and all N/A answers as 0. Add all answers and calculate the percentage to determine the completeness of the code covered by this checklist.

5 Automation Assistance

[1] EJS refers to the Elements of Java Style book that we use as a guide for code conformance. The number next to each reference refers to a rule within the book.