How to make code reviews faster and more efficient / Skyeng Blog

How does a code review usually happen? You send a pool request, get feedback, make corrections, send fixes for a second review, then get approval, and merge occurs. It sounds simple, but in reality the review process can be very time-consuming.

Imagine you have a pool request with hundreds of lines of change. The reviewer should spend a lot of time to fully read the code and understand the proposed changes. As a result, the whole process from creating a pool request to its approval can take several days - this is not very pleasant for both the reviewer and the author of the changes. And the chances are good that in the end the reviewer will still miss something. Or the check may be too superficial, and in the worst case, the pool request can be rejected immediately.

It turns out that the larger the pool request, the less benefit will be from checking it.

How to avoid such situations? How to make pool request easier and more understandable for the reviewer and optimize the whole process?

We are translating an article by our backend developer Sergey Zhuk about how the code review process of the Skyeng mobile application team works.

Change Categories

Let's imagine that you have a task - to implement new functionality in the project. The pool request you are working on may contain different categories of changes. Of course there will be some new code in it. But in the course of work, you may notice that some code needs to be refactored beforehand so that it contributes to the addition of new functionality. Or with this new functionality, duplication has appeared in the code that you want to eliminate. Or you suddenly found a mistake and want to fix it. What should the final pool request look like?

First, let's look at what categories of changes can occur with code.

1. Functional changes.
2. Structural refactoring - changes to classes, interfaces, methods, movement between classes.
3. Simple refactoring - can be performed using the IDE (for example, extracting variables / methods / constants, simplifying conditions).
4. Renaming and moving classes - reorganizing the namespace.
5. Removing unused (dead) code.
6. Corrections code style.

Strategy Review

It is very important to understand that each of these categories is reviewed differently and when considering them, the reviewer should focus on different things. It can be said that each category of change involves its own review strategy.

1. Functional change: solving business problems and system design.
2. Structural refactoring: backward compatibility and design improvement.
3. Primitive refactoring: improved readability. These changes can mainly be done using the IDE (for example, extracting variables / methods / constants, etc.).
4. Renaming / moving classes: has the namespace structure improved?
5. Removing unused code: backward compatible.
6. Corrections code style: most often merge pool request happens immediately.

Different approaches are used to check changes in different categories, therefore, the amount of time and effort spent on their review also varies.

Functional changes. This is the longest process because it involves changing domain logic. The reviewer looks to see if the problem is resolved and checks whether the proposed solution is the most suitable or can be improved.

Structural refactoring. This process requires much less time than functional changes. But there may be suggestions and disagreements about how exactly the code should be organized.

When checking the remaining categories in 99% of cases, merge occurs immediately.

1. Simple refactoring. Has the code become more readable? - merzhim.
2. Renaming / moving classes. Has the class been moved to a better namespace? - Merzh.
3. Removing unused (dead) code is merzhim.
4. Corrections code style or formatting - merzh. Your colleagues should not check this during the code review, this is the task of the linter.

Why should changes be categorized?

We have already discussed that different categories of changes are revised differently. For example, we verify functional changes based on business requirements, and in structural refactoring, we check backward compatibility. And if we mix several categories, it will be difficult for the reviewer to keep several review strategies in mind at the same time. And, most likely, the reviewer will spend more time on the pool request than necessary, and because of this, he may miss something. Moreover, if the pool request contains various kinds of changes, with any correction the reviewer will have to revise all these categories again. For example, you mixed structural refactoring and functional changes. Even if the refactoring is performed well, but there is a problem with the implementation of the functional, then after the corrections the reviewer will have to look at the entire pool request from the very beginning. That is, re-check both refactoring and functional changes. So the reviewer spends more time on the pool request. Instead of immediately contemplating a separate pool request with refactoring, the reviewer should once again review the entire code.

What exactly is not worth mixing

Renaming / deleting a class and its refactoring. Here we come across Git, which does not always correctly understand such changes. I mean large-scale changes when many lines change. When you refactor a class and then move it somewhere, Git does not perceive it as moving. Instead, Git interprets these changes as removing one class and creating another. This leads to a bunch of questions during the code review. And the author of the code is asked why he wrote this ugly code, although in fact this code was simply moved from one place to another with minor changes.

Any functional changes + any refactoring.We have already discussed this case above. This makes the reviewer keep in mind two review strategies at once. Even if refactoring is done well, we won’t be able to delay these changes until functional changes are approved.

Any mechanical changes + any changes made by man.By “mechanical changes” I mean any formatting done using the IDE or code generation. For example, we apply the new code style and get 3000 line changes. And if we mix these changes with any functional or any other changes made by a person, we will force the reviewer to mentally classify these changes and reason: this is a change made by a computer - it can be skipped, and this change made by a person is needed verify. So the reviewer spends a lot of extra time checking.

Example

Here is a pool request with a method function that neatly closes the client connection to Memcached:



Even if the pool request is small and contains only a hundred lines of code, it is still difficult to verify. As you can see from the commits, it contains various categories of changes:

• functional (new code),
• refactoring (creating / moving classes),
• corrections code style (removal of excess dock blocks).

At the same time, the new functionality itself is only 10 lines:



As a result, the reviewer should look at all the code and

• Verify that refactoring is OK.
• check whether the new functionality is implemented correctly;
• determine if this change was automatically generated by the IDE or by the person.

Therefore, such a pool request is difficult to review. To correct the situation, you can break these commits into separate branches and create a pool of requests for each of them.

1. Refactoring: class extraction

There are only two files. The reviewer should only check the new design. If everything is in order - merzhim.

2. The next step is also refactoring, we just move the two classes to a new namespace

Such a pool request is quite simple to check; it can be instantiated immediately.

3. Removing excess dock blocks

Nothing interesting here. Merzhim.

4. The functional itself

And now the pool request with functional changes contains only the desired code. So your reviewer can focus only on this task. The pool request is small and easy to check.

Conclusion

Rule of thumb:

Do not create huge pool requests with mixed categories of changes.

The larger the pool request, the more difficult it is for the reviewer to understand the changes you have proposed. Most likely, a huge pool request with hundreds of lines of code will be rejected. Instead, break it into small logical parts. If your refactoring is in order, but functional changes contain errors, then refactoring can be easily held back, and so you and your reviewer can focus on the functionality without looking at all the code from the very beginning.

And always follow these steps before sending the pool request:

• Optimize your code for reading. Code is much more readable than written;
• Describe the proposed changes in order to provide the necessary context for their understanding;
• always check your code before creating a pool request. And do it as if it were someone else's code. Sometimes it helps to find something that you missed. This will reduce the likelihood of rejection of your pool request and the number of corrections.

My colleague Oleg Antonevich told me about the idea of ​​breaking changes into categories .

From the Editor: Sergey writes a lot of interesting things about programming and PHP, and sometimes we translate something: a streaming video server , rendering HTML files . Feel free to ask him questions in the comments to this article - he will answer!

Well, we also remind you that we always have a lot of interesting vacancies for developers!