Code review is an essential tool for code quality control in the programming industry. However, according to my observations, many teams repeat the same mistakes, which negates the advantages of this tool. This topic has caught my attention for many years, and I would like to share some tips with you on how to improve this process.
Why Do We Need a Code Review?
First and foremost, code review allows us to check the quality of the code. But beyond that, there are at least two other important benefits. The first is knowledge sharing. The more actively you engage in code review, the better acquainted you are with all the changes in the project.
Another advantage is improved communication among team members. Code review is a direct act of communication with colleagues, and in the conditions of post-COVID remote work, every opportunity for communication is significant.
When Do We Ask for Our Code to Be Reviewed?
Typically, a programmer works on their task in a separate branch, and when they consider the task completed (and this is just their opinion, as the task will only be considered truly completed after it has been checked in production), they create a merge request to the main project branch, which is usually called master or staging depending on the chosen CI/CD process. It is at this stage that we ask for our code to be reviewed.
Saving Team Resources
It's important to understand that code review is not a free process. We are literally asking our team colleagues to divert from their current tasks, switch their attention context to another task, and spend their cognitive resources analyzing the code, not to mention that your colleagues' working time is not free. The most efficient way to minimize resources spent is automation.
Therefore, before a real person begins to review your code, it is necessary to use all available automation tools.
-
Unit tests - While discussions on the appropriateness of writing tests are still ongoing, in practice, unit tests have become an integral part of enterprise projects. Write tests, agree on a minimum code coverage percentage for tests, and do not allow code to be added to the project without the necessary level of testing.
-
Code style checking - All project code must be written in a unified style. All "holy wars" over the choice of stylistic elements should be concluded, and the code automatically formatted. For example, Prettier is a good automation tool for js/ts files.
-
Code guidelines - Many basic errors can be detected during the static code analysis stage. I recommend not using ready-made rule sets but rather choosing a configuration that meets the needs of your project independently. For the JavaScript world, use ESLint.
Who Should Review the Code?
Different companies apply various methodologies to determine who will conduct the code review. One such method is peer review, in which you can choose any team member. Thus, the situation where a junior developer reviews the code of a senior developer should not surprise you. This can also be organized through a rotation of developers.
However, in my opinion, the worst option is one where all code reviews are conducted by the same person, especially the team's technical lead. Of course, you can set a rule for the technical lead to review every code merge request, but he should not be the only person doing it. These duties should be evenly distributed among all team members.
CODEOWNERS File
Sometimes, an individual developer may be an expert in a particular part of the program, and we would like them to review all the changes made by other programmers in that part. The GitHub CODEOWNERS file can help with this, allowing you to describe direct dependencies between files and developers who "own" these files. A good example of the CODEOWNERS file can be found in my previous article.
Code Review Is an Essential Part of Working on a Task
Many developers mistakenly perceive the completion of work on a task immediately after creating a merge request. In reality, this is not the case. Quality code review remains the responsibility of the code's author, who must make every effort to ensure this stage goes as smoothly as possible.
How to facilitate this?
-
Minimize the number of changes in each merge request. Aim to add new functionality to the main branch iteratively, introducing one logical unit at a time. Avoid mixing unrelated code; it's better to create several separate merge requests.
-
Provide as much information as possible in the description of your merge request. If you worked on an algorithm, describe its operating principles. In the case of working on the interface, add screenshots or screencasts demonstrating its functionality.
-
Agree with the team on labeling that indicates the type of changes in the code. Some understandable label examples: style, docs, refactor.
What to Do With Large Merge Requests?
Imagine the situation: you have done everything possible to simplify the process of reviewing your code, but the number of changes to the merge request is still large. You care about a quality review because the functionality is complex, but you're not sure if someone is willing to go through all the hundreds of files with changes. In such cases, practices borrowed from extreme programming can help.
Pair programming has proven its effectiveness in writing complex parts of the system. Why not adapt this practice for code review? Although it may seem controversial, pair reviewing code in a live dialogue format, where the code author explains their work in detail, can significantly reduce the time spent reviewing large merge requests.
How to Effectively Comment on Merge Requests.
So, I offer four main recommendations:
-
Be polite. Unfortunately, in some development teams, toxic behavior can be considered the norm. It's important to change this, as only in a healthy environment can an effective code review process be created.
-
Explain your point of view. Not everything that seems obvious to you will be so for others.
-
Find a balance between giving clear instructions and allowing the developer to find solutions independently. This promotes learning and the development of developers' skills.
-
Encourage developers to simplify the code and add comments to it, rather than just explaining it to you. This improves the readability of the code and promotes better understanding among the entire team.
Additionally, reach a consensus within the team regarding the use of specific prefixes in comments that would indicate the importance and necessity of responding to this comment. For example, you could use prefixes like "Must fix" for critical issues that require immediate correction and "suggestion" for proposals that can be considered at the developer's discretion.
Examples of common and useful comment prefixes:
Must fix: This is a major issue and I will not approve a pull request without these changes.
Nit: This is a minor thing. Technically you should do it, but it won’t hugely impact things.
Optional: I think this may be a good idea, but it’s not strictly required.
FYI: I don’t expect you to do this in this CL, but you may find this interesting to think about for the future.
If I had to choose just one aspect to change in the code review process, I would undoubtedly opt for promoting politeness and developing a culture of communication within the team. After all, code review is primarily an act of interaction between programmers. If relationships within the team remain tense, no other changes can make this process truly effective.