Hackystat Developer Documentation
Software Review Guidelines

Philip Johnson
Collaborative Software Development Laboratory
University of Hawaii

$Id: Review.html,v 1.3 2005/02/14 19:03:39 johnson Exp $

1.0 Motivation

Many of the CSDL meetings are dedicated to Hackystat software review. This page is intended to help define and regularize the review procedure in order to minimize the time we spend on it and maximize the benefits we get from it.

The primary goal of the traditional software review process is to remove defects. That is not the primary goal of this process, which is to create an environment in which participants learn new things about high quality software design and implementation. There are several process-level implications of this goal. First, discussion during the meeting is expected to be wide-ranging and "digress" from the particular issue at hand. As long as we're learning something new about high quality software design, it doesn't matter if it directly relates to the code we're reviewing at the moment. Second, we apply the 80/20 rule in various cases, such as by limiting the time that can be spent preparing for the review session. While that might reduce the number of defects found, it keeps the review "lightweight" and prevents excessive workload for participants.

2.0 Procedure

The CSDL Software Review Process consists of the following steps: (1) Announcement, (2) Preparation, (3) Review, (4) Revision, and (5) Verification.

Step 1. Announcement

In the announcement step, the author(s) of a code segment send an email to hackystat-l (or whatever mailing list is most appropriate) with a description of the software to be reviewed. In general, it is referable to limit the amount of code to be reviewed to less than half a dozen classes, although you should use your best judgment.

In addition to listing the files to be reviewed and their location, please provide a list of your "burning questions" regarding this code. What are you worried about? What will the code be enhanced to do next, and what are your concerns regarding that? In general, help the reviewers to provide you with insights into the most important problems you currently feel you have regarding the code.

The announcement email should be sent approximately 24 hours in advance of the review meeting.

Here's a nice example of a software review announcement email:

Greetings, folks:

This Wednesday (09.07.2004) at noon in POST 307, we will be reviewing the Daily Project
classes in the hackyReview module. To perform this review:

0. Install and/or update (a) the Eclipse Jupiter plug-in and (b) The Hackystat Jupiter sensor.
1. Update the hackyReview module from CVS.
2. Select "ProjectActiveTime2" as the ReviewID. 

Please review the following files:
* MostReviewActiveData.java
* DailyProjectReviewActiveTime.java
* ReviewActivityReducer.java

I am particularly concerned about the following:

1. How should the code determine the most active review file? Currently, the file with the most review ID 
and the most review phase is returned in this method. This will enable us to analyze the most review active 
file filtered by review ID or review phase, or both.

2. In DailyProjectActiveTime, is the any way to avoid the hard-coded 5, which converts the DayArray index 
into the number of minutes that it represents?  Should the DayArray have a static constant field such 
as DayArray.INTERVAL?

3. In DailyProjectReviewActiveTime:166, should I check the unchecked exception here?  
DayArrayException will be thrown if clients (I ) use the following incorrectly :

reviewActiveData.getDayArray().iterator()
i.nextNonempty()

However, the regular use of the iterator pattern in Java does not force clients to check the 
unchecked exception, but if the clients use it incorrectly, the IndexOutOfBoundsException (unchecked) 
will be thrown. Since we know the contract of the violation that we are supposed to use i.hasNextNonempty() 
and i.nextNonempty() step by step, should I retain the catch clause in my code?

-------------------------------------------------------------
Supplemental Links and Resources:

The Hackystat Code Review Guidelines:
* http://hackydev.ics.hawaii.edu/hackyDevSite/doc/Review.html

The Elements of Hacky Style:
* http://hackydev.ics.hawaii.edu/hackyDevSite/doc/EHS.html

Jupiter Code Review Tool documentation:
* http://csdl.ics.hawaii.edu/Tools/Jupiter/Core/doc/UsersGuide.html

Hackystat Jupiter Sensor documentation:
* http://hackystat.ics.hawaii.edu/hackystat/controller?Page=help&Subpage=install&Sensor=Eclipse-Jupiter

Thanks for reviewing the code!

Takuya

================================
Takuya Yamashita
E-mail: takuyay@hawaii.edu
================================

Step 2. Preparation

In the 24 hours between the time of the announcement and the actual review meeting, all of the participants in the review (except the author(s)) should read the code and prepare for the meeting. Preparation guidelines include:

