Perhaps you read the first part of the article about the review code by the reviewer (by the way, we already managed to discuss it in the last issue of the Zinc Prod podcast ).
Since the article gained a lot of likes, I write the promised continuation about the code review on the other hand - from the author of the code changes
As usual, we will say MR (Merge Request) instead of CL, because few people understand the term CL.
The original instructions for the authors of MR according to Google can be found here , and I will give a brief squeeze.
So let's go
Description MR
At Google, the description of the changes is stored in the history of the version control system, and it is very likely that many people will read it in the future. Therefore, the description of MR is very important.
The first line (in the title) should contain one phrase describing what was done in MR. Moreover, according to tradition, an imperative (imperative) style is used, i.e. Delete blablabla , not Deleting blablabla
The description itself should be informative, contain a brief description of the problem being solved, links to the necessary documents (if necessary), task tracker tasks and other context. Even in the little MR something like that should be.
A description of the type "Fix bug", of course, is considered inadequate.
MR should be as small as possible
- Little MR can be checked quickly
- Verification will be more meaningful
- Less likely to miss a bug.
- Not so offensive if the entire MR is rejected. After all, it’s very bad when a lot of work is done, and then it turns out that all this was not necessary at all
- Easier to push changes, less conflict
- Easier to achieve good quality code.
- The more changes at a time, the more difficult it is to roll back the code if necessary
Very rarely there is a situation when it is impossible to reduce the size of the MR, breaking it into pieces. The reviewer has the right to reject MR if it is too large
Of course, there can be no hard and fast rule, which MR will be considered large, which small. Is 100 lines of code big or not? Who knows.
- MR must do one thing. Usually this is not the whole feature, but part of it
- Separate refactoring into a separate MR
MR should be small but self-sufficient
- Everything that a reviewer needs to understand MR should be in that MR
- After injecting the code, the system should function normally.
- If an API method is added, the methods for using it should be demonstrated in the same MR. In other words, MR should not be so small that it would be difficult to understand how it will be used, what consequences it will lead to.
- Cover code with tests, and tests should be in the same MR
Do not take to heart
Sometimes reviewers are not very polite, they can write something bad about your code. This is not very professional on their part, but often there is still a grain of truth in such comments, and this must be taken into account. Do not respond too harshly. This will only aggravate the situation.
If you don’t like the tone of the conversation in the comments, it’s better to find this person and chat with him personally, to explain what does not suit you.
If this does not help, Google advises contacting the manager.
From my experience I want to add that often a person writing an impolite comment often does not give an account that it can look somehow aggressive. The text hides half the emotions; for example, what is said in a jokingly friendly tone in text form may seem like a harsh run over. Therefore, I completely agree with Google - in case of misunderstanding, communicate only in person!
Explain the code
If you are asked to explain some point in the code, consider whether you can correct the code so that it is understandable without explanation. Because if one does not understand, then others may not understand.
Response to Reviewer Comments
Often, when the work is done and sent to the review code, it is psychologically difficult to accept the fact that something will also have to be fixed. Therefore, try not to perceive the comments of the reviewer with hostility, think, maybe he is right, and the code will become better as a result.
However, if the reviewer is still wrong, feel free to write about it, providing the answer with arguments and facts.
conclusions
In general, as I understood from a Google document, the author of MR must do everything to make the task easier for the reviewer; for the reviewer to understand why the code was made, how the code was made, there must be all the context necessary for understanding, etc.
It is inevitable that disagreements will be resolved, coming to consensus in a polite professional form.
In the next issue of Zinc Selling, we will definitely discuss this article (and much more), so be sure to subscribe to our podcast , otherwise skip the fun part!