The practice of code review: controversial issues

A lot has already been said about code review, but there are still points that can be argued about. It was them that we recently discussed at one of the internal IT gatherings.

Code review by itself is not a panacea. It can be built to help the project. But this benefit is not guaranteed – there are plenty of examples that code reviews are detrimental to. Let’s talk about where the underwater rake is hiding here and how to conduct a better code review.

It is pointless to conduct a code review when there is no normal problem statement

Let’s start with the basic stuff. Code review is a belated decision if the task was originally set incompletely, incorrectly or without normal input.

A fairly common situation is that a new developer in the team wrote and wrote code, and at the review they say: “It’s impossible, we don’t do this” (for example, we don’t use REST at all). At this point, everyone is in an uncomfortable position. The developer wrote the code, but no one needs the results of his work. And the reviewer, although he understands this, will insist on his own, because in a sense he is responsible for the project.

Who in this situation should convey to a new person that REST is not used is a philosophical question. Perhaps this is the responsibility of the one who onboards or the task of the team leader. Some companies believe that this should be the initiative of the newcomer himself. But as a rule, responsibility is blurred between all participants in the process, so it is much better if all non-obvious points regarding the project and the approaches used are documented.

In any task, you need to discuss what is required beforehand – before even a single line of code is written. It’s good practice to do this when planning.

In reality, it doesn’t always work out that way. It happens that during the discussion a solution was proposed with which everyone agreed, and the developer implemented it. But during the code review, the responsible person comes up with the idea that the problem, in principle, should have been solved differently. But that’s another story – about improving the code with an eye on future refactoring. Whether to listen to a new idea during the review phase depends on the details of the project organization.

Code review should be fast

It’s bad practice to put off code reviews for a week or more. When the review is stretched, the code gets into production very slowly, and the developers get tired of constantly returning to tasks that they handed over many days ago. You have to spend time on a new immersion in the context.

In a good way, a code review should be done as soon as a pull request appears. A delay of an hour is normal, half a day is not very good, because all this time the developer is waiting and he has to keep the context of the task in his head.

Good practice is to control whether the assigned reviewer has looked at the code, and if not, throw additional notifications to him. Perhaps he was just distracted.

In general, it’s not bad when the same people who write the project code participate in the review. As they wait for their pull request to be reviewed, they realize how important it is not to delay colleagues in a symmetrical situation.

The reviewer must understand that his task is not about subjective perfectionism

Reading someone else’s code, there are always thoughts about what could be done differently – prettier. But the reviewer needs to limit his flow of comments. At the forefront is a working code that meets the requirements of the project, i.e. the desire to bring beauty should have a reasonable framework.

