code reviews (was: Re: linked list library)

[Ben Pfaff]

> On Thu, Jun 29, 2006 at 09:23:18PM -0700, Ben Pfaff wrote:
>      By the way, I've decided that it's time to start getting code
>      reviews of nontrivial changes to PSPP, as is done in some other
>      GNU projects (not to mention commercial development), so that's
>      why I'm posting these patches.

I wrote this hastily.  I should not have sounded like I was
giving instructions; that was not my intention.

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.

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.

> [...]

> 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.
-- 
"It was then I realized how dire my medical situation was.  Here I was,
 a network admin, unable to leave, and here was someone with a broken
 network.  And they didn't ask me to fix it.  They didn't even try to
 casually pry a hint out of me." --Ryan Tucker in the Monastery