4 ms·

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

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-really/ http://cvmountain.com/2011/09/whats-wrong-with-this-code-rea...)

swanson 13y ago
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 13y ago
  I agree - and your comment gives me an idea for a series of posts on this exact subject.
  - swanson 13y ago
    Great - would love to read :) http://exercism.io/help/how-to-nitpick http://exercism.io/help/how-to-nitpick is a good resource as well.
redthrowaway 13y ago
That code snippet is a matryoshka doll of brainfuck.
- ShawnBird 13y ago
  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 13y ago
    <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."

swanson 13y ago

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 13y ago
I agree - and your comment gives me an idea for a series of posts on this exact subject.
- swanson 13y ago
  Great - would love to read :) http://exercism.io/help/how-to-nitpick http://exercism.io/help/how-to-nitpick is a good resource as well.

redthrowaway 13y ago

That code snippet is a matryoshka doll of brainfuck.

ShawnBird 13y ago
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 13y ago
  <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."