It is normal practice to mark some comments as minor (suppose, as Google recommends in their manual, with the NIT prefix, https://google.github.io/eng-practices/review/reviewer/looking-for.html). Such a prefix means that the idea is worth conveying to the developer, but the reviewer does not insist on the fulfillment of the wish, i.e. the code can be improved, but this remark does not block the pull request. Depending on the team culture, the reviewer may not even wait for a response to this comment.

There are situations when it is difficult to unambiguously separate taste from practical remarks. But in such situations, it is useful to ask yourself higher-level questions – why is all this happening.

For example, when it comes to acceptable code complexity, we need to think about what we write not only for the machine to read the code, but also for people who will later debug and develop this code. And in fact, it all depends on the tradeoff between how quickly you need to roll out features (that is, how much time we can spend on licking a particular piece of code) and how important it is to simplify it. Perhaps it will not be necessary to return to a specific piece of code at all? Those. licking it, we will spend more time, but the result will not change.

No attentiveness – no normal code review

You can come up with all sorts of ways to distribute pull requests between reviewers, but if a person basically does not want to participate in this, most likely he will formally agree on the code and will not even delve into what is written there. And this is despite the fact that the code review itself includes an answer to the question, does the pull request solve the problem?

It is impossible to force to read the code carefully.

We will leave global questions of motivation outside the scope of this article. Partial distraction of attention is sometimes facilitated by the distribution of responsibility among several reviewers. For example, a team leader and a developer are assigned as reviewers for the same pull request. The developer thinks that the team leader who will look after him will find any problems, if any, so he himself can check the code with one eye. This situation is usually seen in the quality and quantity of comments from both. Similarly, responsibility can be disabled if the project maintainer is watching the code, and in parallel, someone from the outside.

Alas, there is no universal way to deal with distraction. This question remains on the conscience of those who conduct the review.

Code review – not only about the code, but also about the interactions in the team

Code review is a way of discussions within the team, and sometimes training of a new specialist.

It’s a bad practice when code review comments are no longer passed through, but simply corrected mechanically, without looking. Only the code under discussion is improved, not the project as a whole. Yes, there are moments in the code that need to be unambiguously corrected. But there are also things that should be discussed, having developed some solution for the future, and in the future this should help the project. And as soon as the review turns into a mechanical correction, this part of it disappears without a trace.

When does it happen? When the comments are unfriendly. There are comrades who are also proud of the fact that they conduct code reviews very hard – every comment from them sounds like a hit-and-run: “What the hell is implemented here like this?”. This is bad practice. Even if you don’t like something in the code, it’s better to choose more neutral wording.

In the same way, there are developers who react badly even to very mild comments. Instead of making the code clearer in response to a remark about complexity, they write to the reviewer that he himself is a fool.

Both options do not contribute to the quality of the code review. They either lead to conflicts, or to the fact that no discussion takes place – one of the parties simply walks away from the discussion in order to save nerves.

And sometimes there are too many comments. One of my colleagues talked about his previous experience, when in a large company, a crowd of 30 Indians attacked a pull request as part of a review, who had no idea about the project. The number of comments had nothing to do with the quality of the code. And this, of course, is a very indicative, but not the only example. And there are a lot of comments when the first point discussed in this article is not fulfilled – there is no normal task setting or style guide.

Reviewer advice. What happens in a pull request is not always clear from the very beginning. And a good practice is to read the entire pull request to the end, and only then bombard the developer with questions. To do this, GitLab has a convenient function – write a comment and queue it without sending it right away. As a rule, this feature significantly reduces the number of comments, and you do not have to cancel your questions later, apologizing for the wording.

Developer advice. Not all remarks of the reviewer should be reacted head-on and nervous. For example, it is indicated that the code is too complex, but it cannot be simplified. It is enough to remember what the ultimate goal of the whole process is. If the reviewer cares that the code will not be understood in six months by those who get into it “from scratch”, perhaps the question will be removed by comments in the code itself (and not in its discussions in the pull request)?

Cross-team code reviews cost more; you need to understand what is corrected and why

The code review itself is expensive. A good review really helps to keep the quality of the code, and this is very valuable. A bad review takes a lot of people’s time, while the project does not receive any bonuses – it’s just the company’s money thrown away.

There was already an example above with a raid on one piece of code by 30 people who are not associated with the project. There are situations where a cross-team code review helps to look at a pull request with fresh eyes. But this is not the case. If there is no understanding of what each of the developers will bring to the project, there is no point in wasting the time of 30 people who are still not immersed in the task.

The problem is not only that the code is viewed by a person who does not understand what is happening in it. Suppose a colleague just wants to review qualitatively – i.e. look not only at the purity of the code, but also at the implementation logic. In this case, he will have to spend a lot of time even on a small pull request to dive into the context – read the doc, find out what the module as a whole does, what changes were planned to be made there. And this time is paid at the rate of the developer.

If there is no task to “broadcast” the code to these three dozen colleagues, it is more logical to share responsibility somehow. Assign reviewers in accordance with the qualifications and immersion in the project, add approvers for safety net. You can even take into account the size of the pull request in lines and the workload of each individual colleague when distributing tasks for a code review, as well as come up with any other restrictions. If only the team was happy with it and the time was spent usefully!

Established code review practices are good, but not for juniors

Above, we talked about different aspects of code review, it seems that we suggested working practices. But there is nothing universal.

A beginner’s code review is the moment where you can sometimes insert your own version of the code implementation instead of a comment. With more experienced colleagues, it is always recommended to write not your answer to the problem, but a question to a colleague who is the author of the code. Why is it done this way? Why is one algorithm applied and not another? Perhaps there really is a worthy explanation. And with beginners, code review turns into training to a greater extent with the corresponding time costs and the placement of authorities. It is important to keep this in mind when inviting a junior to the team.

It is a good practice when a beginner writes as he sees fit, and then they show him how to do it better. Then he reworks and only then his code is sent for review in the usual sense. It turns out that the code is looked twice. But on the other hand, a beginner rolls into the project faster. You can also connect a beginner to the code review of more experienced colleagues. So he will learn established practices and general principles on the project.

Instead of totals

The practice of code review should depend on the project, more precisely on its criticality. When the object of development is a banking application that counts money, the code must be looked at with great passion and the code review should be carried out in several rounds with the participation of different people. And when a simple landing is being developed, there is no point in this complication. As a result, it is not worth even within the same company on different projects to apply the same template methods without trying to adapt them to the situation.

PS We publish our articles on several sites of Runet. Subscribe to our page VK or at Telegram channelto find out about all publications and other news from Maxilect.

PPS At the link you have a good translation of Google’s manual on code review.