Here are described my research on how to make code revision in a team a more enjoyable exercise that can give new experience to all participants. We have a fully geographically distributed team, all communications are performed via the Internet, and often asynchronously. We use Trello to describe the capabilities of the products, create the code one by one, send pull-requests to GitHub, and also use their review function built into GitHub. This is different from viewing the code face-to-face in the office and even by video chat.

If you do not take the matter seriously, then asynchronous and written revision of the code can cause a disaster in the team, leading to a deterioration of interaction and cooperation. But if all participants try to do everything well, then this approach can work very effectively.

Why do I need to review the code?

Before you begin to talk about how to do and how not to do a review, it is useful to think about who the company needs it for and develop requirements in accordance with this. And it’s good not to forget that review is not just a search for bugs and problems in the code structure.
')
The review is needed to improve the quality of the code and morale in the team .

Analyzing each other's code is one of the main ways of interaction between developers during a normal work day. This should be an inspirational process for all participants to wait for and actively participate in it .

Here are the main tasks solved with the help of revisions:

• Creating an internal team culture. The Review is one of the main processes, which implies the presence of a command culture. And if it is unpleasant for participants to participate in the review, then there is no coherent command culture . Audits should create a positive atmosphere, tolerance and friendliness.
• Improving working relationships due to the opportunity to talk more often with colleagues.
• Relieving tensions with positive feedback about someone else’s code.
• Training. The reviewer can learn new things for himself from someone else’s code, and the author from a review.
• Going beyond the boundaries of their area of ​​responsibility, allowing all team members to analyze different parts of the code, getting acquainted with the entire code base.
• Mentoring and education. The Review is an opportunity for more experienced participants to become mentors and train others. But keep in mind that since time has passed and the code is written, review is not always the best time to learn. Developers must work together on a technical project both before and during code creation.
• Quality code. And finally, quality (including bugs, ease of maintenance, documentation, organization, architecture, usability of the product ...).

What not to do when reviewing

Perhaps this part will seem rather negative to you, but after it comes a part with pleasant things!

There are many different ways to make code review unsuccessful. Militant, evil and unpredictable comments can deprive a team of work pleasure and turn an audit into an emotionally heavy duty.

Understanding what to avoid during a review distinguishes the valuable part of the team’s work from cruel and unpleasant punishment. Eric Dietrich, “What to Avoid When Reviewing a Code”

Do not conduct a review formally and with minimal comments.

(aka find out how badly your colleagues think about you)

It is not necessary just to point out errors without adding anything to it.

Developers spend a lot of time on code and are proud of it, and review is their chance to demonstrate their work. And with a formally minimalist approach, all you can hope for is “Looks good, smerjil” comments. This review is worthy of the title " Find out how bad they think of you ."

Incomprehensible and ill-conceived communication can lead to long-term unpleasant consequences . Command culture will deteriorate, which will reduce productivity. At the same time, developers often receive feedback like “ it looks like this whipping was invented specifically to get the spirit out of them, ” and the revisions themselves turn into “ mental competitions in which people shoot at the target ... and the goal is the developer who wrote the code .”

Do not think that it is enough just to “criticize the code, not the coder”

This is one of the most popular tips: criticize the code, not the encoder. It is good (if you criticize a colleague, and not his code, then you have problems). But this is not enough. Writing code is a creative process in which the developer puts his heart and soul. And if you cruelly and stupidly criticize someone's code, then you thereby criticize the person himself. You need to be able to criticize. Find more positive ways to analyze and submit your suggestions on how to improve the code.

Watch the tone

Do not go to the individual. Phrases like “Why did you do this way?” Are perceived as attacks. A bad option: “The way you wrote this function makes it difficult to read, add more comments” (a personal appeal implies that the person did something wrong). Better say: “Do not you think that it is worth adding additional comments to the code, will this make it easier to read the function?” (Sounds much more correct and allows you to focus on improving the code).

Do not require demanding or defiant expressions. Avoid phrases whose meaning is "you are wrong." Do not tell people that they are wrong or that they said / did something wrong, it will be unpleasant for them. In such cases, people can go to the defense, they cease to work and learn long and creatively. Avoid phrases like:

• "It won't work"
• "Totally wrong"
• "Why are you just not ...?"

Do not exaggerate paint. You do not want the criticism to be offended for being unfounded or exaggerated? Do not say "always", "never", "infinite", "nothing" .

Do not insults. Avoid curses like “fool” or “idiot” , even if you personally do not consider them offensive. Such words have a certain range of meanings, and it definitely will not improve the mood of the author of the code.

Do not use impatient or passive-aggressive turns. Always be discreet and friendly, avoid unpleasant phrases that show irritation and make people feel that they are not doing well:

• "I repeat once again: here you need ..."
• "As I already said…"

