Goal
Everyone should strive for the code review to be short, to the point, clear with the intention of raising the quality and making the code base more consistent while being predictable in length. Not something that is making us miss our deadlines. It should be part of the core development routine. It should be clear for everyone when a code review is done, no unnecessary waiting for each other or constantly “hunting down” and nagging people to do their job. Surprisingly this is not always that evident.
Personal opinions should be aligned with team interests and a review result should be actionable. Either result in an approve / merge or clear rework request. If this is not the case the reviewer and reviewee can easily misunderstand each other.
Signs of Problems
The main one would be the “wall of text” type comments. If you have to type in a novel to justify a point it means that either it is not such a strong point or something went wrong in the PR conceptually and you are addressing a deeper underlying issue (see details in “Scope” section).
This kind of problem could also manifest in long winded discussions. As people are becoming more experienced they tend to have stronger opinions and they will not be afraid to share them. If very senior people have different preferences and they meet on a PR that could result in endless back and forth. When they start to cite opposing articles proving their point, that is when things can get out of hand and you are never sure when the review is over. For example: ”I have read these articles and watched this video and it says abstract classes are bad…. Yes but I have read this and this and it clearly says those solve everything”
Rare but huge PRs / Many follow up PRs is also a good indication that the team’s review process is not working as intended.
Possible Remediation
Agree on team level after a PR is opened by when the first round of reviews should be completed. We should be looking at days not weeks or months to complete a review cycle. If comments / threads are getting too long and out of hand a clarification meeting can save a lot of trouble. Moving from async to sync discussion tends to unblock discussions and speeds things up.
Beware of confirmation bias. If you have an opinion or preference you will be able to find articles backing your point. Usually the decision is between good or at least acceptable approaches so the team just needs to come to a conclusion regarding which one is a better fit.
It makes both the writer’s and reviewer’s life much easier if smaller PRs are opened. I think this is self-evident why. It needs somewhat more upfront investment during design to split the task in smaller chunks but that pays off in the long run.
Don’t just leave vague comments, be explicit about whether these are blockers or you are good to proceed just wanted to share some thoughts.