[KiwiCoder]

Code review ranks just behind design review in value (cost/time savings). In fact code reviews are so beneficial that if I was working on a solo project I would either pay for them to be done or review the code myself after a suitable cooling off period, depending on what I was working on.

On the other hand, I have also witnessed sloppy, lazy code reviews that catch nothing except the occasional typo. This amounts to an unjustifiable waste of time. Fortunately, it is easy to tell a good code reviews from bad by tracking defect discovery and digging into review comments as needed.

One thing that code review catches that nothing else does is code that is poorly written but functional (i.e. passing tests).

The example I always trot out is

    for ( int i=0 ; i < this.MyControl.TabPages.Count ; i++ )
    {
       this.MyControl.TabPages.Remove ( this.MyControl.TabPages[i] );
       i--;
    }

This code works according to spec, passes all the tests, but is bordering on unmaintainable. At best it's a WTF.

(Written up here: http://cvmountain.com/2011/09/whats-wrong-with-this-code-rea...)

[swanson]

I find that one of the causes for wildly different levels of code review (and value derived from them) is a lack of training. There is a real lack of materials for explaining how to do a code review, how to do deal with the human aspect of giving feedback, what is/is not valuable to talk about (arguing over tabs vs spaces should not happen in a code review). Most of my experiences have involved a trial-by-fire process - new engineers receive a few code reviews from more experienced people and that is your "training".

[KiwiCoder]

I agree - and your comment gives me an idea for a series of posts on this exact subject.

[swanson]

Great - would love to read :)

http://exercism.io/help/how-to-nitpick is a good resource as well.

[redthrowaway]

That code snippet is a matryoshka doll of brainfuck.

[ShawnBird]

The first thing I thought of is, even in a hypothetical world without a Clear() method or any means of adding one, why use a for loop instead of a while loop? And if you are set on a for loop, why not simply set the first count to a variable? It is bad on multiple levels.

[redthrowaway]

    <list of dead items>

"I need to delete every item"

    for (i = 0; i < length; i++) {
        remove item[i];
    }

"Shit, that didn't work. Why isn't it deleting everything?

    for (i = 0; i < length; i++) {
        printf("%d", item[i]); 
        remove item[i];
    }

"Huh, it's removing every other item."

    for (i = 0; i < length; i++) {
        printf("%d, %d", item[i], i); 
        remove item[i];
    }

"Weird, it's removing an item, skipping an item, then removing the next one."

    for (i = 0; i < length; i++) {
        printf("%d, %d", item[i], i); 
        remove item[i];
        i--;
    }

"Well shit, that seemed to work."

    for (i = 0; i < length; i++) {
        remove item[i];
        i--;
    }

"Ship it."