A lot has already been said about code review, but there are still points that can be debated. We recently discussed them at one of our internal IT gatherings.
Code review alone is not a panacea. It can be implemented to benefit the project, but this advantage is not guaranteed. There are numerous examples where code reviews have been detrimental. Let’s discuss where the pitfalls lie and how to conduct a more effective code review.
It is pointless to conduct a code review when there is no adequate problem statement
Let’s start with the basics. Code review becomes a delayed decision if the task was originally set incompletely, incorrectly, or without proper input.
A fairly common scenario is when a new developer on the team writes code and during the review, they are told, “This is not possible, we don’t do this” (for example, not using REST at all). This puts everyone in an uncomfortable position. The developer has put in the effort, but the results of their work are not needed. The reviewer, although understanding this, will insist on their own perspective because they are responsible for the project.
The question of who should communicate to the new person that REST is not used in this situation is a philosophical one. It could be the responsibility of the person onboarding or the task of the team leader. Some companies believe it should be the initiative of the newcomer themselves. However, in most cases, responsibility is shared among all participants in the process. Therefore, it is much better to document all non-obvious points regarding the project and the approaches being used.
In any task, it is crucial to discuss the requirements beforehand, even before writing a single line of code. It is good practice to do this during the planning phase.
However, in reality, things don’t always go as planned. Sometimes, during the discussion, a solution is proposed and agreed upon by everyone, and the developer implements it accordingly. But during the code review, the responsible person suggests that the problem should have been solved differently from the beginning. This raises the issue of improving the code with future refactoring in mind. Whether to consider a new idea during the review phase depends on the specific details of the project organization.
Code review must be fast
It is not advisable to delay code reviews for a week or more. When the review process is prolonged, the code takes a long time to reach production, and developers become tired of constantly revisiting tasks that were submitted many days ago. Valuable time is spent on re-immersing in the context.
Ideally, a code review should be conducted as soon as a pull request is submitted. A delay of an hour is considered acceptable, but half a day is not ideal because the developer has to wait during this time and needs to keep the task’s context in mind.
A good practice is to monitor whether the assigned reviewer has reviewed the code, and if not, send additional notifications to remind them. It is possible that they may have been momentarily distracted.
In general, it is beneficial to have the same individuals who write the project code participate in the review process. As they wait for their own pull requests to be reviewed, they understand the importance of not delaying their colleagues in a similar situation.
The reviewer must understand that his task is not about subjective perfectionism
When reviewing someone else’s code, there are often thoughts about how it could be done differently or made more aesthetically pleasing. However, the reviewer needs to restrict the number of comments provided. The primary focus should be on working code that meets the project requirements, meaning the desire for beauty should have reasonable boundaries.
It is common practice to label certain comments as minor (for example, using the “NIT” prefix as recommended in Google’s manual, found at https://google.github.io/eng-practices/review/reviewer/looking-for.html). Such a prefix indicates that the idea is worth conveying to the developer, but the reviewer does not insist on its implementation. In other words, the code can be improved, but this comment does not block the pull request. Depending on the team’s culture, the reviewer may not even wait for a response to these types of comments.
There are situations where it is challenging to distinguish between matters of personal preference and practical suggestions. In such cases, it is useful to ask higher-level questions and consider the underlying reasons for the code’s current state.
For instance, when discussing acceptable code complexity, we should consider writing code not only for the machine’s understanding but also for the people who will later debug and develop it. Ultimately, it depends on finding a tradeoff between how quickly features need to be implemented (i.e., the time spent perfecting a specific piece of code) and the importance of simplifying it. Perhaps there is no need to revisit a particular piece of code at all? In that case, investing more time in refining it may not yield any significant changes.
No focus — no code review
You can distribute pull requests among reviewers, but if a person fundamentally doesn’t want to participate, they are likely to simply agree to the code without delving into its contents. This is despite the fact that code review itself addresses the question of whether the pull request effectively solves the problem.
It is impossible to force someone to carefully read the code.
Addressing the broader issue of motivation is beyond the scope of this article. However, distributing responsibility among multiple reviewers can sometimes help mitigate partial attention or distractions. For example, assigning both a team leader and a developer as reviewers for the same pull request. The developer may assume that the team leader, who will be overseeing their work, will identify any issues, allowing them to skim through the code. This situation is typically reflected in the quality and quantity of comments from both reviewers. Similarly, responsibility can be shared if the project maintainer and an external individual simultaneously review the code.
Unfortunately, there is no universal method for dealing with distractions. This question ultimately falls on the conscience of those conducting the review.
Code review is about the interactions in the team
Code review is an important tool of team communication and sometimes it serves as training for new team members.
It’s not a good practice when code review comments are mechanically corrected without consideration. This only improves the code being discussed, but not the project as a whole. While there are cases where certain code issues need to be addressed directly, there are also aspects that should be discussed to develop better solutions for the future, benefiting the project. When code review becomes solely about mechanical corrections, this important aspect is lost.
This often happens when comments are unfriendly. Some individuals take pride in conducting harsh code reviews, with every comment sounding like an attack. This is not beneficial. Even if there are aspects you don’t like in the code, it’s better to choose more neutral wording.
Similarly, some developers react poorly to even mild comments. Instead of making the code clearer in response to complexity concerns, they respond to the reviewer with insults. Both of these reactions do not contribute to the quality of the code review. They either lead to conflicts or result in no discussion at all, as one party walks away to avoid further stress.
Sometimes, there can be an overwhelming number of comments. A colleague of mine shared an experience where 30 individuals, unfamiliar with the project, flooded a pull request with irrelevant comments. Thus, the quantity of comments had nothing to do with the code quality. Flooding comments also happen when the project is lacking proper task descriptions or style guidelines.
Advice for reviewers: It’s good practice to read the entire pull request before bombarding the developer with questions. Platforms like GitLab have a useful feature that allows you to queue comments without immediately sending them. This helps reduce the number of comments and avoids the need to retract questions later, apologizing for the wording.
Advice for developers: Not all reviewer remarks should be taken personally or with nervousness. For instance, if the reviewer mentions that the code is too complex but cannot be simplified, it’s important to remember the ultimate goal of the process. If the reviewer is concerned about the code’s understanding in the future, comments within the code itself (rather than only in the pull request discussions) might address those concerns.
Cross-team code reviews cost more; you need to understand what is corrected and why
Code review itself can be costly. A good review helps maintain code quality, which is valuable. However, a bad review consumes a lot of people’s time without benefiting the project — it’s simply wasted company resources.
We mentioned earlier the example of 30 individuals who were not familiar with the project conducting a code review. While there are situations where cross-team code reviews bring fresh perspectives to a pull request, this isn’t always the case. If developers lack an understanding of what each person can contribute to the project, involving 30 individuals who are not immersed in the task is a waste of their time.
The issue is not just that someone unfamiliar with the code is reviewing it. Consider a colleague who wants to provide a high-quality review, examining not only code cleanliness but also implementation logic. In this scenario, they would have to invest a significant amount of time, even for a small pull request, to delve into the context. They would need to read documentation, understand the module’s purpose, and grasp the intended changes. This time is paid at the developer’s expense.
If there is no specific need to involve all 30 colleagues in reviewing the code, it is more logical to distribute the responsibility differently. Assign reviewers based on their qualifications and familiarity with the project, and add approvers as a safety net. It may also be reasonable to consider factors like the size of the pull request in terms of lines of code and each colleague’s workload when assigning code review tasks. The goal is to ensure the team is satisfied and time is utilized effectively.
Established practices of code review are good, but not for juniors
In the previous discussion, we covered various aspects of code review and proposed some working practices. However, it’s important to note that there is no one-size-fits-all approach.
With more experienced colleagues, it is recommended to ask questions rather than providing your own solution. Ask why the code was implemented in a certain way, why a particular algorithm was chosen over another. There might be a valid explanation.
During a beginner’s code review, there is an opportunity to sometimes offer your own version of the code instead of just leaving a comment. This approach requires more time and guidance. This is something to consider when bringing a junior developer onto the team.
A good practice is to let beginners write code as they see fit initially, and then show them how to improve it. They can then make the necessary changes before submitting their code for a review in the traditional sense. This way, the code is examined twice. On the other hand, this approach helps the beginner to get up to speed with the project more quickly. Additionally, involving beginners in code reviews with more experienced colleagues allows them to learn established practices and overall project principles.
Conclusions
The approach to code review should be tailored to the project, especially its level of criticality. For instance, when developing a banking application that handles financial transactions, code review should be conducted meticulously and involve multiple rounds with different individuals. However, for a simple landing page development, such complexity is unnecessary. It’s important to recognize that using the same standardized methods across different projects within a company may not be suitable. It’s crucial to adapt and tailor the code review process to each specific situation.
