评审你不喜欢的代码的 10 个友情提示 未翻译

oschina 投递于 07/10 18:19 (共 10 段)
阅读 592
收藏 1
3
加载中

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.

已有 6 人翻译此段(待审批)
我来翻译

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 reallyimpossible?)
  • Good: “How can we do XXX with your change?”
已有 7 人翻译此段(待审批)
我来翻译

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?”
已有 7 人翻译此段(待审批)
我来翻译

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 人翻译此段(待审批)
我来翻译

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).”
已有 4 人翻译此段(待审批)
我来翻译

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.”
已有 5 人翻译此段(待审批)
我来翻译

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.”
已有 5 人翻译此段(待审批)
我来翻译

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?”
已有 6 人翻译此段(待审批)
我来翻译

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.

已有 5 人翻译此段(待审批)
我来翻译

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.

已有 7 人翻译此段(待审批)
我来翻译
本文中的所有译文仅用于学习和交流目的,转载请务必注明文章译者、出处、和本文链接。
我们的翻译工作遵照 CC 协议,如果我们的工作有侵犯到您的权益,请及时联系我们。
加载中

评论(1)

我是可乐
作为开源项目(包括Red Hat内部和外部)的频繁贡献者,我发现最常见的浪费时间的行为之一是处理提交代码的代码评审,这些评审是负面的或阻碍的,但本质上是主观的或争论性的。当提交到维护者不喜欢更改的项目时,无论出于什么原因,我经常看到这种情况。在最好的情况下,这种代码评审策略可能导致在无意义的争论中浪费时间;在最坏的情况下,它会阻碍项目的贡献和多样性,并创建一个充满敌意和精英主义的环境。

代码评审应该是客观和简洁的,并且应该在可能的情况下处理确定性。这不是政治或情感上的争论;这是一个技术问题,我们的目标应该是不断前进,提升项目及其参与者的水平。变更提交应始终根据提交的优点进行评估,而不是根据提交人的意见。

代码评审的策略

当你(作为一个项目维护者)在评审提交的文件时,不管出于什么原因,你都不喜欢以下几种策略:

1. 把你的反对意见重新表述为一个问题

坏:“这个改变将使XXX不可能。(这有些夸张;reallyimpossible吗?)

好:“我们怎么用你的零钱做XXX ?”

2. 避免夸张

简单地陈述你的担忧,并提出问题来帮助达到预期的结果。

坏:“这个改变会破坏性能。”

好:“看起来做X可能比现有的Y慢;你有没有测量/收集数据来证明它不是?”

不如说:“与此同时,我正在收集数据,试图验证X是否比y慢。”

还好:“这个变化改变这单回路O (n)双重嵌套循环O (n²);这不会影响性能吗?”

3.对自己冷嘲热讽

有些想法最好不要告诉别人。如果你没有礼貌,就不要参与。

坏:“我认为这种改变是不好的,会毁掉一切。”

坏:“你确定软件工程是适合你的职业道路吗?”

4. 积极参与

也许你对如何解决问题有不同的想法?如果你积极参与,你可能最终会发现一个比最初的选择更好的解决方案。

坏:“这个改变糟透了,我的版本更好。”

很好:“我在这里也有类似的变化,也许我们可以比较和/或结合想法。”

同样好的:“我有一个类似的变化在进行中,但我选择做X是因为ZZZ;你为什么选择Y?”

。记住,不是每个人的经历都和你一样

一个完全有能力的工程师可能会在不知道一些事实的情况下工作好几年,而你却把这些事实当作常识。说一些显而易见的事情是没有关系的,只要你不是在屈尊俯就,也不是在暗笑。

坏:“难道你看不出来这明显是错的吗?”

Good:“这是不正确的,因
返回顶部
顶部