Process code review in hh.ru

I came across a document with the rules and recommendations for the process of reviewing the code within the company. I decided that such useful information should be shared with the outside world. With the blessing of the author, I publish the work.

Most of these rules are advisory in nature and must be guided first of all by common sense, not blind observance.

General provisions

In hh.ru review takes place in the pull request format on Gitkhab.
Review and pull-requests are required, even if the task changes one line or one character in the code.
Each task must have at least one responsible reviewer, it is indicated in the task as a reviewer and is responsible for the code, as well as the author. Participation in the review of other people is not prohibited.
The responsibility to conduct the task through the review lies with the author of the task.
Any changes after the review, for example, edits during testing, should also be verified.

Goals Review

Dissemination of experience and knowledge among developers.
Support of the principle - the changes should not degrade the already existing code, they should improve it.
Correction of errors in logic and errors related to security, correct exception handling.
Improving code quality, maintainability and project architecture.
Improved code readability.
Improved test coverage and testing at the right level.
Improved documentation.
Compliance with the requirements of the changes in the task.
Prevent the emergence of technical debt or technical tax.
A thoughtful API design, where the API is any module with an external interface. For example, that a resource in a service has a well thought-out URL, a convenient JSON format, correct response codes in different cases, and so on.
The correct history of commits in Gita, with messages reflecting the essence, without temporary commits.

Preparing for the review

Before publishing a pull request, read your code 2-3 times: most likely you will find errors there, which will save time for the reviewer. Check that there are no errors that can be detected automatically - errors in the style of the code, dropped tests. Make sure that there are no temporary comments and a debugging code in the code, and also check if your code corresponds to the accepted style guides.
If you create a pull-request in the process of working on a task, write a note in the title “[WIP]” and tick “work in progress”. When the work is finished, remove the tags and inform about it with a separate comment so that interested people know that the code can be viewed.
Get ready to show the mini task demo to the reviewer.
Give a small portion of the code for review. 200-300 lines of change - the limit for effective review.
Make independent changes by separate commits.
Describe commits with short and informative messages.
Refactoring make a separate commit, it is easier to see the changes in logic, the essence of the problem.
Code formatting, make a separate commit before major changes, or even a separate pull request.
Do not commit insignificant changes. For example, adding line breaks in a file in which you didn’t change anything else.
In the title of the pull request, briefly and informatively describe the essence of the changes.
Describe the problem and solution in the pull request. Describe alternative solutions and why the current solution is chosen. If the changes concern the interface, attach screenshots of “before” and “after”.
In the pull request, give a link to the task, and in the task link to the pull request.
Sometimes it is useful to discuss the chosen solution with the reviewer before writing the code. This will help find the best solution to the problem and simplify the review process.

Choosing a reviewer

Give the review task to a specialist from the relevant field.
If several commands work with a certain code, then it is useful to select reviewers from another team to spread knowledge.
An employee on probation cannot be a responsible reviewer, but can participate in the review process.
Do not give all your review tasks to the same people - ask for reviews from different people.
If the task affects a non-trivial code segment, which a certain person understands, show the changes to him, regardless of who is the responsible reviewer. Also remember that every repository has meintainers, they know more about this code and supervise development.
The rest of the reviewer is chosen arbitrarily, by mutual agreement of the parties. To help you choose, use the recommendations on Github based on the “blame” of the affected code.
After selecting a reviewer, enter it as an Assignee in a pull request. A list of all pull requests you have been assigned to.

Review process, general provisions

It refers both to the author of the code and to the reviewer.
')

