I’m debugging some of the echo cancellation code in libspeex because Teamwork uses it as a low-bandwidth voice chat codec.

Take a look at this snippet:

int N = st->window_size;

for (i=0;i<N;i++)

   st->y[i] = MULT16_16_Q15(st->window[i],st->last_y[i]);

//Compute power spectrum of the echo

spx_fft(st->fft_table, st->y, st->Y)

power_spectrum(st->Y, residual_echo, N);

What do I hate about this? Let me count the ways. Does putting st->window_size into N improve readability? No, it actually obscures things, and those are the only two places it’s used in the function. There’s also no way st->window_size could have been changed between the various places N is used because none of the subfunctions get the pointer they would need to modify the structure. Even if they did, “N” is a dumb name for it - why not “orig_window_size” or something?

Also, look at the call to spx_fft. The second and third arguments are st->y and st->Y. That’s right, the structure at st has two members named “y”, separated only by their case. In my view, there is never a good reason to do something like that.

As a former/recovering OSS zealot I instinctively am inclined to prefer open source software over commercial, but man… this kind of stuff would get caught in a round one code review in a for-profit enterprise. You would never ship this in a commercial SDK.

So maybe I’ve been spoiled by the obsessively policed guts of ipOS.

(And yes, special formatting for code would be great if I plan to post more rants like this in the future. I’ll ask my HTML/CSS slave for some soon.) EDIT: Apparently I already had it. Thanks :)