Thomas A. Alspaugh
Reviewing for Quality

Reviews, inspections, and walkthroughs are all names for similar processes, in which a group works its way through an artifact in a systematic fashion, in a combination of a presentation of the artifact and a discussion of issues that the group finds. The primary goal is to find problems and (without getting bogged down in the details and merits of competing solutions) suggest improvements.

Why review?

  1. You can review sooner than you can test (because there are artifacts before there is running code).
  2. You can review things you can't test (like requirements, designs, tests themselves, documentation, …).
  3. Each problem caught early won't then cause a cascade of other problems based on the first one.
  4. Immediate benefits: find problems in current work.
  5. Indirect benefits: improve people's skills and consolidate the team.

How to hold an effective review meeting

(Many of these items are simply good practice for meetings in general.)

  1. Assemble a team of appropriate people to do the review.
    1. peers at the same level, or
    2. senior experienced pros, or
    3. outside experts (for expertise, authority, and scapegoating)
  2. Except for peer inspections within a team, include people from other areas of expertise as well as experts in the area of the artifact being reviewed.
  3. Schedule it soon.
  4. Assign some roles.
    1. The presenter who presents the material (not necessarily the material's author)
    2. The scribe who records action items and important points of the discussion.
    3. Someone must be in charge (usually the presenter)
  5. Keep the review meeting on track.
    1. Make an agenda (items to review and tasks to accomplish) and follow it.
    2. Start on time and end on time.
    3. Use the time budget to decide when to move debate offline, and to enforce the cutoff.
  6. Produce action items and plans for each identified problem.
  7. Follow up (after the meeting) on each action item.

Pressman's Golden Guidelines for review meetings

Pressman lists these guidelines, many of which again are simply good practice for any meeting:

  1. Accumulate and use checklists for each kind of artifact being reviewed.
  2. Train senior people to review well.
  3. Analyze effectiveness of past reviews and make improvements.
  4. Schedule reviews and allocate resources for them as part of the development plan.
  5. 3-5 person review team is optimal.
  6. Be constructive and not personal.
  7. Keep to the agenda.
  8. Focus on defects, not on solutions.
  9. Can't agree? Make a note, plan to discuss it later, and move on.
  10. Document the discussion.
  11. Two hours max.
  12. Write up a report.
  13. Follow up.

Active Design Reviews

Parnas and Weiss propose a much different approach for effective reviews of designs; their approach can be adapted to any artifact.

In order to make the review effective, the reviewers are forced to force the reviewers to take an active role whether they agree or disagree with the design decisions that have been made. The reviewers must be asked to provide justification for accepting or rejecting design decisions. 

Because using a document is often the best way to find its problems, we try to make the reviewers use the document that they are reviewing. We do this by asking them to answer a set of questions about the document—questions that can only be answered by careful study of the document. Parnas and Weiss p.260 

Below is a checklist of points for an active design review, formatted as a list for ease of reading:

  1. Rather than conduct one large meeting, we identify several types of reviews, each designed to find different types of design errors.
  2. Since each review requires its own expertise, we identify different types of reviewers needed to perform the reviews.
  3. Instead of conducting the reviews as discussions, we devise questionnaires for the reviewers to answer using the design documentation. Responding to the questionnaire requires the reviewer to study the documentation carefully, and to make assertions about design decisions.
  4. The issues raised by the reviewers as a result of answering the questionnaires are discussed in small meetings between designers and each reviewer. There are no protracted group discussions involving all designers and all reviewers.
  5. Because our reviews are very tightly focused, we also ask a few people for overall reviews to decrease the chance that we overlook problems because we neglected to pose a question.

Parnas and Weiss p.261, a single paragraph in the original

The review is conducted in three stages, of which only the first involves a meeting of all the reviewers.

  1. Our reviews are conducted in three stages. In the first stage, a brief overview of the module being reviewed is presented. The overview consists of explaining the modular structure if it is unfamiliar to the reviewers, showing where in the structure the module belongs, and describing the module's secrets. If previous assignments have not been made, reviewers are assigned to reviews and sections of the document. Reviewers are also assigned times during which they may raise any questions they have and a time to meet with designers after the designers have read the completed questionnaires.
  2. In the second stage, reviewers perform their reviews, meeting with designers to resolve questions they have about the design or about the review questionnaires.
  3. In the third stage, the designers read the completed questionnaires and meet with the reviewers to resolve questions the designers may have about the reviewers' answers to the questionnaires. The reviewers may be asked to defend their answers. This interaction continues until both designers and reviewers understand the issues, although agreement on those issues may not be reached. After the review, the designers produce a new version of the documentation. A discussion of design issues, including those raised during the review, are contained in our documents.

Parnas and Weiss p.263

The authors also emphasize the importance of making the design and the design representation reviewable in the first place [p.261], but that is beyond the scope of this page.

Pair Programming

Pair programming is a form of continuous code review. The best summary of this practice is provided by Williams and Kessler in a list of elementary principles for effective pair programming, in a tongue-in-cheek reinterpretation of Fulghum's All I Really Need To Know I Learned In Kindergarten, from which excerpts for each item are quoted below:

Share everything
It is not acceptable to say or think things such as, You made an error in your design, or That defect was from your part. Instead, We screwed up the design, or better yet, We just got through our test with no defects! Both partners own everything.
Play fair
With pair programming, one person drives (has control of the keyboard or is recording design ideas) while the other is continuously reviewing the work. Even when one programmer is significantly more experienced than the other, it is important to take turns driving, lest the observer become disjointed, feel out of the loop, or unimportant.
Don't hit people
Make sure he or she stay focused and on-task (non-violently, of course).
Put things (especially negative thoughts) back where they belong
[A]nyone can control this negative self-talk by putting these thoughts where they belong—out of mind—every time they start to creep in. The surveyed pair programmers indicated it was very difficult to work with someone who had insecurity or anxiety about their programming skills. They tend to have a defensiveness about them. Programmers with such insecurity should view pair programming as a means to improve their skill by constantly watching and obtaining feedback from another.

Also, negative thoughts such as I'm an awesome programmer, and I'm paired up with a total loser should also be rejected, lest the collaborative relationship be destroyed. None of us, no matter how skilled, is infallible and above the input of another.

Clean up your mess
Pair programmers have the advantage of the presence of a partner to help them clean up. Many have related that many obvious, but undetected, defects were noticed by another person watching over their shoulder. Additionally, these defects can be removed without the natural animosity that might develop in a formal inspection meeting.
Don't take things too seriously
A true scenario about a programmer seeking review of the code he produced is discussed in [9]. On this particular bad programming day, an individual egolessly laughed because his reviewer found 17 bugs in 13 statements. After fixing these defects, however, the code performed flawlessly during testing and in production. How different this outcome might have been had the programmer been too proud to accept the input of others or had viewed this input as an indication of his inadequacies.

Conversely, a person who always agrees with their partner lest create tension also minimizes the benefits of collaborative work. For favorable idea exchange, there should be some healthy disagreement/debate.

Say you're sorry when you hurt somebody
The programmers must be able to sit side-by-side and program, simultaneously viewing the computer screen and sharing the keyboard and mouse. Extreme programmers have a slide the keyboard/don't move the chairs rule.
Wash your hands before you start
Tom DeMarco shares his inspiring view on this type of union in [4]. A jelled team is a group of people so strongly knit that the whole is greater than the sum of the parts …

Wash your hands of any skepticism, develop an expectation of success, and greet your collaborative partner by saying, Jell me! This is an unprecedented opportunity for the two to excel as one.

Flush.
Inevitably, the pair programmers will work on something independently. Of the programmers surveyed, over half said they reviewed work done independently when they rejoined with their partner, and incorporated it into the project. Alternately, extreme programmers flush and rewrite independent work. In their XP experience, the majority of the defects could be traced back to a time when a programmer worked independently.
Warm cookies and cold milk are good for you
Taking a break periodically is important for maintaining the stamina for another round of productive pair programming. During the break, it is best to disconnect from the task at hand and approach it refreshed when restarting. Suggested activities: checking email, making phone calls, surfing the Web, eating warm cookies, and drinking cold milk.
Live a balanced life—learn some and think some and draw and paint and sing and dance and play and work every day some
Communicating with others on a regular basis is key for leading a balanced life. If asked, most programmers would probably say they preferred to work alone in a place where they wouldn't be disturbed by other people [9]. But, informal discussions with other programmers—the one you are paired with or any other—allow for effective idea exchange and efficient transfer of information.
Take a nap (or a break from working together) every afternoon
It's certainly not necessary to work separately every afternoon. But, according to 50% of the surveyed programmers, it is acceptable to work alone 10%-50% of the time. Many prefer to do experimental prototyping, tough, deep-concentration problems, and logical thinking alone.
When you go out into the world, watch out for traffic, hold hands and stick together
With pair programming, the two programmers become one. There should be no competition between the two; both must work for a singular purpose, as if the artifact was produced by a singular good mind. Blame for problems or defects should never be placed on either partner. The pair needs to trust each other's judgement and each other's loyalty to the team.
Be aware of wonder (and the power of two brains working together)
Experiences show that a pair will come up with more than twice as many possible solutions as two individuals working alone. They will then proceed to more quickly narrow in on the best solution and will implement it more quickly and with better quality.

Williams and Kessler, excerpts

The role of the intelligent ignoramus

Berry, in his engaging The Importance of Ignorance in Requirements Engineering, posits that an ignorant (but smart) reviewer possesses certain invaluable advantages. His or her ignorance gives license to ask all sorts of foolish or very basic questions, and obligates the experts to take no offense and to give answers that anyone could understand. He is talking about software requirements, but the same situation arises for any kind of review. Berry hypothesizes:

… the very fact that I knew so little about the client's application area [was] a significant factor in the success …. By being ignorant of the application, I was able to avoid falling into the tacit assumption tar pit.

It was clear that the main problem preventing the engineers at the company from coming together to write a requirements document was that they were all using the same vocabulary in slightly different ways, and none was conscious of any other's tacit assumptions. Each was wallowing deep in his or her own pit. My own ignorance forced me to ferret out these tacit assumptions, and my lack of assumptions forced me to regard the differences in the way each was talking about the same thing as signs of inconsistencies.

Berry p.183

He recommends:

On the basis of these experiences, it seems clear that it is essential for each requirements engineering team, consisting of analysts and client representatives, to have at least one domain expert and at least one smart ignoramus! … The ignoramus should be smart about computing, information hiding, and strong typing, but totally naive and ignorant in the client's domain. It is absolutely essential for the ignoramus to be attuned to spotting inconsistencies in what people are saying; this means that the ignoramus should have a good memory for what people say and a good sense of language to be able to detect when a verb is being used with a different set or number of objects. These language skills are required of lawyers, psychologists, psychiatrists, negotiation facilitators, etc.

In a team consisting of at least one ignoramus and at least one expert, each kind of person will find different things about the requirements. The domain expert finds the basic information needed, but he or she falls too easily for tacit assumptions. The ignoramus has no assumptions and asks questions whenever he or she catches a sign of something left unsaid.

We have the interesting situation that ignorance is both necessary and something to overcome. It seems that if there were no ignorance to overcome, the consensus process would be stunted.

Berry pp.183-184

References

Daniel M. Berry. The importance of ignorance in requirements engineering. Journal of Systems and Software, 28(2):179–184, 1995.
Abstract
doi:10.1016/0164-1212(94)00054-Q
Tom DeMarco and Timothy Lister. Peopleware: Productive Projects and Teams. Dorset House, 1977.
Robert Fulghum. All I Really Need To Know I Learned In Kindergarten. Ivy Books, 1989.
D. L. Parnas and D. M. Weiss. Active design reviews: principles and practices. Journal of Systems and Software, 7(4):259–265, 1987.
Abstract
doi:10.1016/0164-1212(87)90025-2
Roger S Pressman. Software Engineering: A Practitioner’s Approach. McGraw-Hill, 1982. Currently in its seventh (2009) edition.

Currently in its seventh (2009) edition; focused on big-system, big-team development.

Gerald M. Weinberg. The psychology of computer programming. Dorset House, 292 pages, 1998.
Laurie A. Williams and Robert R. Kessler. All I really need to know about pair programming I learned in kindergarten. Communications of the ACM, 43(5):108–114, 2000.
Abstract
doi:10.1145/332833.332848

flip bgunflip