|
The approach that I've seen work is to have high standards for code, but communicate the expectations upfront and make sure that code reviews are grounded in those standards (for new people, this means over-communicating, but it becomes more intuitive once you've worked together for a bit). And whatever you do, be constructive, not rude. If your codebase is going to be maintained for a long time, letting sloppy or inconsistent code in is going to make it much harder to maintain or evolve in future, so just lowering standards to make people feel better doesn't work. It's totally reasonable to hold people to a high standard if they're submitting code that others will have to maintain. A lot of the problem is if people start off with the expectation that their code should be merged with minimal modifications, they're going to be upset when a code reviewer seems to be moving the goalposts on them. Also if the reviewers comments are arbitrary (or seem arbitrary) because they're not obviously grounded in consistent standards. I've seen a project go through the process of tightening up code review standards and it was initially painful (a lot of people used to the old way feel like they can't move fast enough, etc) but ultimately worked out and resulted in people shipping working features at higher velocity with far less time spent on rework and maintaining overly-complex and buggy code. There's also way less conflict in onboarding new developers because standards have been made clear in advance and reviewers are expected to be respectful and coach newcomers through the review process. The hard part, I've found, is working with developers who can't or won't get code to the expected standard, even with coaching and detailed feedback. As in, repeatedly making the same simple mistakes pointed out by reviewers and failing to understand and address relatively straightforward feedback.
I'm not sure if there's a solution to this, beyond minimizing the damage and trying to move the person to a role that they're more suited to. In one such instance the person was, in retrospect, a complete liability - they switched to a different team with looser code review, got a "bugfix" committed with some obvious errors that, if not luckily caught by our team, could have potentially had disastrous consequences for some customers, then left the company. Maybe the lesson is that high code review standards contain the damage from such people. Edit: I also think this requires code reviewers to approach the reviews with the attitude of "what do we have to do to get this merged without compromising our standards?", not a desire to tear down the person or block the code change. |