Do not add fuel to the fire. It may be unpleasant for a person to get a comment from someone with criticism, and under it a few more with “+1” or morals for the same reason. In addition, numerous criticisms can simply confuse.

Do not be unsolicited adviser

Restrain yourself and do not suggest to make in a code all changes which you would like to make. You can demoralize the author of the code by offering to change so much that you have to rewrite everything. So choose only the most important and best.

One of the tasks of the review is to find and fix bugs and problems in the code structure. But do not force the author to write the code as you would have done. Do not forget that in the software, many things can be solved differently, depending on the tastes of the developer, and each approach has its pros and cons. If you are going to propose another revision, ask yourself: maybe this is just my opinion? If the author chose other methods, justifying his choice, then better support it. Respect the author's right to own decisions.

Even if someone wrote the code differently than you would have done, this does not mean that the code is written incorrectly. The main thing is a high-quality, easy-to-follow code. If he meets these criteria and the rules adopted in the team, then this is enough. Robert God, "Effective reviews without pain"

You can never force others to write just such a code, which you would write, do not even try (if it is so important to you, then write the code yourself right away). Eric Dietrich, “What to Avoid When Reviewing a Code”

Do not break the rule of "no surprises"

Avoid surprises . Create a common documented agreement for pull requests and code standards for the team and rely on it during the review, not your opinion.

Do not speak only to hear your voice

Before you write a review, think about what you want to achieve. Are all your comments needed? Remember that you are trying to constructively help, and not just express your point of view. Before you post a review, re-read your comments and ask yourself if they are all really useful and important? Remove unnecessary. It is better to analyze your notes after viewing the code, after the fact. While you are dealing with the code, write as many comments as you like, and then calmly review them and decide what to leave.

Leaving comments only to emphasize one’s rightness is not always useful (or reasonable). Sometimes it is better to keep your opinion with you in order to maintain long-term good relations with a person. Catherine Daniels, "How to give and get feedback"

You do not have to find problems during each review.

If the code is good and can be poured into the code base without changes, then everything is fine!

What to do when reviewing

The review can be improved. How exactly? Below you will read the sentences collected on the Internet.

Praise good code

Take the time to praise the strengths of the code before pointing out the flaws and make suggestions.

Human nature is such that we need to know about our successes, not just about failures. Development is a creative process in which people put their souls in, they often take a lot to heart. Therefore, praise is even more important to them. Robert God, "Effective reviews without pain"

You want the team members to see the review as a pleasant and rewarding experience, and not as pure criticism. Positive feedback reduces tensions and helps people respond better to criticism.

It is important to write positive comments so that it does not look sugary or false. You should not get the impression that you are just trying to sweeten the pill.

Avoid doubtful compliments . Phrases like "Good work, but ..." (further - the list of changes that you propose to do) look like a mock up.

How to make praises more honest?

• Evaluate most of the good parts of the code and other aspects and try to say why you liked them.
• Delving into someone else's code, leave your "monologue on the go" in the comments . "Yeah, I realized what it does ... Well, it connects here and causes it, great ... and this part depends on those two, great." Thinking another programmer who understands your code is another form of testing. And when you come across parts of the code that you do not understand, you can ask for an explanation.

First, the positive points

Immediately set the mood, first of all talking about what you liked . This can now be easily done thanks to new features of reviewing the code on GitHub , creating multi-line comments and publishing them in an array (instead of publishing as they are saved), as well as making a short summary (displayed at the beginning of the review) before making comments:

Avoid inevitable charges

Ask, but do not say. It is better to ask questions, rather than make statements.

A statement is an accusation. The phrase “You have not met standards here” is a charge, intentional or not. Ask the question: “Why did you use this approach here?” Robert God, “Effective reviews without pain”

If you ask questions, this will improve the general attitude, the perception of the review will change due to the invitation to dialogue and learning, and the author of the code will be happy to explain the reason or look for a better solution.

Ideally, in response to a correct question, the author himself will find a bug or offer to make improvements to the code.

If you see some moments that may be mistakes, do not immediately tell people that they are wrong. Usually it is enough to ask: “What will happen if I give a zero to this method?”, And the person will probably himself say: “I had doubts, I would correct it”. Allowing him to solve the problem himself and proposing to improve the code is much better than giving instructions to correct imperfections. Eric Dietrich, "Use the code review for execution"

Poorly:

• “You didn’t follow the standards here.”
• “A is wrong, must be used.”
• "Confused Code"
• “You haven't initialized these variables.”

Good:

• “Why did you choose this solution here?”
• “Why did you use A instead of B?”
• “This part is incomprehensible to me. Can you explain?"
• “I did not find where these variables are initialized”

Avoid the accusatory "why." Like the statements, “why” questions can also sound accusing , so without them it would be better.

Poorly:

