Efficient Code Reviews
There are big differences between code reviews and code reviews. Knowing how to do it efficiently is really crucial. We should be able to go from “LGTM, approved” to something that is not a burden but an asset.
The literature regarding PR Reviews, why they are important and how they work is quite extensive. I wouldn’t spend much time going through those here. Instead, based on my experience I would like to cover the main antipatterns and pitfalls that could derail and slow down Code Reviews in general. Naturally what is considered to be a good review (or Code Review process) differs based on the context. Maybe you are working in loosely coupled teams or an open source project driven by community or maybe time is not really a constraint or maybe you are working in an effective closely collaborating team where along with quality concerns you also have strict deadlines like on a regular enterprise project. My takes and examples are aligned with the latter case, but these are not universal facts; they are just common patterns and simple takeaways that I noticed emerging on multiple projects across multiple companies so this is just my opinion please take it as such.
Scope
Goal
The review should be targeting the lower level, closer to the code aspects of the change. The main goal is to validate that the code follows the guidelines and conventions used by the team. Basically whatever that can’t be covered with static code analysers, like structuring and naming entities in the code, opportunities to simplify or make something abstract, optimise where it is justified (beware as it is commonly known, premature optimisation for optimisation’s sake is the root of all evil), use a library where something is done “manually”, utilise newer language features, pay attention to tests on multiple levels… you get the point… you know the lower level stuff. It also should be more focused on the “how are we doing something” not the “what are we doing”. Don’t get me wrong those are really important discussions but if they are happening at the PR level that means already a lot of effort was invested in something that the team doesn’t agree with.
Signs of Problems
The discussion on the PR is not really code specific, it moves closer to architecture level e.g.: “While this change was about authentication I really think we should change our approach and utilise CQRS” or if constantly different frameworks are being proposed to solve the underlying issues.
Possible Remediation
Clear requirements if necessary a team design meeting or if that is not possible at least a high level agreement on the approach should be the starting point. This way everyone is on the same page on how the feature will be implemented so only the lower level details are missing. Yes I know it makes no sense for really small changes. I think it is ok to use common sense here :) . It is also helpful to have clear guidelines with roles and responsibilities well defined throughout the code review flow. Agree on them on a team level and document them or adopt existing ones. It will be a good reference for new joiners.
SLA
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.
Motivation
Goal
Everyone in the team should be involved in code review. It is inherently beneficial for the team but maybe not that obviously beneficial for the reviewer. People learn from each other and pick up the team coding practices via not just writing code but reviews too so they should want to do the review not just be forced to do it
As a general rule: Stop starting and start finishing. Don’t let PRs pile up.
Signs of Problems
That is super simple: Someone is not really reviewing code compared to others or he / she always has to be asked or nagged to review. If you hear “If I start reviewing code I will be late with my feature” or “I’m not that familiar with this part of the application there is no point for me to review” that means that there are issues with the individual’s motivation.
PRs are being open for a long time and starting to pile up is again a good indication that something is not right.
Possible Remediation
It needs to be clear that reviewing a PR is just as crucial and part of people’s daily work as working on their corresponding feature. While it is an interruption and can be a pain, this work contributes instantly and directly to the team’s delivery and possibly unblocks others so it can be more crucial on the team level.
You can also improve the situation if you pay attention to not to pick up new tasks instantly when finishing a PR until at least another request is not reviewed or you can introduce small / friendly competitions or bounties to boost involvement in reviews. I think bragging rights are under-appreciated.
Ownership
Goal
It should be always clear for all participating parties that who is the one with actionable responsibilities so the PR is not idling, someone is always pushing it towards merging. So the transition of ownership should be clear. Multiple owners at any given time should be avoided. If a PR is already opened it should be the team’s priority to merge it as soon as possible
Signs of Problems
Requests are staying open for a very long time so the number of concurrent open PRs are growing. You hear things like “Did you have time to take a look? It has been open for days now. What? Are you waiting for me? I thought it is still in progress / covered by others / missed the notification about it….”
Possible Remediation
Pay attention to the transition points where ownership can be lost. Agree on team level “automatic processes” around those e.g.: After PR is open, who assigns reviewers, when is the review considered to be done, who notifies who and who is merging and when…
Re-Review
Goal
A PR Re-Review should be about validating that your comments are addressed, not starting everything from scratch
Signs of Problems
As a result of the review the whole PR is being rewritten so every new review iteration is a full fledged new review. See “Scope” section for details regarding why. There could also be many re-review iterations due to constantly missed or misunderstood comments. It is also problematic if during the re-review new comments are added to code sections which were already there at the first time around
Possible Remediation
During re-review try to validate only the changes introduced to address your comments Of course there could be stuff you missed in the first iteration but if this becomes a pattern the reviews can become endless. If the situation is getting out of hand a catch up call always helps. In general, in the comments be clear about what you ask for if possible provide code snippets, that helps to get your ideas through in a way that is hard to misunderstand.
In conclusion, proper code review is not just something that has to be done for the code to be finally shipped but it is an essential tool to help the team function as an actual team and be more than just a sum of its parts while helping the team members to constantly grow and learn from each other. Just checking out others’ PRs and reading what your team members are commenting on can help people to grow and become part of a cohesive group. So it is critical to do this right and give it the attention it deserves. As always there are no silver bullets and no global truths, every situation is different so the goal is to be mindful about what you do and how when it comes to reviewing code.
