I constantly commit to open source projects (Red Hat and others). And he noticed that the most negative time was taken by the code review, subjective in essence. Most often this happens with commits, where the maintainer for some reason does not like your change. At best, such a code review strategy leads to a loss of time in meaningless disputes; in the worst case, he actively impedes kommit, creating a hostile and elitist environment.



The revision code should be objective, concise and, if possible, contain only certain facts. This is not a political or emotional dispute, but a technical one. His goal is to move forward, develop the project and all participants. Any commit should be evaluated only on the merits, not on the subjective opinion.

Code Review Strategies

Here are some strategies to keep in mind when reviewing the code that you (as a maintainer) for some reason do not like:

1. Rephrase the objection in the form of a question.

• Poorly: “This change will make XXX impossible” (This is an exaggeration; is it really impossible?)
  
  
• Good: “How are we going to do XXX after your change?”

2. Avoid exaggerations

Simply express your concerns and ask questions to help achieve the desired result.

')

• Bad: "This change will destroy productivity."
  
  
• Good: “It seems that X may be slower than the existing Y; Did you take measurements / collect data to show that it is not? ”
  
  
• Better (if you have time): “I am still collecting data. I'll try to check that X is not slower than Y ”.
  
  
• Too good: “This change changes a single O (n) loop to a double nested O (n²) loop; Will it affect performance? ”

3. Keep snide comments with you

Some thoughts are better to keep with you. If you can not say politely, better keep silent.

• Bad: “I think this is a bad change that will ruin everything.”
  
  
• Bad: “Are you sure that software development for you is the right career choice?”

4. Act positively

Maybe you had a different idea on how to solve the problem? If you act positively, you will eventually find a solution that is better than any of the original options.

• Bad: "This change sucks, my version is better."
  
  
• Good: "I also have a change for this place: perhaps we can compare and / or combine ideas."
  
  
• Too good, “I have a similar change in my work, but I decided to make X, because ZZZ; why did you choose Y? "

5. Remember that everyone has different experiences.

In all other respects, an absolutely competent engineer may not know certain facts for years that you take to be common sense. There is nothing wrong with saying the obvious thing, unless you are patronizing or maligning.

• Bad: "Don't you see that this is clearly wrong?"
  
  
• Good: “This is incorrect, because it causes the exception of the null pointer when X is equal to Y”.

6. Do not diminish the complexity of what is not obvious.

Remember that things that are obvious to you may not be obvious to everyone. By offering alternative approaches and pointing out useful examples, you will help everyone synchronize.

• Bad: "Why not just gobble up the gnomer?"
  
  
• Good: “If you gropnat the gnome, then this part can become easier (see XXX for an example).”

7. Be respectful

Sometimes a commit just doesn’t meet the minimum quality standard. It is quite normal to say this, but to show respect does not require additional efforts.

• Bad: "This is a stupid code written by a stupid person."
  
  
• Good: “Thanks for your input. However, it cannot be adopted in its current form; There are many problems (as stated above). ”
  
  
• Too good: “As stated above, there are several problems with this commit. Maybe take a step and talk about usage scenarios? It will help find a way. ”

8. Manage expectations (and your time)

If the commit is too large and cannot be considered quickly, then it’s ok to say so immediately. Then look for a solution.

• Bad: "I do not accept it, it is too big."
  
  
• Too bad: ignore commit until it disappears.
  
  
• Good: “Could you break it into smaller commits? I don't have too much time for code review, and this is just too big / complex commit for one review. ”

9. Say "please" (be polite)

Just saying “please” you show that you respect the sender’s time, especially when you want to change the formatting or style, which may seem like a minor change. Examples:

• “Could you make a change in the gaps in a different pull request?”
  
  
• “Could you align these variable definitions to make them easier to read?”

10. Start a conversation

If after all this you still don’t like something, but you’re not sure why you might just have to accept it. Or to say: “I don’t like it, but I’m not sure why, can we talk about it?” This is a reasonable question, and even though it may take a little time, it is often worth it because you now have two people, and both learn (to explain and listen), not two people who confront each other.



Even qualified and experienced engineers should be able to say, “I don’t understand why I don’t like it.” This is not an invitation to attack the position of the reviewer, but rather an honest search for knowledge.

Summary

Avoid hyperbolic or pompous statements, avoid disputes, elitist or degrading expressions and constructions like "obvious" and "why would you just ...". Use clear, factual statements and supporting language, ask questions and move forward. Remember that colleagues and contributors are people too. Their time deserves the same respect as yours.