• “Why didn't you follow the standards here?”
• "Why are you just not ...?"

Good:

• “For what reason did you decide to move away from standards here?”
• “What guided you when ...?”

Be more modest

Ask questions, but do not ask. Instead of telling the author what changes are needed, ask him about the code or make a suggestion and ask the author what he thinks about it. So you pass the ball to his side of the field and show respect for his free will, giving the author a chance to explain his decision or to say whether your proposal is useful for the code.

Instead of “Let's call this variable username, because otherwise it is not clear,” it is better to rephrase the sentence in the form of a question: “Maybe, call it username? The current name does not look too clear to me, because it is also used in a different context in someotherfile.js. ” Daniel Bader, “7 Ways to Avoid Deteriorating Relations When Reviewing a Code”

When you offer something, be sure to explain the reason why this change can improve the code.

Use personal examples. Show the author of the code that not only did he make such an error. This is a great way to soften criticism: “It was hard for me to do it myself ...”, “I did the same”.

Not every question needs to be answered. Accept for yourself immediately that not every your question the author of the code should give an answer . So you can ask questions in your review that make you think. They do not have to be answered in order to infuse the code into the code base, but on the other hand they push the developer’s thought and improve the quality of its work in the long run.

Do not break the rule of "no surprises"

Use the checklist:

Surely each member of your team repeatedly makes the same ten mistakes. And the most difficult to detect omissions of some elements. So the checklist is the easiest way to avoid the most common mistakes and reduce the likelihood of omissions. SmartBear, "Best Code Review Techniques"

Of course, the checklist cannot cover all possible cases. But it’s enough to include in the good list all the requirements that a pull request must satisfy in order to be merged with the code base. This is a useful tool for both the coder and the reviewer. The checklist can help improve the code, make it more consistent, avoid violating rules and standards. It will be very useful for new members to take notes of the code requirements before sending their first pull request and conducting the first audit.

An example of a checklist from which to start:

• Your branch should contain one logically separate operation and no unrelated changes.
• You must have quality commit messages. See < commit-> .
• Your thread should contain new or changed tests for all new or changed code. All tests must be performed in a branch. See < > .
• Your thread should contain new or updated documentation for all new or updated code. See < > .
• Your branch should be relevant to the master branch and merge without conflicts, so make a re-raise to pull request.
• All new code must meet our accepted architectural requirements and guidelines for Python, JavaScript, HTML and CSS styles. See < > .
• If the new code contains changes to the database schema, then it should include the database migration. See < > .
• If the code contains changes that violate backward compatibility with plug-ins, API-clients or themes, then how necessary are such changes? Is the effect of such changes large enough for non-compatibility? Has the change list been rejected for backward compatibility?
• Does the new code add any dependencies (for example, new third-party modules are imported)? If so, were new dependencies evaluated, were they added in accordance with the correct procedure? See < > .
• Has the code been tested with working data? See < > .
• If the user interface has changed, was it tested on screens of different sizes and in different browsers? See < > .
• If the user interface has changed, does it meet the accessibility requirements? See < > .
• If new string variables appeared that were visible to users, were they internationalized? See < > .

Use extensively documented programming standards:

Programming standards are a common agreement between developers . A set of mandatory for all recommendations for writing high-quality and easy-to-maintain code. Standards are the foundation of the review , because during the review we check the code for compliance with the standards. Instead of making non-standard requirements for the code, request to be included in the list of recommendations.

Without a reference project of standards available to the whole team, developers may not even understand where problems will be revealed during the next review . And this does not add to the effectiveness of their work.

Standards must be exhaustive. You can rely on the formatting style of the PEP 8 code, but this is not a complete standard for use during the review.

Analyze what you need, and the rest will take tools

During the review, there should almost never be any problems with formatting the code . Audits should provoke the emergence of productive thoughts and discussions, and spending time on style and formatting will not help. For such things, use tools like linters and automatic formatting tools.

Conclusion

This article is about how to give positive conclusions and correctly criticize how to successfully make suggestions during the review of the code. I suggest you make a condensed version of this guide with key points highlighted and distribute it to each team member. Put the instructions in the general repository, so that anyone can begin its analysis and suggest their changes, and then the whole team will be involved in the discussion.

I also recommend thinking about how you create code, which then gets on the review. You probably want the coder and the reviewer to have similar ideas about technical design before the code is reviewed. Decide in advance who will write the code for a particular feature, and who will review it, and encourage them to work together so that the two will discuss the structure of the code and its intermediate versions before checking out the final version. We need a high-quality architecture (two heads are better), but we also need to avoid unnecessary debates during the analysis, when both developers offer completely different solutions (one of them will have to completely rewrite the ready-made code).

If, before the review, a strong cooperation is established between the author and the reviewer, then during the review, most likely, only a few minor issues will emerge.