10 tips for reviewing code you don't like

As a frequent contributor to open source projects (both within and beyond Red Hat), I find one of the most common time-wasters is dealing with code reviews of my submitted code that are negative or obstructive and yet essentially subjective or argumentative in nature. I see this most often when submitting to projects where the maintainer doesn't like the change, for whatever reason. In the best case, this kind of code review strategy can lead to time wasted in pointless debates; at worst, it actively discourages contribution and diversity in a project and creates an environment that is hostile and elitist.

A code review should be objective and concise and should deal in certainties whenever possible. It's not a political or emotional argument; it's a technical one, and the goal should always be to move forward and elevate the project and its participants.  A change submission should always be evaluated on the merits of the submission, not on one's opinion of the submitter.

Code review strategies

Here are several strategies to keep in mind when reviewing submissions that, for whatever reason, you (as a project maintainer) do not like:

1. Rephrase your objection as a question

• Bad: "This change will make XXX impossible." (This is hyperbole; is it really impossible?)
• Good: "How can we do XXX with your change?"

2. Avoid hyperbole

Simply state your concerns and ask questions to help get to the desired outcome.

• Bad: "This change will destroy performance."
• Good: "It seems like doing X might be slower than existing Y; have you measured/gathered data to show it isn't?"
• Better (if you have time): "In the meantime, I am gathering data to try to verify that X is not slower than Y."
• Also good: "This change changes this single loop O(n) to a doubly nested loop O(n²); won't this affect performance?"

3. Keep snide comments to yourself

Some thoughts are better kept to yourself. If you can't be civil, don't engage.

• Bad: "I think this change is bad and will ruin everything."
• Bad: "Are you sure that software engineering is the right career path for you?"

4. Engage positively

Maybe you had a different idea about how to solve a problem? If you engage positively, you might end up discovering a solution that is better than either original option.

• Bad: "This change sucks, my version is better."
• Good: "I also have a similar change at this location XXX: maybe we can compare and/or combine ideas."
• Also good: "I have a similar change in progress, but I chose to do X because ZZZ; why did you choose Y?"

5. Remember that not everybody's experience is identical to yours

An otherwise completely competent engineer could go for years without knowing some fact that you take as common sense. It's okay to state the obvious, as long as you aren't patronizing or snide about it.

• Bad: "Can't you see that this is obviously wrong?"
• Good: "This is incorrect because it causes a null pointer exception when X is Y."

6. Don't diminish the complexity of something that's not obvious

Remember that things that are obvious to you may not be obvious to everyone. Suggesting alternative approaches and pointing out useful examples can help get everyone on the same page.

• Bad: "Why not simply frob the gnozzle?"
• Good: "It might be possible to frob the gnozzle, which would simplify this part (see XXX for an example)."

7. Be respectful

Sometimes a submission just doesn't meet a minimum standard for quality. It's okay to say so, but it doesn't cost anything extra to be respectful.

• Bad: "This is stupid code written by a stupid person."
• Good: "Thanks for your contribution. However, it cannot be accepted in its current form; there are multiple problems (as outlined above)."
• Also good: "As outlined above, there are multiple problems with this submission.  Maybe we could back up a step and talk about the use cases instead?  That could help us find a path forward."

8. Manage expectations (and your time)

If a submission is too large to be reasonably reviewed, it is okay to let the submitter know right away. Keep moving forward.

• Bad: "I'm not merging this, it's too big."
• Also bad: Ignoring it until it goes away.
• Good: "Could you please break this down into smaller changes? I do not have a lot of time for code reviews and this one is just too large/complex to review in one pass."

9. Say please

Just saying "please" goes a long way toward showing that you respect the submitter's time, especially when you want something to be different due to formatting or style, which might seem to be a minor detail of the change. Examples:

• "Could you please separate the whitespace changes into another pull request?"
• "Could you please align these variable definitions so they're 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 have to just live with it. But it's also okay to say, "I don't like this and I'm not sure why, can we talk about it?" It's a reasonable thing to ask, and even though it might take a little time, it's often worth the investment because now you have two people who are both learning (one by explaining and one by listening) rather than two people who are opposed to each other.

Even skilled and experienced engineers should be able to say "I don't understand why I don't like this"; it's not an invitation to attack the position of the reviewer but rather an honest quest for knowledge.

Summary

Avoid hyperbolic or bombastic assertions, avoid argument strategies, avoid elitist or demeaning language, and avoid constructs like "obviously" and "why don't you just...".  Use clear, factual statements and supportive language, ask questions, and move things forward.  Remember that coworkers and contributors are human people, and their time is worthy of the same respect as yours.