Improving the review process in a team
Review is an important stage of development and one of the most frequent points of interaction among developers with code and among themselves, especially in distributed teams. Typically it involves a developer (reviewer) examining code changes proposed by another developer.
In this case, the developer (we hope):
Up to date with the business process
Understands how his code fits into the overall solution architecture
I am sure that the proposed solution works
Reviewer (worst case):
Doesn't know the business process
Not aware of the solution architecture
Not sure if the solution works or works as it should
The team leader wants:
To ensure the review goes quickly
To ensure that the review is of high quality
So that the developer and reviewer remain on good terms.
So what can you do in each of the three roles to keep everyone happy (and safe)?
Developer
Share context
The developer (in theory) knows everything about the task. Therefore, if he wants to quickly complete a review of a task, he should share his knowledge with the world. Precisely with the world, and not with a specific reviewer:
there can be many reviewers
knowledge may be useful to others – QA, PM, team lead, etc.
The best option for this is to leave a comment in the tracker:
What exactly did you do?
How and what I checked
Possible nuances that may be useful to the reviewer (Links to documentation, nuances of the solution)
The reviewer will still have to collect all this information (for a quality review), and he (they) will either go to the developer or look for it themselves. Both are additional time costs for 1+ team members that can be avoided.
Self review
The developer could rewrite the solution several times during the task. As part of such rewriting, all sorts of “residues” from discarded implementation options are inevitably formed. They are clearly visible to the reviewer, but completely invisible to the developer without looking at the final merge request. The conclusion is obvious – before submitting the code for review, you need to look at it independently in the exact volume (and form) in which it will be transferred. To save your colleagues’ time (and your own – the code is highly likely to be returned from review for improvements), it is important to do this before each submission for review (relevant in the case of ping-pong).
Things to check:
the solution must do what is needed – the application is assembled, launched, tests are executed (yes, this also happens). If you have it automated, great. If not, it’s worth automating.
the decision can be merged (no conflicts)
there is nothing superfluous in the solution (can be solved by various linters/checkers/sonars, but may not be solvable)
Unfollow comments, but do not close the discussion
Questions about merge requests are left as comments. The developer may agree with the comment and fix it, or he may not agree. In both cases, it is worth recording your decision based on the remark:
I agree and the fix will be as part of the merge request
I agree, but the fix will be within a separate technical task/branch
I don't agree and why
It is important not to close the discussion on your own based on a comment – the one who opened it should close it when a consensus is reached and recorded. Because the reviewer will first of all go to check his comments. If they are closed, they will have to be opened and (possibly) initiated again. Not to mention the fact that some of them may be undeservedly missed/forgotten, which creates the risk of missing a problem and getting a defect from QA (at best).
Reviewer
Immerse yourself in context
The reviewer may not really know anything about the task. For a quality review, he needs to dive into the context:
What is the business process and how is it changing?
What are the technical limitations on the project?
What are the rules in a team for organizing system architecture and code?
Ideally, the context should have already been described by the developer. If not, you should request the context from the developer himself or the task director.
Conduct the review step by step
Being in context, you can start the review itself. Within its framework you need:
Check that the task does what it needs to do
Visually analyze the correctness of corner cases (exceptions, errors in interservice interactions, TTL operations, etc.)
Review solution against architecture team guidelines
Check the solution for compliance with the team's rules for organizing the code base
Check technical details of the implementation (complexity of algorithms, greed, etc.)
Evaluate the writing style
It is important to carry out the review step by step and in this sequence – from the general (important) to the specific (unimportant).
Another important point is that if you find problems at one level, you should stop and return for revision. The reason is simple: changes in the more general (important) are almost guaranteed to lead to changes in the more specific (unimportant). The reviewer will spend time commenting on sections of code that could potentially change, and the developer will spend time analyzing what he needs to fix and what is no longer relevant.
Separate the important from the unimportant
As part of the review, you can find problems of varying degrees of importance:
Critical – problems for which a solution cannot be made.
Technical – problems of a technical nature that need to be done, but do not have a direct impact on the business and current performance.
Not important – questions are usually of a stylistic nature
When specifying a problem, it is extremely useful to indicate its importance. For example, mark technical and non-important problems with marks like “TECH” and “NIT”. Depending on the urgency of the task and the scope of the problem, technical problems can either be dealt with as part of the current task or transferred to technical tasks for improvement (refactoring). This creates a transparent technical backlog, the volume of which is known to all team members.
Team lead
For a specific merge request, a team lead may well not be needed. His task is to build a process and monitor its work.
Let me remind you of the team lead’s expectations from the review:
To ensure the review goes quickly
To ensure that the review is of high quality
So that the developer and reviewer remain on good terms.
Let's look at what can be done for each of the points.
To ensure the review goes quickly
To determine how to make a review fast, you need to figure out why it might be slow:
Take a long time to immerse yourself in a task
Colleagues cannot agree on technical nuances
The reviewer wastes time checking things that could have been checked automatically
You can immerse yourself in a task for a long time for various reasons:
The developer may not have described the context – everyone needs to be taught to do this
The developer could have described the context poorly – you need to develop a guide/checklist of what should be in the context description and use it in the workflow
The reviewer is far from the subject area – it is worth reconsidering the process of selecting/appointing a reviewer
If colleagues cannot agree on technical nuances, then most likely the team does not have a common understanding of what is important and what is not. This understanding needs to be formed in one way or another. But, even if such an understanding is present, there are always new or edge cases – the solution to such situations can be accelerated by changing the format of the discussion (conditionally, colleagues will agree on a call faster than in GitLab comments) or by introducing an “outside view” – another developer, in order to obtain a quorum to make a decision on the issue.
If a reviewer spends time checking codestyle and other basic things, this needs to be automated by setting up a merge request pipeline:
checking the project build
passing unit tests
checking with linters, static analyzers, etc.
To ensure that the review is of high quality
The review was carried out with high quality if eliminating the detected comments leads to an improvement in the solution, and the solution itself ultimately successfully passes through QA. Yes, not all problems found during the testing stage are the result of a poor quality review. But in my practice, most of the problems detected by testers could be found. It turns out that a high-quality review is ensured by the completeness of the “substantive” comments.
Since completeness of comments implies completeness of comments, both technical and related to business logic, the main task of the team lead is to set up the review process so that the task is reviewed by people who can ensure this completeness.
If there are no people in the team capable of qualitatively testing both components of the solution (business and technology) or there are few of them, it is worth considering the review in several stages and at the same time increasing the expertise of team members (so as not to limit the review to 1-2 people).
To prevent the developer and reviewer from quarreling
One of the team leader’s tasks is to ensure a healthy working atmosphere in the team. A review and the comments left within it can cause resentment, unhealthy conflicts and other troubles in the team. To prevent this, it is worth introducing and following a number of rules regarding what and how comments should be left:
All comments should be directed at the code, not at its author. Not “you’re a shit coder”, but “the code is stylistically (asymptotically) suboptimal”
If you criticize, suggest. If you have ideas on how to resolve a comment, it’s worth voicing them. (“The request will be executed, although there may be cases when this is not needed. Optional.orElseGet to help”)
If there is something to praise for, praise it. We are all accustomed to celebrating only shortcomings, although successes are no less (and sometimes more) important.
Corollary from point 1. If you have questions for a person, you need to contact the person directly in a personal message. A standard story in the spirit of “praise in public, scold in private”
Conclusion
Review is an important stage of development and a point of interaction between team members. It is important that it takes place quickly, efficiently, transparently and with pleasure for its participants:
Before submitting it for review, put yourself in the reviewer’s shoes.
Automate everything that can be automated (if the profit covers the cost of automation).
Build processes, optimize them and follow them.
Respect your colleagues, their work and their time as your own 🙂