Spend no more than 1 hour reading and analyzing the code. It is perfectly acceptable to do your preparation during the hour immediately preceding the review, as long as you know you can work without distractions for that hour (and ignore the rumbling in your stomach). Remember that you are doing this to help out a fellow group member, and what goes around comes around.
Use the Eclipse Jupiter plug-in to perform the review. If you encounter issues with the plug-in, please post them in the Jupiter area of the Hackystat Scarab bug tracking system.
In addition to the burning issues brought up by the author, always check for violations of The Elements of Hacky Style.
Make sure you commit your Jupiter-generated .review file to the CVS repository prior to the meeting.

Step 3. Meeting

During the face-to-face meeting, we go through each comment using the Jupiter group review mode. In many cases, we'll edit the files to include additional instructions or comments.

Step 4. Revision

After the review has concluded, one or more people (usually the authors) will "volunteer" to perform revision on the code following the comments brought up in the review and recorded in the text files. Here are the guidelines for performing revision activities:

Please edit each Jupiter issue to indicate what you did (if anything) in response to the review comment. Sometimes, even though we're all agreed on a change during the meeting, once you actually get into the hacking, you realize it wasn't such a good idea after all. That's fine; the important thing is to record what you decided to do.
In general, you have one week to do the revisions, which will be reviewed during the verification phase at the next weekly meeting.
Remember to commit your updates to the CVS repository!

Step 5. Verification

In the verification phase, the group goes (quickly) through the text files one more time, this time focusing on what the authors did in response to the comments, and perhaps asking for clarification. Verification typically occurs at the start of the Thursday lunch meeting, prior to beginning the review of the current week's code.

In some cases, verification generates enough comments that we decide to perform another review. This is OK--it just means you are tackling a particularly difficult problem.

3.0 The "Develop-Review-Enhance-Disperse" (DRED) process model

The Hackystat software review process is actually a component of a larger development process that we affectionately call "DRED".

DRED arose in response to the fact that many Hackystat development tasks have a similar nature: they consist of initial development of a new or improved "service", which must then be "dispersed" throughout the remainder of the system to enhance or correct pre-existing code. For example, the development task for the new XML-based approach to user data persistence will begin by extending the User class with new facilities. Once implemented, these facilities must be adopted by other components that until now have implemented their own user-level persistence mechanisms due to inadequacies in the current Properties-based User persistence.

In this situation, there are a couple potential risk factors:

The service is dispersed into other components before it is ready. This could result in a certain amount of "thrashing", as the service is redesigned and then the changes have to be re-dispersed into the components again.
The service is idiosyncratic and understood only by one developer. This could result in a less elegantly designed, harder to understand implementation.

The Develop-Review-Enhance-Disperse (DRED) process is my proposal for a way to reduce these risks during the development of these tasks. Note that this risk reduction should translate directly into a reduction of time spent in development.

3.1 DRED Stage One: Develop

The "Develop" stage of the DRD process involves the initial development of the service. Each task has a principal developer (the "Master Chef", or MC) as well as someone they have official permission to bug regarding problems and bugs (the "Sous Chef", or SC). I recommend, but do not enforce, that agile practices like Pair Programming and Test-Driven Development be followed during this stage if the MC and SC deem them useful.

3.2 DRED Stage Two: Review

Once the Develop stage has reached a stable point in implementation, the developers should call for a Review of their design and implementation. The review should follow the guidelines posted above.

3.3 DRED Stage 3: Enhance

After review, the service goes back to the MC and SC for enhancement and improvement. Once these changes have been completed, then the service moves on to the next phase, dispersion. (In some cases, the service may require one or more additional rounds of review before dispersion).

3.4 DRED Stage 4: Disperse

Once the service is ready for dispersion across the Hackystat code base, this task will be assigned to developers other than the original MC (and perhaps SC). The point of this phase is to ensure that developers other than the original implementers gain experience as "clients" of the service early on in its life. This spreads knowledge and also may uncover additional opportunities for enhancement and improvement of the service.

4.0 Comments solicited

We are still learning how to best perform software review in a way that fits our group members and our goals as a research group. If you have comments or ideas on how to improve it, don't hesitate to bring them up.

Hackystat Developer Documentation Software Review Guidelines