Best practices in Code Review

The correct code review process is a control process. Control that the task is completed in full. Control that the general rules and agreements are observed. Ensuring that the solution is not redundant and that it is easy to maintain and evolve in the future.

For starters, it’s a good idea to ask your team the following questions:

  1. How long does it take to review the code for an average (spherical in a vacuum) problem?

  2. How do you minimize review time?

  3. How do you determine that the review of a particular issue is done correctly?

Collective code ownership

Reducing the bus factor due to the fact that each component in the code was seen and analyzed by more than one pair of eyes, and the knowledge does not belong to one specific developer.

Code reuse

When several people, solving different work problems and having a different set of knowledge about the product, look at each other’s code, they can see certain patterns and either propose to reuse one of the ready-made solutions, or select the current one in a separate library.

Knowledge sharing in a team

Everyone on the team has a lot to learn. Someone is good at algorithms, someone is good at metaprogramming, and someone is a Jedi MVC. Being able to look at each other’s solutions and ask questions is a great way to raise the technical level of the entire team.

Error detection

The most obvious goal is to detect errors of varying severity. This is what your manager thinks about in the first place when you offer him to lay down a certain time process for code review.

Uniformity of the project

As there are programmers in the world, there are so many different views on how to write code. Tabs and spaces, project file structure – in the absence of consistency, this can greatly complicate the process of reading and analyzing the code.

Self review

The author is able to see most of the actual errors in his code for himself.
Therefore, before connecting your colleagues to the review, you should carefully review your code once or twice on your own.

This simple procedure will greatly save time and nerves and improve relationships with colleagues.

Code review speed

  1. The team’s speed is generally slowing down. Yes, if a person doesn’t respond quickly to a code review, they do other work. However, for the rest of the team, new features and bug fixes are delayed by days, weeks or months as each CL awaits code review and re-code review.

  2. Slow reviews also discourage code cleanups, refactorings, and further enhancements to existing CLs.

  3. Developers start to protest against the code review process. If a reviewer only responds after a few days but asks for major changes each time, it can be frustrating and difficult for developers. This is often expressed in complaints about how “strict” the reviewer is. If a reviewer asks for the same significant changes (changes that actually make the code run better), but responds quickly each time, the complaints usually go away. Most complaints about the code review process are actually resolved by expediting the process.

How to do code reviews quickly?

If you are not in the middle of a focused task, you should do a code review shortly after it arrives.

One working day – the maximum response time (i.e., this is the first case the next morning).

Following these guidelines means that the typical CL should receive multiple rounds of review (if necessary) in a single day.

Speed ​​and distraction

There is one point where personal speed takes precedence over team speed. If you’re in the middle of a focused task like writing code, don’t get distracted by code reviews. Research has shown that it can take a long time for a developer to return to a smooth development flow after a distraction. Thus, distraction from coding actually costs the team more than asking another developer to wait a bit for the code review.

Instead, wait for a breakpoint in your work. For example, after completing an ongoing task, after lunch, returning from a meeting, returning from an office kitchenette, etc.

Comments on MR

  • Be polite

  • Explain your reasoning

  • Avoid explicit orders by simply pointing out problems and letting the developer solve

  • Encourage developers to simplify code or add comments instead of simple explanations of complexity

Good example: the concurrency model here adds complexity to the system and I don’t see any actual performance advantage. Since there is no performance advantage, it is best if this code is single threaded rather than using multiple threads.

Bad example:
Why did you use streams here when it is obvious that parallelism does not provide any benefit?

Checklists and their automation

Examples of questions from such a checklist: “Is there a commented-out code?”, “Is the business logic covered by tests?”, “Are all errors handled?” Memorize all the questions that the team often have for review, and put them on the list.

The main thing is not to let this checklist grow. Think about ways to automate each of the checks listed there – if you put in enough effort, you can cover most of the needs. There are a huge number of tools that can help in this job – linters, static analyzers, duplicate code detectors

Architectural review

Code Review is not the best place to argue about global architectural decisions, the code has already been written, the task is almost completed, rewriting everything from scratch is too expensive. Such things should not be left for later, but discussed in advance.
As an option – conducting an architectural review, protecting your component in front of the team either in words or in the form of a diagram. Of course, there are times when someone dawns on someone already during the review – then this proposal, if it is really valuable, should be documented in the issue tracker and never come back to it.

See good

If you see anything good in a commit, please let the developer know, especially when he solved the problem in one of your comments at his best. Code reviews often just focus on bugs, but they should also reward and value good practices. From a mentoring point of view, sometimes it is even more important to tell the developer what exactly he did right, and not where he went wrong.

No perfect

The key point here is that there is no “perfect” code – there is only code better… The reviewer should not require the author to polish every tiny fragment.

Rather, the reviewer must balance the need for further progress versus the importance of the proposed changes. Instead of striving for the ideal, the reviewer should strive for continuous improvement A commit that generally improves the maintainability, readability, and comprehensibility of the system should not be delayed for days or weeks because it is not “perfect”

Code review size limitation

Scheduled work

Can implement a table for code review on a schedule

Name

9-12

12-15

15-18

Alex

+

David

+

Alyosha

+

Who is right?

When a developer disagrees with your proposal, first take the time to review their position. He is often closer to the code than you are, and therefore may actually have a better understanding of certain aspects. Does his argument make sense? Does it make sense in terms of code quality? If so, admit that he is right, and the question will disappear.

However, the developers are not always right. In this case, the reviewer must further explain why he believes that his proposal is correct. A good explanation demonstrates both an understanding of the developer’s answer and additional information on why the change is required

In particular, when a reviewer believes that his proposal will improve the quality of the code, he should insist on it if he believes that the resulting improvement in quality justifies the additional work. Improving the quality of the code, as a rule, occurs in small steps.

Sometimes it takes several rounds of explanation before it is actually accepted. Just make sure to always stay politeand let the developer know you hear it, just disagree

Are you offended?

Reviewers sometimes feel that a developer will be upset if a reviewer insists on improving. Sometimes developers are really frustrated, but usually not for long, and later they will be very grateful that you helped them improve the quality of their code.

Usually, if you are polite in your comments, the developers don’t really get upset at all, and the concern is only in the head of the reviewer.

Usually more frustrating comment writing stylethan the insistence of the reviewer regarding the quality of the code

What you should pay attention to?

  1. To what extent does the proposed solution match the task?

  2. How difficult is it to understand, maintain, expand?

  3. How resource-demanding is the resulting architecture and what is the complexity of the implemented algorithms? Could it be simpler, clearer, cheaper?

  4. Did you do something unnecessary that is not related to the task?

  5. Are we using external dependencies and libraries? How justified is this?

  6. Does the solution have any disadvantages?

  7. Typos in the code magic numbers and other undefined constants. Magic strings and other unexpected data that the programmer did not take into account when solving the problem.

  8. Null checks and other places where you can potentially “shoot yourself in the foot.”

  9. Checking the solution from the point of view information security etc.

  10. Testability of code and availability of unit tests. This stage of checks is also very important, because it is aimed at improving the very notorious code maintainability.

  11. Has the developer chosen good names everywhere? A good name is long enough to fully convey what the element is or does without being so long that it becomes difficult to read.

Similar Posts

Leave a Reply Cancel reply