- managers would swoop in and leave incorrect criticisms. Asserting plainly false things and in one case reviewing python code as though it was JavaScript.
- devs would make conflicting requirements for improvement. Dev A demands change A. Dev B wants change B. A and B are incompatible and neither dev will back down. Management of course refused to help settle the argument.
- change requests completely unrelated to the PR being reviewed would get crammed into the PR. So every small code change could balloon into weeks on refactoring work.
- Often times code review comments were made without ever reading the code being reviewed. Such as a comment of “Why aren’t you using library X?” sandwiched in between invocations of library X.
The culture was clearly rewarding people for shitting on others work instead of doing any work themselves. Getting even the simplest code changes past required endless meetings explaining the most basic facts about computers.
So glad to be gone from there.
This was a problem in my previous job. Options I thought of:
1. Get a moderator. This is what we did. He did not just settle disputes - he did more than that. But it's good to have at least this role.
2. Have a total of 4 reviewers review the disputed section. If there is a majority, change it. If it's a 2-2 split, just leave it as is. (Team did not accept this).
3. Disputed code (whether changed or not) should have comments saying this vs the alternative was discussed in a code review. Why? Because if there isn't consensus, over time, people are going to keep changing those lines of code back and forth (some developer who was not involved in the review will decide to refactor - rinse and repeat).
In the first instance, I suspect there is blame culture or otherwise the developers previously had that experience and thus carried the baggage to the new job. I believe this is how you can get pull requests that either never get approved or merged (nobody wants their name attached to it in case they get called out). When it comes to not having time to review at all, I suspect that's the classic "I'm too busy" excuse, where if the whole team is too busy working independently then they can't ever expect to get anything to production without a serious effort each time.
In that instance the code review and lack of productivity is just a symptom of a lack of team dynamic.
The latter point is weird because I've had that and I didn't enjoy it at all. I could never tell if the approval was because the work was good or because it was a rubber stamp operation. Especially based on other feedback it felt like I was being put on a pedestal and I hated that I could basically do no wrong, even when I did because we merged and then I had a gut feeling that I fucked something up.
I'm not sure accountability would fix that particular manifestation of the problem.
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.
I have a recollection of seeing a study once that found the best balance point of review time vs dev time but haven't found it again. I recall it being somewhere around 20-30% but now that I can't find it I'm questioning my recall.
So assuming the same payoff benefit (big if, but let's go with it) between PR style reviewing and more formal inspections, every engineer should be spending around four or five hours a week performing reviews to hit the sweet spot.
Even with that structure I found it challenging to develop a good reviewing style that picked up the important stuff without getting bogged down in silly details that lead to arguments. Reading John Ousterhout's "A Philosophy of Software Design" was really helpful. He focuses on module decomposition, quality of public interfaces, and documentation among other things.
So I won't submit to a committee review before my code is accepted. Its a recipe for anguish and failure.
Module decomposition is usually something I try to pick off in design, but when accepting pull requests from outside it's a critical consideration. We're not blocking creativity but trying to ensure that the code base not only remains high quality and that the design morphs appropriately over time as we meet new requirements. By making this a review point you can help create a culture where engineers think about it before submitting. That in turn makes it easier to justify dates to managers.
* Assign enough time for code reviews - otherwise developers will just care about their own code submissions and let reviews rot.
* 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.
* Make reviewers co-responsible for the quality of code reviewed, otherwise instant acceptance for everything becomes attractive.