Path: utzoo!attcan!utgpu!jarvis.csri.toronto.edu!rutgers!cs.utexas.edu!uunet!yale!leichter
From: leichter@CS.YALE.EDU (Jerry Leichter)
Newsgroups: comp.software-eng
Subject: Re: code reviews
Message-ID: <64324@yale-celray.yale.UUCP>
Date: 21 Jun 89 20:24:10 GMT
Sender: root@yale.UUCP
Organization: Yale Computer Science Department, New Haven, Connecticut, USA
Lines: 72
X-from: leichter@CS.YALE.EDU (Jerry Leichter (LEICHTER-JERRY@CS.YALE.EDU))

Many years ago, in wilder times, I and two friends spent a fair amount of
time, ahem, breaking into our college computer.  We generally worked on hacks
cooperatively.

One day, one of us developed a very simple program.  He decided that it was
simple enough, and so clearly correct, that he could just try it out.

Well, needless to say, the program had a completely obvious bug which one of
us spotted instantly on looking at the code.  The effect of the bug was to
cause a system crash leaving fairly obvious pointers to the culprit.  The
author had run the thing twice...the first crash "must have been a coinci-
dence".  (Back in those days, the average time between crashes was a couple
of hours, so this was not a completely absurd thought.)  We were VERY lucky
not to get caught.

The rule we learned from this was:  Never hack alone.  From there on out, none
of us would run a new hack program unless one of the others had checked it out
first.  We had no further incidents of this particular sort.

Years later, I worked for an organization with a policy of code reviews.  We
actually had multiple levels of review.  There were informal design reviews.
In practice, most people hated those and they were not very useful - they
tended to bring out the worst in those people who insisted on doing things in
particular standard ways which were usually irrelevant.

On the other hand, we also had a very strict CODE review policy - we called it
"releasing".  Any particular implementation project, whether new code or a
patch to existing code, required two people, a coder and a releaser.  The
coder wrote and tested the code, and handed it to the releaser.  The releaser
was responsible for checking that the code met its specs, was written to our
internal standards (which were not very strict; what they required was all
very simple and reasonable), was reasonably documented, and so on.  More gene-
rally, the releaser was expected to understand the code as well as the coder
did.  If he found problems, he could at his disgression fix them himself or
require the coder to fix them.

One very important element of this system was that there were no separate
groups of coders and releasers - everyone did both jobs.  If you released my
code today, I might release yours tomorrow.  This avoided the typical con-
flicts you often see between development and QA groups.  (Yes, it can have
disadvantages, too.)  It also provided some check to ensure that people didn't
try to cut corners on either job - though of course that happened.  (One per-
son who worked for me for a while viewed his releaser's role as one of finding
and fixing all the bugs.  He was known to hand over code that wouldn't com-
pile.  Needless to say, he was seen as a problem....)  However, in my experi-
ence the system worked quite well, and the code quality that resulted was
quite high.  (Sorry, I can't quantify any of that...but I'd certainly use such
a system again.)

What did doing this cost?  It's hard to be exact because people varied.  A
good manager had to adjust for the individuals involved.  For most people, I'd
estimate release time at about half of pure coding time - where pure coding
time did NOT include design or debugging.  For the "problem" person I mention-
ed above, I'd have to drop coding time and increase debugging and (even more
so) release time; the total would end up somewhat higher.  But that rarely
happened.  Since except for small, isolated projects pure coding time wasn't
likely to be more than a third or so of the total time, release time might
add about 15% to the total - and it could often be used to fill in otherwise
dead spots on schedules, so in real terms its contribution to total completion
time might be quite small.  On the other hand, it also provided you with a
second expert on the particular piece of code involved.  The training to do
that would have cost you some time, too.

The group in which I saw this done was not very large (50 people perhaps in
total) and was probably unusually talented, since management made it a point
to hire the brightest people they could, regardless of experience.  How well
this kind of approach would generalize to other groups, I can't really say.
(I also believe that the group eventually stopped doing this when pressure
from above to get stuff done faster became much greater.  There's never time
to do it right, but always time to do it again....)

							-- Jerry