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 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 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.
Besides working on their tasks, the role of a senior or lead engineer should be one where junior and mid-level engineers can both depend on you as well as receive your trust. When you're always handholding them, question your own practices first.
If you are always the one holding the gavel, giving the final say to everything, all you are doing is lording over others. When you are doing that, you are not only stifling the career development of others, but you are doing more work than you need to.
Instead, you should develop your team to be self-regulating so that you don't always have to give approval to every decision. If everyone on your team from the bottom up understands what's expected of them and are given the trust to make decisions without your presence, this gives them a level of responsibility and ownership over the codebase. When engineers take that kind of care in the work that they do, that means you don't need to be looking over their shoulder.
When it comes to code reviews, my suggestion is to require 2 approvals in order for code changes to be merged, and for those reviewers to be random team members every time. They could be you or even juniors engineers. Everyone can experience the position of examining other people's code and provide feedback that's taken just as seriously as anyone else's.
Since you won't always be a part of the reviewing process, others will be forced to be more engaged in the process, since if they don't take it seriously enough, they will be responsible for any issues that come up.
Mistakes will be made, but I've seen mistakes occur even when code has passed review. If you can afford some forgiveness, engineers will learn from their mistakes and processes can be changed to prevent certain mistakes from happening again. This is how people grow in their roles.