As the harshest prisons are those of our own making, code reviews suck only if we make them suck.
In principle, I love code reviews. We should all learn to be open to constructive criticism. Yet, there are is something about the typical code review process that frustrates me, and I imagine many other engineers.
It's not about when reviewers are jerks, which is actually rare. I can deal with jerks. The core problem I see with typical code reviewing practices stems from good intent. Occasionally from lack of competence, granted, but generally not by malice.
Code review sucks when it prevents us from getting work done. This happens when we focus too heavily on a perception of quality while putting aside efficiency and pragmatism.
Imagine an engineer submits pull request, for which other team members add their reviews. Among the comments are concerns around a contingency the engineer didn't consider, consistency around function naming, and other minor concerns.
The engineer takes time to addresses those issues, and submits the PR for approval. This results in everyone chiming again in with more criticisms that weren't in the first wave of comments, all of which sound reasonable. Since this engineer isn't in a senior role, they feel pressure to treat everyone's opinion with equal weight.
Code review sucks when it prevents us from getting work done.
So they make changes and submit the pull request for review yet again, which becomes subject to even more discussion. This time it's around whether two of the functions the engineer wrote could be combined into one. As this idea sounds reasonable, the senior developers opt not to have the PR merged(again) until the suggested change is made.
Thus, despite the code having achieving the acceptance criteria and passing through automated testing, the job can't be considered done until the review cycle has settled.
I see this happen all the time. Yes, I'm writing this entry because it's happened to me on more than one occasion, but I've seen others subjected to such inquisitions on more than one occasion both in a professional setting and in open-source.
What bugs me about this kind code review, which focuses heavily on consistent conformity, is that it needlessly prevents features from getting shipped. As humans, it's psychologically painful because we need to feel like we are getting work done. Spending too much time on the same piece of work, especially in the continuing quest for perfection, is stagnation when there's other work to get done.
When a code review drags on, whatever the reason, engineers get demoralized. The more open-ended a unit of work becomes, the less motivated we are to do it because the finish line is unclear or way off in the distance, or even moving further away from us. No one wants to be a donkey chasing a carrot on a stick!
To make matters worse, code reviews can quickly turn into backseat coding where engineers instruct other engineer on exactly what code they should be writing. This can be seen as insulting because it turns the engineer into a glorified proxy between their keyboard and the other developers. If you don't like it when people backseat drive, why would you want to tolerate backseat coding?
It's one thing if there is a serious architecture issue that needs to be addressed before code ever gets merged. Code that's incomprehensible also needs to face hard questions. Do some decisions require further deliberation before being committed to? Of course.
When the code is technically valid and does the job asked of it, that's another thing, even if it isn't the most "elegant" or doesn't follow some of the team guidelines. Is making everything "perfect" out of the starting gate really better than shipping features and allowing engineers to move on to other tickets?
If your answer is yes, the rest of this entry isn't for you. On the other hand, if you believe that perfection can be addressed incrementally, there is a simple rule your team can adopt to make sure perfection doesn't become a roadblock to progress.
Want to get team members more engaged in code review and remove blockers to progress? There's a simple change you can make to your code review process that will make this possible.
During a code review, if nobody can describe, in an objective way, why a piece of code needs to be revised, the current work should be accepted(i.e. the pull request should be merged) and a new ticket should be created to encapsulate effort that will go toward refactoring.
If you follow a sprint cycle, the new ticket should be added to the following sprint, rather than the current one. This forces everyone to not focus too much time on any single ticket, and allows engineers to move on to other tasks while feeling good that they've completed something on time.