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.
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;
}
function isDry(weather) {
return typesUtil.isSunny(weather) ||
(typeof weather.hasSnow == ‘boolean’ && !weather.hasSnow);
}
function isDry(weather)
var hasSnow = false;
if(typeof weather.hasSnow === 'boolean') {
hasSnow = weather.hasSnow;
}
return (typesUtil.isSunny(weather) || !hasSnow);
}There is also "minor" stuff a linter won't catch, such as bad naming. IMO naming is actually quite important.