All code is common, there are no such concepts as “my code” or “your code”. This means that for every line of code every developer is responsible, and not just the one who wrote this line.
Create an atmosphere of mutual understanding, be polite and friendly, appreciate and respect each other, do not impose your opinion. Listen to the opinion of your opponent and do not stand on your own principle. Do not turn reviews into a negative experience for colleagues.
Ask questions if something is not clear. Argue and clarify questions and answers. The author of the code may not be obvious what is meant by the question “why so?”, And the reviewer does not understand the reasoning of the answer “I do not agree”.
Do not stretch the review process, save time, quickly respond to comments, discuss verbally, do not provoke long discussions and don’t make holivars. If you can not quickly come to a consensus, contact your colleagues, meintainer or head of the functional direction for help.
If the task is not on a couple of lines, spend 10-15 minutes with the reviewer on joint viewing of the code, it is useful to do this even before creating a pull request. Discuss the essence of the tasks and solutions, look how it was and it became in the working environment. This will help the reviewer to better immerse himself in the context of the task. Most comments will remain unwritten, which will save everyone time.
After an oral discussion, always describe the decision made and the reasons for it in a pull request. The responsibility for the description lies with the author of the code.
If the code contains the same type of errors, the reviewer should focus on the attention of the author of the code in the first or second comment, without commenting on each entry, and the author should analyze the code for similar errors before publishing the fix.
Praise for the author's successful decisions and suggestions of the reviewer.
Leave comments on changes in the pull request itself, not on individual commits. Thus, all correspondence will be in one place, including after the rebuy.
Reply to comments in the same thread where they started. If the comment applies to the entire pull-request or in a single thread a few comments, quote the commented message. To move from the letter to the outdated discussion thread, use the “in path / to / file” link instead of the “view it on GitHub” link.
If in the review process any cardinal decisions are made from the point of view of coding standards or other arranged agreements, the author of the code is obliged to record these decisions in the relevant documents.
The review is not only about the code, but about the whole problem. Do not treat the requirements in the task or the layouts as the ultimate truth - everyone makes mistakes and no one can take into account all the nuances. A fresh look and practical suggestions will only benefit the product.

Process review by the author of the code

The task is not completed until it has been reviewed, tested and released for "Prod".
Reply to all comments of the reviewer, do not leave comments unanswered, regardless of whether you accepted them or not.
Think about questions that arise or may arise during the review. Perhaps a section of the code lacks a comment or it is better to write the code more clearly.
Do not fix existing commit (“git commit - namend”), instead make changes with separate commits, one for each fix. Correcting existing commits makes the review process very difficult, as you have to watch the whole code again.
After adding a new commit to the pull request, write a comment about the changes in a separate comment so that interested parties are informed. This applies to both revisions during the revision and revisions during testing.
After the review, before submitting the task to testing, rewrite the history of commits (git rebase --interactive), making the appropriate corrections to individual commits and deleting temporary commits. Note the --autosquash option to simplify the process.

Review process by the reviewer

If at the time of the request you are busy, let us know exactly when you will begin the review.
Do not ask, but suggest changes.
Comment on the code, not the person who wrote it.
You take responsibility for the written code, approach the review appropriately, but this does not mean that the author of the code should strictly fulfill all your requirements. Remember, many things can be done in different ways.
Follow the objectives of the review and suggest changes to the merits. Do not insist on non-critical changes. Indicate the importance of corrections in the comments.
If you find a serious problem, suspend the review and wait until the author fixes it. Most likely, after the fix, you will still need to revise again.
Pay attention not only to what has been changed, but also to what has not been changed. Look for and show the author a code that should have been changed, but was not (forgotten imports or unused classes, using the refactored code elsewhere, and so on).
After making changes by the author of the code, review the corrections and re-view the entire pull-request. Perhaps taking into account corrections you will have new comments or questions.
Do not waste time reviewing code that has not changed as part of the task. The selection of a part of the code in a function is considered to be a change. The transfer of the code "as is" for the change is not considered. Reformat code in the affected file at the discretion of the author.
The reviewer does not independently rule the code in the branch, because he so wants or is easier. Changes are made by the author of the code.
If you are going to review, try not to delay and not switch to other tasks to the detriment of the current

Used materials and related links

Code Review Best Practices by Kevin London
Why code review matters? by Anton Kuznetsov
Code Reviews Can Make Or Break Your Team by Amir Yasin
The Ethics of Code Reviews by Marco Troisi
Effective Code Reviews Without The Pain by Robert Boag
MDN - Code Review FAQ by Marcia Nus and others
How to create a good pull request
How to perform a good code review
Top ten pull request review mistakes from Skot Noneberg
Code review in remote teams by Sean Hammond
May the Code Review with Yegor Tolstoy
How I learned to make the world better in HeadHunter by Ivan Baranov

PS Share in the comments your own rules, recommendations and useful cases on the review process

Source: https://habr.com/ru/post/422399/

All Articles