Re: [Audacity-devel] throwing out code (was Re: Vocoder, Nyquist, decimals and commas)

[Dan H. <da...@go...>]

On Wed, Sep 02, 2009 at 02:38:35PM -0700, Vaughan Johnson wrote:
> This throwing away code that's not understood is a really bad thing. 
> It's happened to me multiple times, from seasoned team members as well 
> as noobs, and it's easy to miss at the time, so a pain to fix later. 
> I've gotten so I at least skim the -cvs entry of most commits, 
> especially on code I've primarily worked on, kind of a one-person code 
> review. But it's time consuming and would be partly unnecessary if 
> people would understand the changes they're making. We could do more 
> elaborate code reviews, but I don't think we have time for it.
> 
> How can we make this happen less often?
> 
> - Vaughan
> 

I agree that this is something to think about. The changes I have made
haven't involved much throwing-away of code, but I have managed to break
things once or twice due to not fully understanding the subtleties of
how what I was changing affected the program as a whole - this despite
my trying to be very careful *not* to break things.

A couple of suggestions:

1) Copious commenting when adding anything whose purpose may not be
obvious to somebody that hasn't worked on that area of the code before.
In particular, if fixing a problem whose cause wasn't immediately
apparent, an explanatory comment would ensure that anybody working on
that code later on can take that potential problem into account (and so
won't unfix it). Generally this already seems to happen, but you can't
have too much of a good thing.

2) Automated testing. This could be done (to some extent) with the
script system. There could be a suite of basic test scripts which check
for generally sane behaviour and for specific known problems that have
occurred at any stage. Then when somebody wants to change or remove code
they can run the tests to check that nothing broke. The script system
would need expanding quite a bit to get anything approaching
comprehensive coverage, and the test suite is a bit tedious to build up,
but if it catches bugs early then it'll probably save work in the long
run.

Of course it ultimately falls to the person making a change to be sure
that they aren't breaking anything, but the more of the code they need
to understand in order to do this, the more likely they are to overlook
something. So this is also an argument for breaking code up into
smaller, more cohesive modules where possible, so that changes in one
place don't have unintended consequences elsewhere.

Dan