Cod-review often generate controversy. While preparing the lecture “We are disaccustomed to toxic behavior on code review” at the AlterConf conference , I was ready to hear a lot of objections and criticism. But she did not expect the community to support the idea so much. I assumed resistance, but the community accepted me very favorably and with approval.

I was asked to share the slides , but now I thought that the slides themselves were of little help and were taken out of context: they lacked explanations. Therefore, I decided to publish this article. Later, the conference organizers posted a video .

Listed below are some types of unproductive code review behavior and recommendations on how to make the process more benign and eliminate toxicity. All behavioral models are tested either by me or in a company I know. In the past, I also had such sins.

Unproductive behavior number 1: the transfer of opinion as a fact

Do not make statements if you cannot refer to the documentation, formal recommendations, and code examples to support these statements. People should know why they are asked to make changes, and the personal preferences of another developer are not a good enough argument.
')
Instead of saying:

This component should be made without a change of state (stateless).

... better provide some context:

Since this component has no life cycle or state methods, it can be made stateless. This will improve the performance and readability of the code. * Here is some documentation.

The transfer of opinion as a fact often occurs when discussing style and syntax. These are really important topics, but not for code review, because style and syntax have nothing to do with the problem that the developer initially resolves.

Such discussions are better to lead separately and determine the style for the whole team. Implement a linter and automatic code fixes . Then you will refer to these recommendations for style, and not to your personal opinion.

It is especially important not to give your opinion as a fact if you have a higher rank and authority in a team or company. The developer has the impression that he has no choice but to obediently fulfill your requirements.

Unproductive behavior number 2: an avalanche of comments

When a person makes a mistake, chances are high that this error occurs in several places. I noticed that reviewers sometimes indicate each of the cases instead of leaving one detailed note with links to useful resources.

Consolidation of comments allows you to send the same message without suppressing the author. An avalanche of duplicated comments on one issue looks like a nagging.

Unproductive and overwhelming:



Multiple comments for one recurring error (spaces at the end of the line)

More useful option:


Consolidated feedback

I can understand that it is sometimes useful to point out each place of an error, since the comment disappears when corrected in subsequent commits. But if the error occurs many times, it is obvious that the developer did not know about a particular manual, and he should simply point out the right resources.

Unproductive behavior number 3: ask the engineers to solve other people's problems, "while they are here"

Do not ask developers to deal with problems that are not directly related to the changeset or the problem this code is trying to solve. Even if a developer extends or modifies a dirty code with an abundance of bad practices, do not ask him to clean and fix this alien code.

I do not propose depriving developers of responsibility for the code just because they did not write it initially. In fact, it is not good to say something like "Alien code is not my problem." It's just more appropriate to create a separate ticket and pull request to fix the dirty code. Thus, you avoid a sharp increase in the volume of someone's work, solving the problem with technical debt.

TL; DR : Do not ask the developer to fix the problems while they are here. If the code does what is required in the ticket and does not introduce new problems into the code base, approve it and then create a ticket to clear the code base.

Unproductive Behavior # 4: Ask Condemning Questions

Avoid asking judgmental questions like this:

Why didn't you just ____?

This implies that some simple solution should be obvious. This makes the developer defend himself.

Often these condemning questions are merely veiled demands. Instead, offer a recommendation (with documentation and links), omitting harsh words.

You can make ____ that has the advantage of ____.

Unproductive behavior number 5: sarcasm

In the reviews there is no place for sarcasm. As a rule, sarcastic comments do not provide context and effective feedback. Instead, describe the problem in detail and provide recommendations, but leave aside caustic jokes.

Unproductive:

Have you even tested this code before shipping?

Useful:

Failure occurs when entering a negative number. Could you please do this?

Here is another example of a comment in the code review , which is neither funny nor useful:

We are not angry, we are merciless. As you can see, I wrote “beep!” On the import of each file you touched. I meant the following: “Your import violates our standard convention, which implies a certain order: first, the built-in modules, then third-party, and then the project level,” but this is too long a phrase to type it in each case.

In the given example, “beep!” Is not at all useful and not meaningful. This is pedantic humor that does not help the author.

Unproductive behavior number 6: Emoji instead of describing the problem

When pointing out problems in the code, avoid emoticons with a thumb down or emoji with a nauseous little man. It is as useless as sarcasm, for the same reasons. Emoji is mysterious, easily misinterpreted. People waste time trying to understand your thoughts.


Do not use emoji to indicate coding problems.

In any case, you should not have an emotional reaction to programming errors.

It is quite normal to use positive emoticons, such as “thumb up” or “hurray!”, But not to indicate problems, but as a tip on a good code.


Approving emoticons are great

Unproductive behavior number 7: do not respond to all comments

The authors also contribute to the toxicity of the discussion. If you combine the code without answering all the comments, it is surprising to the person: he tried to help you, and you make it clear that some of the reviews do not matter.

If the comment is not relevant or you will not take action, just briefly explain why.

Do not ignore colleagues.


Combining code without responding to a comment

Unproductive behavior number 8: ignoring toxic behavior, if it comes from the best professionals

Toxic behavior should not be ignored or underestimated solely because the developer is an excellent professional and extremely productive employee. Although it does a fantastic job, it is important to keep in mind that its toxic behavior leads to stress and burnout of other developers.

Experience with a person who exhibits toxic behavior:

Others will find that working with this person is draining and demotivating. They will do anything to avoid interacting with it, even if it adversely affects their ability to perform tasks. Communication will be disrupted throughout the organization, and employees will start looking for job openings elsewhere. While you are faced with the consequences of talent leaks and unsuccessful projects, this particular developer will continue to work calmly, as if nothing had happened. - Joseph Gefro

