Re: code reviews (was: Re: linked list library)

[John Darrington]

On Sat, Jul 01, 2006 at 01:23:41PM -0700, Ben Pfaff wrote:
     Let me elaborate.  I've watched three different systems of code
     reviews in some detail:
     
             * Linux kernel, where it's meant primarily to ensure that
               code is coming from an approved source.  There are
               "magic keywords" but the reviews themselves are
               informal.  Fairly often there are no reviews at all, at
               which time the fate of the patch is up to Andrew Morton
               or to Linus.
     
             * Autoconf and Gnulib, where proposed patches are sent to
               a mailing list and comments are sent as replies.
               Reviews are informal and those proposing the changes
               also seem to be responsible for deciding how to respond
               to them, by dropping the patch or modifying it or
               checking it in unchanged.  When there are no replies,
               usually it gets checked in without discussion.
     
             * A medium-size company that makes proprietary,
               commercial software, which has expanded from 150
               developers to over 1000 while I've been associated with
               them.  Again, here you send your proposed check-in to
               an appropriate mailing list (sometimes more than one)
               with an explanation.  Reviews are informal and posted
               to the list.  If there are no reviews, you re-post the
               patch or post it more widely or complain loudly that
               you need a review.  Then you fix the problems or
               discuss them until you reach consensus that it's
               unnecessary, and check in the result.

Have you looked at the way aegis works?  One of its features is that
it can require code reviews before anything gets checked in.  The
review policy is configureable according to the enterprise's needs.

     In my experience, this is really, really useful.  From what I've
     seen, code that gets reviewed ends up better.  Just from posting
     this linked list change, your suggestions enabled me to make
     improvements in code that I had already gone over with a
     fine-toothed comb.
     
     To recap: I have seen good things come from code reviews, and
     I've had good experiences myself, so I'm going to try posting
     most of my changes here (or on savannah) in advance of checking
     them in, in hopes of getting them reviewed.  And I'm going to
     encourage others (I guess that's just you and Jason) to try it
     also.  Obviously it doesn't always make sense, and when there's
     no one competent to review it (e.g. GUI-only code) are a good
     example of that.  If it doesn't work out, maybe I'll stop.  But
     I'm pretty optimistic that it'll be helpful.
     
     John Darrington <address@hidden> writes:
     
     > 1.  How do I distinguish between a trivial and nontrivial change ?
     
     Fixing a spelling error in an error message or a comment is
     trivial.  Rewriting the entire lexical analyzer is non-trivial.
     Anything in between is completely your judgment call.

Fine.
     
     > The "code reviews" conducted at any commercial organisations with
     > which I've been involved, have been a joke.   Nobody had the
     > time/competance/inclination to do them.  Consequently, if there were
     > done at all, they were simply a beaurocratic exercise.
     
     It sounds like your own experiences with code reviews have been
     the opposite of mine.  I'm sorry to hear that; they can be really
     good things.

I didn't mean to sound quite so negative.  I agree that code reviews
can help.  But I've seen so many software houses, where (usually to
justify a claim of compliance to ISO-9001 and/or CFR 21.11) the
directive is given "henceforth all software changes must be reviewed
(and recorded in form XYZ)".   It didn't improve software quality and
just became an administrative burden. 

But with most of those projects, the directive was given several years
after the code had become an unmanageable mess.  The PSPP code does
not fall into that category so it's a good idea to start doing them
before it degenerates to that state.

I like the idea that author of the patch gets to make the decision to
review/not to review, and to accept or decline suggestions from the
reviews.  I think it's also a good idea to suggest that patches which
have not been reviewed after say 7 days can be automatically committed
at the author's discretion.

J'


-- 
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://pgp.mit.edu or any PGP keyserver for public key.

pgpka2svLXB_5.pgp
Description: PGP signature