4 ms·

> Establish productive code review rules. E.g. always go from big picture to details, otherwise you'll waste time on details that you end up throwing away. Wel

> Establish productive code review rules. E.g. always go from big picture to details, otherwise you'll waste time on details that you end up throwing away. Well, that's a nice rule. Another one I heard people say is that code reviews are not for arguing about architecture or design decisions. If we take those two rules together, code reviews are good neither for discussing the details nor for discussing the high-level picture. Which in my experience is about right.

quantumhobbit 8y ago
Right. Code Review should be about catching mistakes. The high level design is already known, and the low level details are checked by a linter. Code Review just makes sure that you didn't accidentally introduce a bug and that the code has appropriate tests.
- FlipperBucket 8y ago
  > Code Review should be about catching mistakes. Overly-complicated code is a mistake. Maintainability matters almost as much as a passing unit test. You want your code reviews to catch meaningful things, but they often end up being about things like this (fixing inefficient small bits of code). e.g. function isDry(weather) { if (typesUtil.isSunny(weather) || (!typesUtil.isSunny(weather) && typeof weather.hasSnow === 'boolean' && !weather.hasSnow)) { return true; } return false; } vs function isDry(weather) { return typesUtil.isSunny(weather) || (typeof weather.hasSnow == ‘boolean’ && !weather.hasSnow); } vs function isDry(weather) var hasSnow = false; if(typeof weather.hasSnow === 'boolean') { hasSnow = weather.hasSnow; } return (typesUtil.isSunny(weather) || !hasSnow); }
- ahartmetz 8y ago
  When you are dealing with an open source project, people will just show up with small and sometimes large changes. In that case, you may need to review design. There is also "minor" stuff a linter won't catch, such as bad naming. IMO naming is actually quite important.

quantumhobbit 8y ago

Right. Code Review should be about catching mistakes. The high level design is already known, and the low level details are checked by a linter. Code Review just makes sure that you didn't accidentally introduce a bug and that the code has appropriate tests.

FlipperBucket 8y ago
> Code Review should be about catching mistakes. Overly-complicated code is a mistake. Maintainability matters almost as much as a passing unit test. You want your code reviews to catch meaningful things, but they often end up being about things like this (fixing inefficient small bits of code). e.g. function isDry(weather) { if (typesUtil.isSunny(weather) || (!typesUtil.isSunny(weather) && typeof weather.hasSnow === 'boolean' && !weather.hasSnow)) { return true; } return false; } vs function isDry(weather) { return typesUtil.isSunny(weather) || (typeof weather.hasSnow == ‘boolean’ && !weather.hasSnow); } vs function isDry(weather) var hasSnow = false; if(typeof weather.hasSnow === 'boolean') { hasSnow = weather.hasSnow; } return (typesUtil.isSunny(weather) || !hasSnow); }
ahartmetz 8y ago
When you are dealing with an open source project, people will just show up with small and sometimes large changes. In that case, you may need to review design. There is also "minor" stuff a linter won't catch, such as bad naming. IMO naming is actually quite important.