Path: utzoo!utgpu!news-server.csri.toronto.edu!cs.utexas.edu!sun-barr!lll-winken!uwm.edu!uwvax!shorty.cs.wisc.edu!dparter
From: dparter@shorty.cs.wisc.edu (David Parter)
Newsgroups: comp.software-eng
Subject: Re: Code inspections
Keywords: inspection, software engineering
Message-ID: <1991Feb1.214750.28536@spool.cs.wisc.edu>
Date: 1 Feb 91 21:47:50 GMT
References: <40530@genrad.UUCP> <7362@tekchips.LABS.TEK.COM> <1991Jan29.230431.5918@cbnewsm.att.com>
Sender: news@spool.cs.wisc.edu (The News)
Organization: University of Wisconsin CS Department
Lines: 85

At my previous place of employment, some type of code review/read/inspection
was accepted as normal. Unfortunatly, there was no clear agreement on
what exactly this event was all about (thus the multiple choice name),
so some code reviews were more detailed than others.

Here, however, I offer some observations about the process. Of course,
this is all anecdotal, and your mileage will vary...

    1.  The more comfortable the author (and the reviewers) are with
	the others involved in the review, and with the review process,
	the more productive the review will be, and the more likely to
	avoid ego battles. Of course, if everyone is everyone's best
	friend and therefore refrains from offering valid criticism,
	then it is a waste of everyone's time.

    2.  The more experience all the participants have with code
	reviews, the more effective they are. Once everyone knows what
	to expect ("yes, they will criticize my comments, and if my
	comments aren't telling them what the code does, then I guess
	they are right, my comments aren't good enough"), and has had a
	few chances to be both reviewer and author, they are
	comfortable with offering and taking criticism.

    3.  MAKE SURE TO STAY WITHIN THE AGREED PURPOSE OF THE REVIEW. The
	worst review I have ever heard of (I was not there, I was down
	the hall and heard a lot of it) was the following situation:

	The code author was a new hire (w/in 6 months), just out of
	college. It was the first time his code had been subject to
	review.  His manager was not at the review (he was not in town,
	in fact).  One of the reviewers was a senior person who had
	been invovled in the initial project plan (of which this was a
	small, but independent part), but not at any of the subsequent
	reviews related to this part (functional specification,
	design).

	The senior reviewer proceeded to turn the code review into a
	design review, ripping the code and design to shreds, and
	producing a new design at the meeting.  No one stopped him, and
	no one defended the programmer. Needless to say, the programmer
	was very upset. When his manager returned, and heard what had
	happened, he was not pleased either -- but he knew that it
	wasn't the programmer's fault, and told him so.

    4.  Good code reviews, with "tough" reviewers often leads to better
	code because the author makes an effort to prepare for the
	review.  He or she may read through the code, anticipating the
	comments of the reviewers, and improving things beforehand.
	What would have been "good enough" in the past gets improved by
	the best person to do the improving -- the original coder.

    5.  One poster mentioned that the novice programmer presenting his
	or her code for review the first time may be very upset by the
	review. One way to soften the blow is have the novice
	participate as an extra reviewer (it was also mentioned that he
	or she may be wary of criticizing senior people, so "extra"
	because they may not be that productive) at several reviews of
	other parts of the project, with (some or all of) the same
	reviewers who will be reviewing his or her code, so that he or
	she will be used to their personal style and will know what to
	expect.

    6.  If the reviewers do not understand the code, or how it fits
	into something else, DO NOT RELY ON THE AUTHOR to clarify. 
	
	I was the author for a sets of changes to some existing code.
	The review focused on the parts I changed, not on the program
	as a whole. I was called upon to give an overview of how it all
	fit together, and what I had changed. My overview was accepted,
	the code was approved, and a few days later I found numerous
	errors that should have been found during the code review (or
	design review, which is sometimes difficult for modifications
	to existing code) -- and weren't, because they believed my
	explanation of how things worked, which turned out to be wrong.
	Since no members of the review team had an understanding of the 
	"big picture," at least one of them should have been charged 
	with doing a more detailed review of the "big picture" in order 
	to provide the assurances that various assumtions and/or 
	asertions were valid.

Good luck in your reviewing,

	--david
-- 
david parter					dparter@cs.wisc.edu