Path: utzoo!attcan!uunet!zephyr.ens.tek.com!uw-beaver!mit-eddie!bloom-beacon!eru!hagbard!sunic!mcsun!ukc!mucs!logitek!grep!frank
From: frank@grep.co.uk (Frank Wales)
Newsgroups: comp.software-eng
Subject: Re: Code inspections
Message-ID: <1991Jan30.213035.12661@grep.co.uk>
Date: 30 Jan 91 21:30:35 GMT
References: <14964@megatest.UUCP>
Reply-To: frank@grep.co.uk (Frank Wales)
Organization: Grep Limited, LEEDS, UK
Lines: 77

Here are some datapoints and opinions based on personal experience.

In article <14964@megatest.UUCP> pat@megatest.UUCP (Patrick Powers) writes:
>Boring because they are a time consuming and non-creative activity --

This implies that criticism has no part to play in creation, which I
do not believe.

>current issue of IEEE Software recommends 150 lines of code reviewed
>per man-day as a good figure.

The last project where we reviewed the code has a figure of about 10 lines
of code reviewed per minute (based on reviewing 8,500 lines of
product, which was done by the three authors in three working days).
Whoever reviews at a rate of one line per three minutes had better have
some pretty long lines of code.

>People identify closely with their creations and find criticism painful.

Criticism is in part an educational process; programmers who don't want
to learn what they do wrong, or what other people think of their work,
aren't the kind I'd depend on to produce quality products.  If the people
involved in the project give a damn about doing their best, they will
quickly come to enjoy the code review process as a learning experience;
maybe it will also deepen the respect they have for each other's work,
which is a valuable team-building tactic that good managers can exploit.

>Not only that, but your average programmer was very likely attracted to
>programming in order to avoid social interaction and to create
>something under his/her personal control without anyone else watching.
>[other vaguely insulting generalisations deleted]

It seems to me that maybe you're working with too many low-grade
code-grinders.  Hire some actual software professionals in their place.

>In order to reduce these problems the following has been suggested:
>1) The author not be present at the inspection

Bad idea.  Very bad.  To apply a courtroom metaphor, it would be like
denying the accused the ability to be present, and give his own version
of the events.  At the code reviews I run, the author of the code is
the reader of the code.  It is the responsibility of the others present
to convince the author that code is poor, where this is appropriate.
The principal software engineer is responsible for deciding issues of
style, and the project manager has final say on what goes in the actual
product.  Interruptions are not allowed to enter the code review room,
while disagreements are not allowed to leave it; they must be resolved
before people go back to writing software.  

>2) Only errors are communicated to the author.  No criticism of style allowed.

Also a bad idea.  If I can't understand something, I don't care if it works,
because my confidence in it is reduced.  Style may not matter at run-time,
but it certainly matters at read-time and think-time.  If you go under a bus,
I don't want to have to hire a medium to figure out how to fix your code.

Other random comments: I use scheduled code reviews as a place to
resolve implementation details which were decided upon on the fly by
a programmer when the design documents or other colleagues could not
give an authoritative answer when the code was written.  In this regard,
they are a valuable way of helping to analyse the many small-but-important
decisions that get taken during software construction.  A second
important use is to allow each programmer to become familiar with the
work of his colleagues, which is a combined educational and confidence-
building exercise.  And a third use is to allow programmers to teach each
other tricks and techniques that have never been explicitly communicated or
written down anywhere else but in the code itself.  Hold the whole review
off-site if you can, in a quiet room with plenty of paper, pencils, 
a flipchart and {black|white}board, coffee, Pepsi and lots of donuts.

I believe code review is a valuable tool, and avoiding it for what amounts
to egotistical reasons serves neither the developers nor the customer.
[FYI: I usually review my own code during a project, even if I am
 the sole author -- just like Oscar Wilde, I enjoy a good read :-).]
--
Frank Wales, Grep Limited,             [frank@grep.co.uk<->uunet!grep!frank]
Kirkfields Business Centre, Kirk Lane, LEEDS, UK, LS19 7LX. (+44) 532 500303