If you do not take steps to remedy the situation, there may be serious consequences: developers will be overloaded, attacked and demotivated. They will be afraid of feedback, which should actually help them grow.

I personally felt a great deal of anxiety whenever I received an email with comments on my pull-request from a former colleague (known for his toxic, low-helpful reviews). Although toxic behavior at all affects differently, but definitely no one benefits from this toxicity.

Note : I want to make it clear that if the developer could not resist and once showed this behavior, this does not make him “toxic”. We are talking about repeated insults and caustic comments.

Useful code review practices

Below are some recommendations that apply to any normal communication, although here we are talking in the context of programming and code review.

Useful Behavior # 1: Use Questions or Recommendations to Develop a Dialogue.

Never ask people to make corrections or ask condemning questions, because this prevents dialogue between you and the other person.

Why you just did not combine these transformations in the file with constants?

Formally, this is a question, but it does not create a dialogue between you and the interlocutor. He just makes him defend himself. Instead, ask what the interviewee thinks about your decision, for example:

What do you think of pulling these conversions into a constant file? There are quite a few of them, so a separate file may well make sense at the moment.

... or suggest a recommendation:

In this file, you have a lot of translation calls for the "function x". It may make sense to create a separate file with its constants.

Useful behavior number 2: to cooperate, not to hide

When you program together, you have to be near, ask questions, discuss and point to resources.

"... when you want to help or work together, you must fully participate, and not just interfere sometimes" - the leadership of the Recurse Center

Useful behavior number 3: respond to every comment

If you, as an author, do not intend to use the advice of the reviewer, just make a note informing about it. Do not ignore those who spend time to help you.

For example:

Person A: - What do you think about creating an auxiliary function for this API call? Everything else is good.

Person B: - This line was not part of my changeset. I will combine the code, create a separate ticket on GitHub and write it in the work plan for our group.

Useful Behavior # 4: Know When It Is Better To Organize A Personal Meeting

After dozens of comments and proposed solutions, it should be clear that online communication has become unproductive for the issue at hand. Send out a meeting invitation to all involved colleagues.

Thus, the group will be able to quickly make a decision and apply it.

Problems that take hours and tons of comments can usually be solved in a few minutes of productive conversation. - “Neat Java”

Useful Behavior # 5: Take advantage of learning opportunities and don't brag

Before taking part in the review code, ask yourself:

Does your comment really help you learn or are you carping?

Think about your comments. Remember that the purpose of the review code is to teach and help other developers grow. This is not a tribune for speeches.

Useful behavior number 6: do not show surprise

Be careful not to show surprise if a person does not know something. Do not grieve people that they "should already know" this information. Conversely, feel free to admit that you lack experience on the topic - this is a great way to ask for help.

For details, see the “Do not pretend to be surprised” rule from the Recurse Center manual.

Useful behavior number 7: automate everything you can

There is little point in discussing errors that linters, Git hooks, or automated tests can catch. This is a ton of extra comments that look like cavils. People are not very good at identifying such errors, therefore, automation tools have been created.

There are also tools to automatically run tests when adding new code. They display warnings if a changeset violates one of the tests. For example, TeamCity and Jenkins CI.

Also, use Git hooks to run tests and linters on the new code. They intercept a commit if they find bugs.

Let the tool indicate the problems so that people do not have to do it.

Useful behavior number 8: do not adopt toxic behavior

No need to adopt the toxic behavior in their code review, because it is like revenge or some kind of hazing for new developers.

Find colleagues who will support you.

If you notice unproductive code review, try to inform a colleague, speak directly and honestly (if you feel safe in your position / company).

Useful behavior number 9: managers, carefully consider the candidates, listen to the team and apply their powers

Managers have great opportunities to create a positive and supportive atmosphere in the team.

Let's rephrase the tips from the article "Harm Toxic Developers" :

• Prevent the appearance of toxic developers in the team. When hiring pay attention not only to technical training. Find out how the candidate collaborates and communicates. Critically analyze his work and see how he reacts. Make sure that every candidate improves the culture of the company, and does not just fit into it.
  
• When a toxic developer still got to the state or was inherited, ask each employee for an opinion about it in a personal conversation. If the developer is toxic, it will pop up in reviews about him.
  
• Talk to the toxic developer. Give specific examples of effective comments in code review. Continually work with him, continuing to collect information in personal conversations with his colleagues and manager.
  
• Do not isolate a toxic developer. (*)
  
• Repeat to staff about the need for a friendly environment.

(*) Although the article proposes isolating toxic developers, I find it important to encourage the toxic developer to cooperate with the team, but in healthier ways. Isolation does not solve the problem. If you give it a separate job, toxicity will decrease in the short term, but the developer will not give up his unproductive behavior. He will only lose the podium to demonstrate it.

Useful behavior number 10: set the standard while the team is small and growing

Small teams are able to accept new ideas and implement them. Even if you do not consider it necessary to set standards, because now there are no problems, but it is important to maintain good cooperation practices when replenishment comes.

Useful behavior number 11: understand that you can be part of the problem

To create a more supportive environment, it is important to be honest with yourself and reflect on any ineffective behavior.

Being a junior, I noticed a mistake in someone's code several times and was happy, because I could leave a comment! Now I understand that I used this opportunity to show off, not help. You need to carefully evaluate your actions.

I think that it is useful for everyone to take time for this difficult self-analysis.

And the last ...

We are not talking about the content of the reviews, but only about the tone

I know that the reviews are important, and we are not fighting against them here. I do not ask to compromise the quality of the code. A well-meaning code review culture and high quality code should not be mutually exclusive.

I just hope that people will take steps to provide constructive, effective feedback and create more favorable conditions in which developers feel comfortable, learn, grow and not be afraid to make mistakes. We can all improve together.