【外评】代码审查反模式

代码审查似乎是个不错的主意,对吗?两个开发人员看同一段代码意味着有两次发现问题的机会;它可以传播对项目发展方式的理解;审阅者可以从详细阅读作者的代码中学到有用的技巧,或者发现向作者传授他们还不知道的有用技巧的机会。

但这只适用于 “轻量级 ”代码审查员,他们利用代码审查来改进代码库和开发人员的集体技能。代码审查也可以是一个强大的工具,用于完全不同的目的。当代码审阅者转向黑暗面时,他们有很多方法来阻碍或延迟代码的改进,惹恼补丁作者或完全打击他们的积极性,或者追求自己的其他目标。

如果您最近才转投黑暗势力,您可能还没有想到所有的可能性。因此,这里列出了一份代码审查反模式清单,供黔驴技穷的黑暗面代码审查员参考。

反模式

千回百转之死

你开始阅读代码。一旦发现有瑕疵,就添加审核注释指出来。然后你就停止阅读。

开发人员认真修正了你的瑕疵,并提交了修订补丁。

你开始阅读该版本。你又发现了一个独立于第一个版本之外的瑕疵。你完全可以在第一次就提到它,但你没有,因为你还没有读到那一步。于是你指出了这一点,并再次停止阅读。

以此类推。反复进行,直到开发人员失去希望。

如果你所在的时区与开发人员所在的时区相差甚远,你自然会得到 24 小时的往返时间,这样补丁就会尽可能缓慢地发展。如果您所在的时区与开发者相近或相同,那么为了适当延迟补丁的发布,您就必须安排忙于其他几件事情,这样您就有了一个很好的借口,可以花上一两天的时间来查看每个新版本。

抵制诱惑,不要说’现在看起来好多了’之类的话。如果你暗示自己越来越满意,就会给他们继续尝试的理由!

赎金说明

对于提交补丁的开发者来说,这个补丁似乎特别重要。(也许他们直截了当地说,这是他们说服您接受补丁的理由之一。或者你只是读懂了字里行间的意思)。

但这个补丁对你来说并不是特别重要,所以你处于有利地位!现在,你可以挟持他们需要的修改,直到他们做了很多额外的切题相关工作,这些工作其实不需要在同一个提交中,但对你很重要。

如果你还想再见到你心爱的补丁……

双人团队

一个补丁。两个审查员。

每次你们中的一个人提出修改要求,开发人员就会顺从地进行修改,而另一个人就可以抱怨了!

继续轮流提出不相容的要求。但一定要直接向开发人员提出意见。避免在审核主题上直接争论,也不要接受补丁作者的任何建议,即你们应该互相讨论,就补丁应该是什么样子达成共识。

看看在开发人员放弃之前,你们能来回乒乓多少次。

(在紧急情况下,如果找不到共犯,可以尝试反驳自己之前的审核意见。但通常会有人注意到。如果有两个评论者,效果会更好)。

猜谜游戏

有一个问题,有很多可能的解决方法。开发者选择了一种解决方案并提交了补丁。

基于一些与是否解决问题无关的理由,对该解决方案提出批评。让你的批评含糊不清:也许是违反了晦涩难懂的设计原则,也许是与同一领域未来开发的某些虚无计划不兼容。尽量让批评与他们真正想要实现的目标无关。

如果你说得足够含糊,开发人员就想不出任何方法来调整他们现有的补丁,而他们又有信心能解决你的批评。他们最好的办法就是选择一个完全不同的解决方案来解决原来的问题,并尝试实现它。

所以,现在你也可以用同样无益的方式把它打倒了!

不要让开发人员把你困在 “好吧,你觉得这个问题应该怎么解决?”这样的对话中,也不要给他们任何你会满意的解决方案的暗示。他们迟早会失去继续猜测的意愿。

优先级倒置

在第一次代码审查中,挑一些简单的小毛病。变量名有点不清楚;注释中有错别字。

等开发人员解决了这些问题,你再抛出你的重磅炸弹:这个补丁有一个更根本的问题,需要彻底重写一大块内容–这就意味着你已经让开发人员在补丁的这部分内容中做了很多吹毛求疵的修正,而这些修正都被丢弃了。

没有什么比让开发人员做了大量工作后又让他们扔掉这些工作更能说明 “你的工作不被需要,你的时间不被重视 ”了。这本身就足以让开发者放弃。

姗姗来迟的设计审查

一项非常复杂的工作已经进行了数周或数月,分成了许多独立的补丁。一半的工作已经完成。

你个人并不同意整个工作的设计,但在最初的讨论中并没有征求你的意见。或许你参与了,但你是辩论中失败的一方。

但现在,你被要求在中间审查一个次要但重要的东西,比如一个简单的 bug 修复,而这个 bug 却阻碍了很多开发人员。这就是你的机会!

要求对迄今为止的整个工作设计进行论证。如果开发人员不知道答案,因为他们只是在做整个工作的一小部分,那就耸耸肩–这不是你的问题,在你满意之前,你不会点击 “批准 ”按钮。

如果你真的很幸运,也许你还可以通过一个貌似合理的理由,让开发人员撤销一些已经完成的工作。小心避免让他们知道在哪里可以找到与你意见相左的设计讨论。

自相矛盾

如果只有一个大补丁,那就太难读了!要求将其分割成独立的子块。

另一方面,如果有很多小补丁,那么有些补丁本身就没有意义!因此,坚持要求将它们重新粘在一起。

对于任何一部足够大的作品,你都能找到理由提出这样的抱怨,只是要决定是哪一种。

不一定非得是 “拉取请求中补丁的数量”。您可以根据具体情况权衡利弊。例如,如果代码写得清晰易读,那么它的性能就可能差得令人无法接受;如果代码优化得很好,那么它就很难读,也很难维护。

翻转

人们通常会对代码的同一部分进行一种修改,比如在列表中添加额外的内容。您之前已经查看过几次这样的修改。另一个修改刚刚出现:作者已经看过之前的修改,仔细遵循了现有的模式,并选择你作为代码审查员,因为你显然已经习惯了这一领域。

如果你突然对以前从未遇到过问题的修改提出异议,就会让所有人都紧张起来。遵循现有模式是不够的!作者应该预料到你最近改变了主意,不知道这种变革应该是什么样的。

如果作者指出了你的前后矛盾,并展示了你没有提出反对意见的前三次相同的修改,你会怎么做?

那你就说’你说得对,那些也应该改。注意不要承担任何个人责任–你不是自愿修改所有现有案例的。如果你真的很幸运,开发人员会把这句话理解为他们自己要修改现有案例的指示,这样你就省去了很多精力。

不过说真的,朋友们……
到这里为止,这篇文章就是个笑话。

(我希望我根本没必要这么说,但我肯定会有人误解的)。

我并不是真的想鼓励代码审查员变得烦人和碍事。我甚至也不是真的建议代码审阅者做所有这些坏事,至少是故意的,至少在大多数时候是这样。上述描述大多是对审阅者真实行为的夸大。或者甚至不是这样:只是夸大了当你是一个沮丧的补丁提交者,而你的补丁已经被审核了几个星期,并且感觉好像没有任何进展时的感觉。

我的意思是:不要做这些事情!尽量减少往返次数,而不是增加往返次数。在挑剔所有琐碎的小毛病之前,先要求重要的改写(如果需要)。当您批评补丁时,请就您愿意接受的版本提出建设性建议。如果您对整个代码库有意见,请在一般性讨论中向所有开发人员提出,而不是对您被指派审查其补丁的开发人员小题大做。如果您不小心做了这些事情,请有一些自知之明:注意到您的错误给开发人员的生活带来了额外的困难,向他们道歉,并尽量提供额外的帮助以弥补过失。(或许还可以帮助改进实际代码,比如编写自己的修订版补丁,或在自己的后续补丁中进行额外的润色)。

权力

如果说这里有什么严肃的观点的话,我认为是这样的。当一个开发人员成为另一个开发人员补丁的代码审查员时,这种关系就产生了临时权力。审核员有权阻止该补丁的提交,即使他们在其余时间对补丁提交者没有任何权力。

有权力就有责任。你应该只将权力用于预期目的,而且只在必要时使用。在这种情况下,就是根据整个开发团队对 “好 ”的定义,尽可能使补丁变得更好。

因此,这些反模式(或它们所讽刺的轻微行为)大多是滥用权力。审阅者正在利用对另一位开发人员的临时权力作为筹码,以实现其他一些个人目的,这些目的可能与补丁的好坏无关,也可能与补丁的好坏截然相反。

这些反模式中的个人目的各不相同。它可能是审阅者所支持的某项无关的工作;可能是审阅者的个人风格偏好,而团队中的其他成员却无法采纳;可能是懒惰,利用这个机会写下你想要的单行描述,而让别人去做艰苦的工作;也可能只是单纯地抵制变革,甚至可能是对补丁提交者的个人厌恶。

如果补丁提交者在上一次轮到他们担任代码审阅人时有过这些不良行为,那么这种不喜欢可能是有道理的!这也是您应该慎用代码审查员权力的另一个原因。如果开发人员陷入了互相报复的怪圈,那么你的软件项目就注定要失败。

把关代码审查

到目前为止,我一直在关注同行代码审查。代码审查员和补丁提交者是同事,谁也管不了谁,谁也没有对整个代码库的最终决定权。这就是为什么你在代码审查中获得的权力是暂时的:明天,这个补丁打完之后,你就不再拥有它了。后天,情况可能会相反。

另外,在同行评审的情况下,代码评审者和作者的目标应该是基本一致的。如果双方对哪些功能应该被写入代码,或者某些东西在批准前需要打磨到什么程度,或者可接受的编码风格是什么等问题存在严重分歧,应该在代码审查之外解决。

但在其他类型的代码审查中,情况就不完全是这样了。特别是,如果您的软件项目的外部人员主动向您发送了一个补丁,情况就大不一样了。

(这种情况通常出现在自由软件中,因为世界上任何人都可以修改你的源代码,其中一些人还会尝试把修改的内容发回给你)。但这种情况也可能发生在其他情况下–在一家开发专有代码的公司里,如果一个团队的开发人员特别想要另一个团队还没有找到的功能或修复方法,他们可能会碰运气向另一个团队的项目发送补丁)。

在这种情况下,补丁接收方不愿意接受补丁的可能性就大得多了,因为他们要么认为本来就不应该进行修改,要么完全不同意修改的方式。在这种情况下,这并不是滥用作为同行评审员的临时权力,而是合法行使作为项目负责人或团队的永久权力。这是我的软件项目–我有权决定它的发展方向。

当代码审阅者处于这种 “把关 ”角色时,以补丁违反了现有的一般设计原则或要求(猜谜游戏)为由对其提出批评并不总是错误的,但却不建议如何更好地解决同样的问题。也许我不知道如何才能以符合要求的方式解决这个问题。事实上,也许这正是难点所在,也是我自己还没有做出同样改变的唯一原因!

但即使是在把关的情况下,不加解释地部署 “猜谜游戏 ”也是不礼貌的。当我这样做时,我通常会试图解释开发人员是如何忽略了困难的部分,如果我自己不知道答案,我也会说出来。

当然,像 “千回百转之死 ”那样的消极攻击性阻挠也是无可厚非的。如果你真的不希望代码中出现这个补丁,而你又是把关人,有合法权力做出这个决定,你可以用语言表达出来,这样提交者就不会再浪费时间了!

免责声明

为了写这篇文章,我已经收集了很多年的笔记,这些笔记来自我参与过的代码评审(双方)、我观察过的其他人之间的代码评审以及我只在谈话中听说过的代码评审。

因此,本文并非针对最近审查过我代码的任何特定人员!

本文文字及图片出自 Code review antipatterns

你也许感兴趣的:

共有 122 条讨论

  1. > 优先级倒置

    > 在你的第一次代码审查中,挑一些简单的小毛病。变量名有点不清楚;注释中有错别字。

    > 等待开发人员修复这些问题,然后扔出你的重磅炸弹:这个补丁有一个更根本的问题,需要彻底重写一大块内容–这意味着你已经让开发人员在这部分补丁中做了很多吹毛求疵的修复工作,但最终还是要放弃。

    我已经不止一次这样做了,而且每次都感觉非常糟糕。问题在于,我很擅长在阅读代码时发现小问题。但我不善于发现更大的设计问题,直到我后来思考时才发现

    1. 这就是为什么我喜欢 Github 在发布完整评论之前添加评论的功能。你可以先做一轮吹毛求疵的评论,然后花一些时间让其他的评论沉淀下来,再回来重新审视全局。然后,你还可以添加这些评论,如果一些吹毛求疵的评论不再相关/重要,还可以将其删除,等等。

      1. Bitbucket(对于我们这些还在使用 Atlassian 的人来说)现在终于有了这项功能。

    2. 公平地说,当代码非常凌乱时,通常很难找出重要的问题。我不知道这是否只是我的大脑有问题,但有时在我清理完一段代码后,我的理解会提升到一个新的层次,让我发现之前似乎完全看不到的问题。这也是为什么我在编程时会在代码格式和结构(如函数提取和内联,以及其他不改变行为的转换)上花费大量精力的原因。

    3. 我有一个同事,他会在同一轮评审中留下吹毛求疵的意见和重磅炸弹。比如 30 条评论,如果你看了最后一条,其中 29 条都是无关紧要的。

      我无法向他解释这个问题… 我只是学会了在开始处理任何问题之前,先通读他的完整评论。除此之外,他是我共事过的最好的人之一,反馈意见也很有价值。

      1. 真的有什么办法吗?只发布重磅炸弹,而不表达自己对事情的看法?很多时候,表达意见的部分比实际的小毛病更重要。

        1. > 只发布重磅消息,而不表达你对事情的看法?

          是的。

          > 通常情况下,表达意见的部分比真正的问题更重要。

          你的意见并不那么有价值。先关注你能提供的最有价值的反馈吧。

          1. 如果你花时间写出了一般的抱怨,那么把它丢掉就不好了。我认为,一个好的折中办法是建立一个系统,允许在评论中发布评论摘要,这样你就可以在前面提到重要的事情。

            1. 这根本不是感觉不好的问题,而是如果他们写了,他们就会在一点一滴中再犯同样的错误。如果有人习惯性地拼错了一个单词,而你认为也许应该正确拼写(或其他修正),那么也许在他们进行大的改写时需要指出这一点。

        2. 这就像

           > 因为 X 的原因,整个程序无法运行,你需要重写所有这些。
            >
            > 将 foo 重命名为 bar。
            > 错字。
            > 缺少空格。
          

          所以啊… 剩下的就不要发了吗?我经常会回到之前的评论草稿中,当我意识到这些评论已经不再重要时,就会对其进行修改或删除。评论不应该是意识流,而应该是适当的书面反馈。

          1. > 评审意见不应是意识流,而应是适当的书面反馈。

            我的建议是不要把它看得太重,并假定评论者是出于好意。

            如果有人给了我反馈意见,让我做了很大的改正,那么我会为自己的疏忽拍拍额头,向他们表示感谢,然后继续我的生活。对其他问题的额外反馈是一种奖励。要求他们花更多的时间审查他们的评论则是白费力气。

      2. 在我看来,两者都很重要,只要明确主要问题,吹毛求疵是真正的吹毛求疵(因此可以接受的数量有限,不会阻碍修改)。

        否则,你在重构时就会有一个 “重磅炸弹”,然后在第二次审核时又会通过一些你本可以在最初重构时解决的小毛病。

      3. 这是工具的缺陷吗?

        推广过程中留下笔记,让他后来才发现,而工具中却没有办法重新安排优先级?

        1. 审阅工具保留了草稿,并有单独的 “提交审阅 ”步骤。编辑/删除评论非常容易。大多数人都这样做了,但他没有。

      4. 审阅描述(至少 GitHub 有)应包含 TLDR。

    4. 我认为吹毛求疵不仅对作者毫无用处,而且会主动增加噪音,要么掩盖了你希望注意到的真正问题,要么只是给作者带来毫无意义的工作。吹毛求疵似乎更像是一种滋补品,让审稿人能够说些什么来表明他们确实看了代码,结果只会浪费时间。

      1. 这取决于吹毛求疵。如果是拼写错误,那就值得修改。

        如果是格式问题,或者是一些可以被提示的问题,那就应该自动处理。

        如果是代码风格的问题,如果只是意见分歧,我一般会听之任之,或者尝试与同事讨论我们的偏好(尽管我仍然会批准 PR)。如果代码确实很糟糕,或者是初级开发人员的问题,那么提供反馈是有意义的。

        1. > 如果是拼写错误,那就值得修正。

          我的看法是,如果值得修正,那就不是吹毛求疵。小问题仍然是问题。

          自动化的东西,如果不是自动化的,也是应该修正的问题。

          教学机会也不是吹毛求疵,因为教学很重要。

          对我来说,如果我不会为修复它而添加一个提交,那么它就不值得评论。我会添加一个提交来修正拼写错误,但不会为了修改某个变量名而添加一个提交。前提是这是一个真正的提交,而不是你打算把它重置掉。

          1. 如果你把 “吹毛求疵 ”定义为 “不值得修改的地方”,那么顾名思义,吹毛求疵就是毫无价值的。

            我听说 “吹毛求疵 ”一词通常指 “小意见”、“我的偏好 ”或 “非常小的问题”。

            > 教学机会也不是吹毛求疵,因为教学很重要。

            偏好通常不是一个教学机会,它只是一个小分歧。它是试图让你的代码看起来像我(审阅者)写的一样。例如,也许你比我更喜欢简洁的名字。我们可以就每一点个人偏好进行争论,但这并不能很好地利用我们的时间。

            1. > 如果你把 “吹毛求疵 ”定义为 “不值得修改的地方”,那么根据定义,吹毛求疵就是毫无价值的。> 我听说 “吹毛求疵 ”一词通常是指 “小意见”、“我的喜好 ”或 “很次要”。

              这些都是不值得修正/改变的东西,所以这只是一种不同的表述方式。也就是说,“小评论”、“我的喜好”、“很次要 ”这些东西在公关中毫无价值。在闲聊时,它们可能会很有趣,但对于高效的工作流程来说,它们只是噪音。

              偏好也是一样……如果你只是依赖一种偏好,那就不值得。如果你正在教授一个更好的成语或类似的成语,那么它可能是值得的。这并不是硬性规定,只是一般来说,大多数 “吹毛求疵 ”式的公关评论都是净损失。

      2. 这就是我喜欢传统评论的原因:https://conventionalcomments.org/

        吹毛求疵会被明确声明为 “吹毛求疵”,因为参与公关的每个人都知道他们根本不是为了阻挡,如果他们花费的时间超过了他们的价值,我们会鼓励他们忽略这些吹毛求疵。

    5. 人无完人;犯错没关系,只要你自己承认错误,并为自己的错误向他人道歉。

      只有当评审人犯了这样的错误(或这里列出的其他各种错误,或其他一些错误)却不承认、不表示同情、不努力帮助疏通时,我才会开始刷新我的简历。

  2. 我最喜欢的反模式:

    把所有精力都放在捕捉内核和自动格式化工具可以捕捉到的错误上。当被问及架构决定时耸耸肩,真正重要的是这个字段是 PascalCase,而它应该是 camelCase。

    当被问及为何不在构建过程中自动捕获这些错误时,你会有几种常见的回答(额外提示:这些回答很可能是真的!):

    – 线程太慢,快速开发体验至关重要

    – 对整个现有代码库进行着色,实际上会毁掉作为有用工具的 “git blame”。

    – 不同的团队无法就是否对齐列包装的函数参数达成一致,因此他们同意在这一点上存在分歧。

    所以,与其把检查空格与制表符、LF 与 CRLF 的脑力开销自动化,还不如先用自己的线程过一遍 PR,挑剔每一个违规之处,哪怕是建议级别的,然后再考虑提交的正确性。

    1. > 换行太慢,快速开发体验至关重要

      从字面上看,我已经有 15 年没有使用过没有内置过夜功能的编辑器了。如果有人告诉我这些,那只能说明他们不懂代码,他们的代码会有更大的问题。

    2. > 对整个现有代码库进行过细校验将有效地毁掉 “git blame ”这个有用的工具。

      在项目根目录下放一个“.git-blame-ignore-revs ”文件,其中包含提交哈希值(和 # 注释)。GitHub 和其他流行的 git 工具会考虑使用它。

      1. 这真是个好建议,谢谢,我还真不知道有这个功能。

      2. 非常有趣!看来这是一个使用 `git config` 的本地解决方案。

    3. 可能的解决方案:强制预提交或预推送钩子,链接代码。

      1. 我们有这个方案,但有一些开发人员懒得在本地安装钩子,所以在处理他们的 PR 时,每次只修复一个衬垫错误,然后推送一个新的提交,看看在 CI 中有哪些错误。

        1. 我很高兴我不是唯一一个不得不忍受这种蠢货的人。不仅是疯狂的 git 日志,这些操作的计算也不是免费的。只要运行该死的预提交钩子,它就能帮你解决问题!

        2. 那就是他们的错了。毕竟这是他们的时间。

          安装预提交钩子再简单不过了。

    4. 我工作过的效率最高的团队就是在代码审查中忽略了这一点。

      比如,如果格式明显有误,而作者可能并不想这样做,我们会提出建议,但我们不会因为一个小问题而耽误代码评审。如果有一个变量是帕斯卡大小写而不是骆驼大小写,如果从算法和文档的角度来看是好代码,我们也不会阻止代码的输入。我们只是模糊地试图在每个文件中保持一致,像成年人一样行事。

      我们在价值方面确实达成了一致。我们坚持认为可以测试的代码都要进行测试。这是一个 C++ 代码库,所以我们坚持要尽可能快,建议采用适当的算法和数据访问模式。总之,我们非常关注错误,并让错误变得浅显易懂。

      我们的另一个共识是,自动代码格式化几乎总是会让代码看起来比这里或那里的小毛病更糟糕。任何强制要求做那些无关紧要的事情的人都是在碍事。

      1. 如果你没有自动格式化工具,那么当有自动格式化工具的人对你的文件进行处理时,你就会发现会出现混乱的差异。

        更糟糕的是,由于不同的人对格式化器的配置不同,你会发现在不断的乒乓球比赛中,你会得到混乱的差异。

        这是一个已经解决的问题,许多编辑器和工具都尊重 .editorconfig。

        学会配置它,就再也不用考虑格式问题了。

        1. 如果有人在别人的项目上运行与之不匹配的自动格式化器,总会造成问题。

      2. 我认为大小写保持一致是很重要的,因为如果不一致,人们在打字时就会绊倒,这很烦人,甚至可能导致错误。

        不过有一点我同意,讨论这个问题是浪费时间。这个问题最好通过自动工具/衬垫来解决。在编辑器、git 钩子和 CI 中。

        但如果你和你的同事对此没有意见,那我想就没问题了。

  3. 这是我经常遇到的问题。

    赎金说明

    > 这个补丁似乎对提交补丁的开发者特别重要。(也许他们直截了当地说,这是他们说服你接受补丁的理由之一。或者你只是读懂了字里行间的意思)。

    > 但这个补丁对你来说并不特别重要,所以你处于有利地位!现在你可以挟持他们需要的修改,直到他们做了很多额外的切题相关工作,这些工作其实不需要在同一个提交中,但对你很重要。

    审阅人断然否认这种情况,但这种情况每次都会发生。

    对于修改文档的提交,他会要求重做整个文档,包括没有修改的页面。

    在他提出的每个问题都得到解决之前,他都会拒绝批准。

    我已经不再要求他们审核 PR,而是让其他团队审核我的 PR。

    1. 我也犯过这样的毛病,一般我会写一些 “这样做很好,但不在范围内,除非你愿意承担 ”之类的话。

      1. 这个提议很合理。

        我的问题是,如果你想做其他事情,那就为它们提出问题。不要试图解决每一个你不喜欢的问题,这些问题与我试图修复的代码密切相关。

        比如,他有很好的反馈。这不是反馈或内容的质量问题,而是它与我目前急需实施以解决问题的内容无关。

        1. 我明白你的意思,我也同意。这也许是件好事,但这样的事情不应该阻止公关。

    2. 我曾让一个平台负责人不断尝试将小的重构转化为功能 PR,因为他发现这是实现持续架构改进的唯一方法。(医疗产品,所以流程很死板)

      1. 我分不清这是抱怨还是钦佩。

        1. 这很令人讨厌,但很有效,也有点可以理解,所以是的,两者都是:)

      1. 不是,但我不会去管理层处理个人问题。我只负责处理。当你去找管理层时,你必须接受管理层的决定。当你只做决定时,你就获得了自主权。

  4. 千百次往返的死亡有一个反面,即审查员指出他们能发现的所有问题,然后作者只修复一件事。你可能会认为作者的动机是尽量减少往返次数,但也许他们接下来会拿出反向赎金条,威胁说要找不同的审阅人,或以某种方式颠覆代码审阅,因为 “情况紧急,补丁必须马上打进去”。

    1. 在我之前的工作中,质量不高、缺乏主人翁精神的情况经常发生。

      在审查代码时,我总是小心翼翼地在不重要的地方加上“[建议]”或“[挑刺]”的前缀。这些都是小事情,比如注释中的错别字,不会妨碍我批准查看。

      因此,了解我的人都知道,我不会轻率地标记一些东西。不过,我经常会在审查某人的代码(注意,是我认识并尊敬的人)时,提出 3-5 条必须修改的意见,然后才会批准。因为如果不修改,要么会造成错误,要么会给以后的生活带来不必要的麻烦。他们只修改了其中一项,我就不得不指出他们没有解决其他任何问题。然后他们会回去再修改一个,如此循环往复,直到他们最终解决了所有问题。

      只有一次,我因为厌倦了来来回回的折腾而心力交瘁,批准了不合标准的代码。那次是一个学生,他的带薪实习期快满了,他必须在离开之前部署好代码(我们最终没有使用那个项目……)。(我们最终没有使用那个项目……)。

      1. 如果是我,100% 会在 MR 结束时写上 “如果你不想工作,可以直说”。这种话在我的团队里根本行不通,这是对其他人的伤害,也是一种侮辱。

    2. 如果我不得不告诉补丁提交者修正相同的问题两次,我就会特别恼火。请仔细阅读我写的所有内容!

  5. 我看到很多这样的评论都将代码审查归结为 “挑剔的审查员 ”或 “懒惰的提交者”,但我的亲身经历却与这两种解释都不相符,这就是当代码审查被用作政治武器的时候。

    在我的案例中,另一位工程师正在觊觎我的技术主管职位,他们开始使用 OP 帖子中列出的几种策略来拖延和阻挠我的拉取请求(我们之前一直保持着良好的关系),而他们的 PR 则是在我睡觉的时候(美国和欧洲的工作时间不同)由我们团队的一位初级工程师盖章的。

    管理层注意到了这一点,并对我产生了永久性的厌恶,我再也无法从这种伤害中恢复过来。直到为时已晚,我才完全意识到发生了什么。不到一个月后,我就被解雇了,而另一位工程师却得到了晋升。不过,这也是非常宝贵的一课,一个月后我就找到了一份更好的工作。

  6. 做好代码审查很难。你不可能一蹴而就,当然,项目总是很晚才完成,所以有批准的压力。如果你希望代码评审做得很好,你就需要制定一项政策,规定评审员需要连续 3 天在不做修改的情况下批准代码,但管理层当然希望尽快发布代码,然后继续开发下一个功能。

    发现问题很容易。大多数人都会用眼角余光注意到一个拼写错误,然后把它放大。(我的拼写很糟糕,但我经常这样做)。要注意到代码有错误和漏洞则难得多。我已经通过了很多次代码评审,只需修正并推送一些小问题,直到第二天我才意识到代码存在严重问题。

    1. > 项目总是迟到

      这是万恶之源。为了提高肾上腺素水平,把下周就能解决的问题说成是 “今天的当务之急”,从而让你今天多工作 30 分钟,这种人为设定的最后期限导致很多软件最终变成了一坨屎。

      软件需要深入思考和全面理解。是的,这一切都需要权衡利弊,但有一点是不能再快了。

      1. 我认为你忽略了一点:想要的功能总是超过时间。你需要某种形式的最后期限来迫使你考虑削减哪些功能。在迫不得已的情况下,没人愿意删减任何可以加入的功能,所以你总是会迟到。

        最后期限是人为设定的,但它们一旦存在,就会迫使很多人去做实际的时间安排,因此它们就变得真实了。从理论上讲,你可以改变它们,但实际上大家都在依赖你。

        你说的没有错,但要结合上述情况。

        1. GP 在这里,我并不反对你的观点,工程设计就是权衡利弊,你不可能用几个月的时间来解决简单的问题,从而将工程设计做到完美。你需要进步和效率,所以有时不太重要的问题需要用不太理想的黑客手段等来解决。我同意这一点,但 “所有事情都要在昨天完成”、“如果能在 6 个月内完成,就告诉客户在 2 个月内完成 ”等只为制造压力和提高生产力的文化,会成倍地降低工程质量,而不是提高生产力和效率。

      2. 我同意你的观点,但我也知道,我是一个需要某种外部激励的人,否则我就会陷入无休止的重写和重构中;我不清楚这是否必须是老板/客户设定的最后期限,或者有一个稍微偏向于实践-理想连续体另一端的同事就足够了,但我知道,至少在施加某种压力的情况下,我的工作才能做得最好。

        1. 我最近被诊断出患有注意力缺失症,对自己的了解解释了我的一种行为,这种行为似乎与你的行为截然相反–最后期限的压力让我恐慌,而我的发泄方式就是骑自行车。

          给我每天和每周的承诺,我会最专注、最优质地完成工作。

      3. 是的,这正是我不喜欢冲刺的原因。

        Scrumban 的压力更小。

    2. 如果说有什么时候需要有意识地避免 “骑车”,那就是代码审查期间。容易出错的地方价值相对较小。导致整个系统瘫痪的不是拼写错误的注释,也不是 “变量名不符合我的初衷”。

      在我的评论中,我最看重的一点是程序员是否真的验证了程序的功能,最好是通过一些单元测试。这样我就可以自由地考虑架构问题,以及是否存在潜伏的错误,而不必审查它是否能运行。这对我们的 “基础设施即代码 ”审查特别有帮助。许多此类审核都是事后审核;它们已经运行过了,我们知道它已经带来了正确的基础架构,审核的目的是确保它在架构上的方向是正确的,也许标签标准没有被意外违反,以及其他高层次的重要事项,而不是我们是否喜欢某个子系统的特定名称。

      取得这种平衡可能是代码审查中最难的事情。

      此外,代码审查的主要工作之一就是让额外的眼睛来验证没有人在代码中设置明显的后门。如果你做到了这一点,那么即使你不做其他事情,也会带来一些价值。如果一个 Bug 被编写代码的人发现了,那么它被一个不在编写代码的人发现也不是什么可怕的事情,因为他正在做别的事情,而且所处的环境也完全不同。认为不会被别人发现的想法近乎神来之笔。但至少,今天你看不到任何 “eval(base64_decode(‘big long string’))”。审查的目的只是为了提高团队的工作效率,而不是为了保证没有 bug 会传到 QA、prod 或客户那里。它们只是产品组合的一部分,不应该被当作更重要的东西。

    3. > 不能一蹴而就

      > 如果你想让代码评审顺利进行,你就需要制定一项政策,规定评审员需要连续 3 天在不做修改的情况下批准代码评审。

      可能是我理解错了,但听起来你似乎在处理非常复杂的 PR/提交。

      我认为首先要解决的问题是将提交的大小控制在合理的范围内,这样就不需要 3 天的审批和多次审核了。(这在我看来太疯狂了!)。

      我的典型提交量大概是 100-200 行(其中很多要小得多),所以需要 3 天的审批时间就太夸张了。一次通过就足够了。

      1. 等等,你的提交有几百行,还是你的 MRs/PRs 有几百行?我喜欢尽可能保持提交的原子性,虽然不一定每次都能成功,但经常会在提交后削减提交,让 repo 处于工作状态。我很少提交 100 行的改动。也许这些都是在很多地方使用的变量重命名?或者是一些擦拭提交,只是确保工作时间结束时提交?

        如果只需进行一次改进,那么一次通过通常是行不通的,因为改进的实现需要再次审核,除非它已经被审核者从字面上理解为对代码的建议。

        1. 对我来说,改动越小越好,但这取决于我在做什么。目前,我正在进行重大重构,以便将 1.5 MLOC 的单体代码库移植到新的运行时上。这涉及到大量的重构工作。你不希望一次改动很多东西,所以举个例子,一次重构可能是提交删除一个在新运行时将不复存在的模式。这样很容易就会修改 2000 个文件,总计 10,000 行。该提交是原子性的,只做一件事,分割开来没有意义。

          尽管如此,即使每个文件中都是相同的原子变更,审查员仍需要检查所有文件。

    4. 我对代码审查的原则是:我的大脑是一个糟糕的解释器。要么我运行代码并发现它能正常工作,要么我相信测试(见 #1 为什么这样做行不通),要么我盖上橡皮图章然后继续工作。我的选择主要取决于该部分代码出现错误的风险/影响,而我已经在这里工作了足够长的时间,我的直觉通常都是正确的。

    5. 在您的流程中,代码审查承担着怎样的负荷,以至于您需要 3 天的积极工作来批准?自动验证、验收测试和手动回归测试的一些混合体应该涵盖审查后的代码是否正确。

      这样的严格程度似乎只有失败案例非常严重的系统才能承受,而且还能继续构建。

      1. 如果客户去了其他地方,失败的代价是昂贵的。我不在网络世界,在那里我们可以每天多次推向生产。我工作的嵌入式系统在某些情况下甚至不在手机信号的范围内(我们正在研究 starlink),因此更新意味着需要人工去更新设备,而更新的成本又很高。

    6. > 我的拼写很糟糕,我经常这样做

      你为什么不在集成开发环境中使用拼写检查器?我不明白人们为什么不这么做,这太简单了,而且如果你没有不断地提交拼写和语法错误,就会显得更专业。

      不过我从来不会在代码审查时提出来,浪费时间。

      1. 我见过的所有拼写检查程序都很麻烦。他们永远搞不清楚我说的是哪个词….。

        我在代码审查中提出拼写问题,但他们总是 “修正后推送,无需进一步审查”。

  7. 以下是我在不同团队中见过的一些解决方案:

    对于一次性审查,设定期望值: 开发人员要求的是反馈,而不是许可。他不需要向审核员解释为什么他没有采纳所有的修改。不要赋予他人无意义的权力。这将解决提交中的大部分问题。

    对于更有条理/更重要的评审,由第三方版主决定有争议的代码修改。如果做不到这一点,那就坚持 > 1 名审核员,并声明只有在审核员达成共识的情况下才进行修改。这通常能解决风格问题。

    我希望看到但尚未看到的修改:

    审稿人不应该提问–他们应该陈述关注的问题。所以不要说 “你为什么不通过 X 来做这件事?而是说 “我认为通过 X 而不是 Y 来做更好,因为……”。“.

    绝对禁止 “你为什么要这样解决?”

    1. 在我的印象中,提问往往是审稿中最好的部分,至少在整体审稿文化健康且富有成效的情况下是这样。如果不出意外的话,这往往是一个很好的机会,可以补充说明一些写完后不明显的地方。而且,接受良好审稿的作者通常也会同意,增加清晰度会对这些情况有所帮助。

      在这种情况下,“你为什么这样解决?”也是一个很好的问题。对于大的 “整体方法”,通常的说法是应该先设计后实施。但有时,这个问题会帮助我们发现一些复杂的小问题,而这些问题只有在实际工作中才会显现出来。(这也经常会促使我们添加一些澄清性的评论)。

      1. 如果你确实感到困惑或需要澄清,提问是件好事。但如果你有疑虑,请在提问时表达出来,这样就有了上下文。

        > “为什么要这样解决?”

        “因为这样能解决问题”。

        如果你没有顾虑,为什么要问我这个问题?如果你有,为什么不告诉我?如果你不说,那我就对你的问题给出最基本的正确答案。

        对话是双向的。如果你想让我花很多时间来解释,那就回报我!任何人都可以提出开放式问题。回答问题比提问要花费更多的精力。让我值得花时间回答你的问题!

        强迫评论者在讨论中投入精力也能解决很多问题。人们不会浪费自己的大量时间去玩这类游戏。让门槛高到足以阻止 90% 的坏演员行动。

        1. 在我看来,这一切都很因人而异,首先是信任度/团队凝聚力等。比如,如果我所在的团队团结不力,我几乎肯定会同意你的观点,而不会有更多的细微差别。

          然而,如果我所在的团队运作良好,以 “你为什么这样解决问题?”为例,我可能并不总是知道为什么会问我这个问题,但仅仅是被问这个问题就足以让我提出一些假设。如果不是这样,也很有可能是一个学习的机会:也许我的工具包里缺少了什么,也许我没有注意到某些意外的结果。促使我不带偏见地思考这些可能性,可能会帮助我学习(或强化我将要学习的东西),让我能够追溯我的步骤,寻找我理解中的其他空白。

          我说这些的同时也承认,在我的职业生涯中,我往往会比许多同行在复习和复习背景下的交流方面投入更多的精力。有时,我过度沟通的倾向会产生与我本意相反的效果。

          无论如何,从其他沟通挑战的角度来看,我更倾向于你的立场!

          1. > 我可能并不总是知道为什么会有人问我这个问题,但只要有人问我这个问题,我往往就能提出一些假设。如果不是这样,也很有可能是一个学习的机会:也许我的工具包里缺少了什么,也许我没有注意到某些意外的结果。促使我不带偏见地思考这些可能性,可能会帮助我学习(或强化我将要学习的东西),让我能够追溯我的步骤,寻找我理解中的其他空白。

            我完全不反对这些事情可能发生,重点是 “可能”。代码审查员说:”有趣的方法。你本可以通过 X 来解决这个问题,这样会有额外的好处。你选择 Y 而不是 X 的原因是什么?

            这是一个双向对话。在你上面的段落中,你没有足够认真地考虑到真正需要学习新知识的可能是审阅者,因此对话应该反映出这种对称性。

            我经常看到(是的,在低效团队中)以下模式:

            评审人: 你为什么要这样解决问题(X)?

            我:因为(概述一个简单的原因)。

            评审人:但为什么不是 Y?但为什么不是 Y?

            我:为什么是 Y?

            评审人: (50%的情况下没有给出很好的答案,只是坚持认为 Y 更好,因为网上随便有一篇文章)。

            请注意

            1. 评审者无法说明为什么他的方法更好。

            2. 在这次交流中,开发人员要比审核人员付出更多的努力。

            在一个运作较好的团队中,情况会是这样的:

            审核员 你为什么要这样解决问题(X)?

            我:成功了。你为什么这么问?

            评审员: 哦,我觉得 X 会更好。(如果我幸运的话,他会解释原因)。

            我:好吧,继续……?

            评论者: (继续或紧扣主题,但提供更多背景信息)。

            这两种互动方式都很 “糟糕”,相比之下,评论者只需直接说明他所关注的问题。

    2. > 他不需要向审稿人解释为什么没有采纳所有的修改意见。不要给别人无意义的权力。这将解决提交材料中的大部分问题。

      在我看来,这是团队不健康/不信任的表现。

      团队通常会要求对代码进行审核,所以,是的,你_是在请求许可。如果审核员提供的反馈意见您觉得不重要,您可以随时请求其他人批准。

      背着别人这样做显然是不健康的,所以您应该与审阅人讨论一下为什么某些东西是重要的/不重要的。如果你觉得无法进行这样的对话,或者审稿人没有考虑到你的想法,那就说明你们的意见不一致。

      1. > 团队通常会要求对代码进行审核,所以,是的,你是在请求允许。

        你漏掉了

        > 对于一次性评论,要设定期望值: 开发人员要求的是反馈,而不是许可。

        我说的不是代码审查是什么,而是它们应该是什么。我亲眼目睹过这种政策的效果。但这并不适合所有类型的项目。

        此外,还有一个小问题: 要求代码审查并不等同于允许。它只是意味着人们需要征求反馈意见。

        > 如果审查员提供的反馈你觉得不重要,你可以随时要求其他人批准。

        如果团队允许这样做,那么我认为这是完全可以接受的。对我来说,这等同于

        “由第三方版主决定有争议的代码修改。如果你不能这样做,那就坚持 > 1 名审核员,并声明只有在审核员达成共识的情况下,你才会进行修改”。

        > 在别人背后这样做显然是不健康的,所以你应该和审阅者讨论一下为什么某些事情是重要的/不重要的。

        同意,解决办法是

        1. 让代码审查讨论透明化–至少对管理层透明(例如,通过代码审查工具可见)。这就限制了滥用队友的可能性。

        2. 不要让他们背着审查员。只需邀请其他审阅者(或上报版主)即可。要公开进行,让审核员知晓。

        我从没说过 “不要对话”。对话是必要的。我说的是 “不要在没有适当制衡的情况下给予评审人不当影响”。如果他在提交的论文中使用了这种策略,请确保您的团队文化是公开的,并且可以处理这种情况。您可以从其他评论中看到这些问题是多么常见。期望大多数团队都是高度信任和运作良好的团队是不现实的。然而,简单的规则可以让团队更接近这一目标。

        > 如果你觉得无法进行这些对话,或者审核员没有考虑到你的想法,那么这就表明你们的意见不一致。

        或者(更常见的情况是),这表明审核员是一个糟糕的审核员,并且/或者滥用了他的权力。

        1. > 期望大多数团队信任度高、运作良好是不现实的。

          也许我们在这里有两个对话。如果团队信任度不高,或者有些人很难打交道,你的方法就比较正确。

          而我的方法则是我眼中理想的团队凝聚力和效率。

          这两种方法都值得考虑,但就我个人而言,如果我不喜欢我的团队,我就不会在这里呆很久。

          1. 信任是一个连续体。毕竟,代码审查的存在只是不信任的形式化:-)

            我认为,围绕代码审查的政策和文化会因情况而异(新员工与经验丰富的开发人员、大团队与小团队、小代码变更与大代码变更等)。不过,我不认为我提倡的任何东西对信任度高的团队来说都是坏主意。

            可能的例外情况是,代码审查 “不是请求许可”。如果你已经将其构建为一种把关机制,只需确保有一个 “裁判 ”或 “打破僵局者 ”来使这一过程更加顺畅/快速。即使是信任度高/运作良好的团队,偶尔也会在评审过程中出现无谓的争吵,而评审者和开发者都应该有办法跨越这一鸿沟。不,“成熟一点,讨论清楚 ”并不是每次都能奏效。

            至于表达关切–什么时候这不是个好主意了?这不是我想出来的。我是从阅读有关有效沟通的书籍中学到的。在日常对话中,这是一条很好的生活准则。

            1. 当有人无视我的反馈意见,要求别人进行评论时,我就会很恼火,因为我知道别人会批准任何事情。我认为这样做相当不尊重人,而且规避了代码审查的意义。

              1. 您的评论很好地诠释了我的意思:

                问题不在于开发人员,而在于批准一切的人。

                一个需要代码审查的团队,如果有一个什么都要审批的人,那就是一个不健康的团队。

                开发人员之所以要求代码审查,并不是因为他想要反馈,而是因为这是团队政策规定他必须履行的程序。如果团队政策改为 “代码审查是为了获得反馈,而不是许可”,那么他就不会浪费你的时间了。问题不在于他,而在于团队文化/政策。如果团队关注的是反馈,那么你只会收到那些希望得到你反馈的人的请求。

                > 当有人无视我的反馈时,我会很恼火

                你是对他规避代码评审的目的感到恼火,还是对他没有采纳你的反馈意见感到恼火?如果是后者,我强烈建议您做出改变。对反馈意见采取行动永远都应该是可选的。

                当然,让代码审查完全成为可选项是大多数团队所不具备的。但他们没有这种能力的原因之一是,你很少能得到一个高度信任的团队。

                尽管如此,我们还是可以通过强制代码审查的缓解协议来解决你所强调的一些问题。如果所有的代码审查都是通过 Github 进行的,而当你留下了强烈的反馈意见时,其他人却批准了合并,那么团队就真的需要制定一个协议来处理这种情况。

                1. 我明白了,我想我更能理解你的意思了,我想我同意。

                  > 你是对他规避代码审查的目的感到恼火,还是对他没有采纳你的反馈意见感到恼火?如果是后者,我强烈建议你做出改变。对反馈意见采取行动永远都应该是可选的。

                  这种情况只在几个人身上发生过。通常在这种情况下,我会留下评论,但评论永远不会被处理;作者会要求另一位审稿人,而我的评论则会被忽略。

                  我不会以公关为要挟,如果事情来来回回超过一两轮,我几乎总是会向作者让步。

  8. 我看到的另一种情况是,坚持吹毛求疵的人从不接受对自己公关的评论。

    仅供参考:

    1. 在构建链中使用格式化工具和线程器 = 零格式化废话吹毛求疵。

    2. 尽可能提问而不是批评。这样比较友好,而且当你认为自己看到了一个错误,但事实并非如此时,也不会那么尴尬。

    3. 这是审核员了解代码库最新情况的机会。

    4. 这是被评审者分担错误的机会。审核过的人不能因为后来发现的 bug 而指责他们。不能费心审查的人也不能抱怨。

    5. 只要你能诚实地做出正面评价,就请做出正面评价–这样既能减轻压力,又能增进善意。

    6. 在公关中表现得像个混蛋的人通常是你不希望在团队中出现的那类人。也就是说,这是一种发现这类人的方法–看看他们是如何利用权力的。

    1. 我还喜欢做的一件事是在 Slack 上发送信息,或在公关上留下额外评论,表达我的总体感受/赞美。

      此外,关于格式化/线性化的说明也很有价值。我很惊讶有这么多团队只是在他们的公关报告上不断提出吹毛求疵的意见,而不是自动进行这些检查。

  9. 我还没有遇到过这样的环境,PR 要么基本上就是橡皮图章,因为外部审计说他们需要做代码审查,要么就是文章中描述的千刀万剐。

    我相信这种情况在工作中一定存在,但我在 4 家不同的公司工作了 5 年,还没有遇到过这种情况(除了我在代码评审中为其他人提供的 PR 之外)。

    1. 当我带着这些问题进入一个团队时,我有两种成功的方法:

      1. 区分关键评论和意见/好的评论。并不是每条评论都需要作为该特定公关的一部分来解决,它们可以在纪律严明的团队中合并后的后续工作中解决。

      2. 让审核人员自己为 PR 做出贡献。有些开发人员会过于执着于代码的 “所有权”,而最终还是要由团队来负责(至少在你有一个无瑕疵/注重改进的文化时是这样)。我不需要原始分支的所有者来修改错别字或吹毛求疵的地方,而我只需要花更少的时间就能搞定。我还没发现有什么代码建议功能能做到这一点,所以有时你会因为前后意见不一致而不得不重新提交,但大多数情况下都没问题,而且整体上还能提高速度。基本上,把 PR 变成了异步结对编程。

      1. 不错。我想我已经发现,在我的遭遇中,亲自为其他人的代码评审做好一切并没有激发其他开发人员做出相应的改变。

        也就是说,我还没有走进一个缺乏公关文化的环境,并设法实现文化变革(目前还没有)。也许资历是一个先决条件?

      2. 很大,尤其是第二点。我在阅读代码时会修改错别字和明显的错误,并在任何不应该阻止发布的注释上留下 “请自行决定 ”的字样。

        尽管如此,“拆车 ”现象还是会发生,只是程度较轻,而且是在更实质性的问题上,比如模块的整体设计。

    2. 因此,这里有几件事:

      首先,同行评审的部分好处在于有两个人看过代码,而且很熟悉。即使是橡皮图章,只要有人真正看过,LGTM 可能就足够了。

      其次,即使同行评审只能发现 10% 的问题,它也能发现最明显的错误。

    3. 我认为这份文件是审查的黄金标准,对参与双方都是如此:https://github.com/google/eng-practices/blob/master/review%2…

    4. 30 年前,当我还是一名实习生时,与我一起工作的资深开发人员必须决定我的代码是否值得进行 “正式评审”,还是只需进行代码评审就足够了。我们进行了代码审查,但作为审查的一部分,我确实听说了一个代码审查有用的过程,显然该公司确实将其用于重要/困难的代码。

      在正式审查中,你要把代码打印出来,然后让其他 5-10 名开发人员在一间会议室里一起看(这间会议室里没有电脑,不过有时你会回去再打印一些代码,以了解上下文)。这样每小时大约可以完成 10 行代码,而开发人员每天最多工作 2 个小时就会精疲力竭,因此这需要很长时间。不过有报告称,这样做的结果是对代码进行了最有用的审查/修正。

      我从未见过上述做法,只是听说过。有朝一日我也想见识一下,但我怀疑自己是否能做到。

      1. 听起来像是费根的评论。我也没有参与过这种审查,但我认为促使人们去做审查的一些力量已经被削弱了。

        当你要交付的代码无法轻易更新(即成本低廉)时,你就会试图在过程中尽早消除尽可能多的缺陷。CI/CD 和网络交付似乎会削弱如今开发的大多数软件通过费根评审的动力。

        我可以想象,在安全关键型系统中仍会发生某种形式的这种情况(或者更有可能的是,技术水平已在此基础上取得了进步)。

    5. 当我拉下有问题的分支,在本地进行测试,并想办法破解它时,我时不时就能得到很好的反馈。我很少有这样的时间。

      1. 代码审查不是为了测试功能或查找错误。它是关于设计的合理性检查(尽管在一个表现出色的团队中,设计不应该让人感到惊讶)、复杂性、可读性和测试覆盖率。

    6. 您可以将代码审查视为知识转移的机会。

      ……这样做还有一个额外的好处,那就是你的反馈对象会真正阅读你的反馈,因为这些反馈与他们正在做的工作直接相关。

  10. 其中大部分,也许是全部,都有双重拉取请求反模式。

    > 赎金说明

    开发人员提交了一个对他们特别重要的补丁。当你指出为了使组件在逻辑上保持一致,还需要对其他地方进行修改时,你却被告知这些修改与要解决的问题(他们要解决的问题)无关。

    > 猜谜游戏

    开发人员提交了一个实质性的补丁。补丁的内容表明,提交者没有理解他们所修改代码的设计。当审核员将这一情况反馈给提交者时,提交者希望审核员放弃正在进行的工作,转而设计提交者需要的功能。

    > 优先级倒置

    提交了一个大型补丁,但对底层设计解释不清。正确审核补丁是否合理需要时间。提交者(公开)抱怨说,他们(可以证实)没有收到代码审查的回复,这阻碍了 X 功能的开发。

    > 自相矛盾

    需要权衡利弊,而提交者的决定与实际情况相去甚远。

    问题通常不在于挑剔的评审者或马虎的提交者,而在于缺乏明确的贡献准则。

  11. 如果您发现自己在审阅过程中来回 “乒乓”,等了 24 小时的审阅,五分钟就搞定了,然后再等 24 小时重新审阅,那么您就应该像对待其他异步流程一样对待它,因为您发现自己希望它是同步的–停下来,把您和审阅者都叫到一个房间里,在房间里完成审批,或者至少在一个点上,很明显您有超过五分钟的工作要完成审批。

    代码生成器具有代码审查功能,但这并不意味着你的所有代码审查行为都必须在其中进行跟踪。

  12. 我觉得我可能已经见识过了所有列出的反模式的两面,还有一些。

    其中很多都有一些自然的根源。这些原因普遍适用于审核者和被审核者。

    – 没有足够的上下文(为什么需要这样修改,为什么现有代码是这样的)

    – 共同方向不够(我们应该做这件事,我们已经决定不做这件事了)

    – 对时间的期望不一致(帮我审核这 1000 行 PR,我有全职工作)

    – 隐藏的背景(为什么我们不能…… /因为有这个历史先例,我们不能这样做)

    – 不同的人格类型/障碍(ASD 和其他问题可能意味着吹毛求疵/技术问题和更大的问题一样 “重要”)

    – 工具问题(没有办法只提交大问题而不提交小问题)

    – 代码是作者身份的一部分(制表符与空格)的替代物

    ===

    我认为有助于避免这些问题的主要方法有

    0. 在可能的情况下,除了审阅之外,还要建立某种默契。

    1. 假定作者有积极的意图。

    传达期望/限制

    沟通适当的背景

    不要对所有的沟通都一视同仁,不同的审稿人有时需要不同的方法。

    5. 记住评审的是代码,而不是人(这适用于双方)

  13. 让我们思考一下写作… 许多形式的写作都得益于一个明确的起始目标,然后是一个大纲,接着是内容的多次反复。

    代码评审中的一个类似挑战是,评审人无法快速看到(a)目标和(b)提纲。如果没有这两点,就很难 “引导 ”出什么样的反馈会有帮助。

    a. 在很多情况下,审阅人和被审阅人可能会认为他们有相同的目标,但这往往是微妙或明显的错误。即使是同一个组织中的人,也可能为不同的主人服务。

    b. 不同的软件文化对计划、大纲、设计文档等有不同的理解。有的文化是 “随心所欲”。有些人把这些东西记在脑子里,但很难很快与别人分享自己的想法。如果对问题的轮廓没有一定的概念,你怎么能正确地评估代码呢?

  14. 我认为这个列表很好,但如果这些事情经常发生在审查中,我认为要么是文化问题,要么就是程序员没有注意或没有为彼此设置防护栏,以避免提交会在审查中被发现的事情。

    每个像样的集成开发环境都有一个选项,用于设置代码命名和结构的某种配置。

    我认为代码审查应该有点迂腐,应该在保证质量的前提下进行,并考虑到未来的变化。

    这里列出的模式都是有效的,但如果这些模式经常出现,而不是让团队感觉舒适,那就需要审视一下你们的工作文化了。

    1. 风格指南、排版器和格式化器可以消除很多潜在的问题,但根据我的经验,最需要它们的团队往往是最难/最不可能获得任何支持的团队。

  15. 通过结对编程可以大大加快代码审查速度!

    当你们一起编写代码时,很多问题甚至都不会出现,因为你们已经讨论过了一切。有时,结对编程后甚至不需要代码审查!

    1. 很多时候,我发现代码评审之所以有用,正是因为评审者与代码之间有一定的距离;一个全新的视角可以很好地发现问题。(出于同样的原因,我经常会在提交审核之前仔细检查自己的修改)。

      结对/群组编程通常也会缺少关于代码实际用途的说明(因为这些都是口头传达的,从未在评审中写下),这就给代码考古带来了困难。或者,这只是源于我不得不处理的(外部)代码库,而这些代码库都是以这种方式完成的,并不能一概而论……

    2. 如果你需要一个以上的审核员,只需进行 (n+1)-tuple 编程(其中 n 是所需审核员的数量)。越多越好!

      1. 我想你是在开玩笑,但在我所在的地区,有一家公司以坚持 “暴民编程 ”方法而闻名,5 个以上的开发人员会坐在一个房间里,1 个人负责开车,其他人负责填写旁边的文档。

  16. 试图运用模式等启发式方法来解决这个问题是错误的。归根结底,是审核人员缺乏归属感、信任感和做正确事情的动力。缺席的领导层没有让工程师为他们的不作为或审查缓慢负责,这才是助纣为虐的罪魁祸首。

    这种功能失调多发生在控制不力的团队中,但有时也会暂时发生在其他功能正常的团队中,即多人争夺项目的哲学控制权。

  17. 有时,问题可能会归结为沟通不畅,我发现采用公关意见的格式可以很好地解决这个问题,这样就可以消除哪些是阻挠、哪些是可选建议、哪些是真正的问题等方面的歧义,例如:

    https://conventionalcomments.org/

  18. 我很惊讶没有更多关于如何审核进入内核的代码的研究。各种子系统都有不同级别的审查。对于最终的修改,大部分都是 “公开 ”进行的。哎呀,我们现在认为理所当然的许多工具都是在内核上磨砺出来的。

  19. 我希望看到这样一个系统:你不审查补丁,你修改它们,然后把它发回去,让他们审查改进后的补丁。当有人按原样接受补丁时,游戏就结束了。

    但也许这只适用于同一项目中的同行。

  20. 作者和审阅者都希望得到 “好代码”,但在任何项目中,“好代码 ”的定义又有多少呢?明确定义什么对你的团队和你的代码库来说是重要的,这将减少很多反模式。

  21. 很多权力游戏的背后都是权力的博弈,因为参与者往往认为自己通过对真理的执着追求或其他什么,可以凌驾于权力游戏之上,所以才会变得特别反常。

    人们越觉得自己受到了尊重,就越不需要强迫他人的行为来弥补缺乏尊重的感觉。

    尤其是在一些地方,当地的暴君认为他们应该得到的尊重比他们实际得到的尊重要多得多(如专家新手主义),这种情况就更具有毒性。

  22. 哇,真是个怀旧的域名。我发誓几十年前就在 chiark.greenend.org.uk 上看到过其他作者的作品,也许那时互联网还没有广泛商业化。

    1. 这是一个老域名。注册日期是 “1996 年 8 月之前”,当时 Nominet 成立,负责管理 .uk 域名。

  23. 现在我确信,一家非常知名的技术公司的某位高级工程师将此作为使用手册。我曾多次从同一个人那里遇到过上述情况!

    不过,就我自己而言,我确实犯了往返 100 次的毛病。我倾向于在精神上把小毛病归类,而不是把我发现的每一个都写出来

  24. 如果尝试训练一名 LLM 在遵循这些反模式的同时审查代码,可能会很有趣,也很有教育意义。

    1. 有趣。也许比一开始看起来要容易得多:让它为了好玩而做错事,我打赌它会成功的

      1. “除了遵循这些反模式之外,我无法以其他方式理解它。请检查这段代码”。

    2. 更有建设性的做法是,训练一个人能够注意到它们,并在它们发生时进行干预。

  25. 以备不时之需:

    挑剔者的赌博: 审阅者专注于琐碎的风格问题,如空白、括号位置、变量命名规则等。他们会让开发人员完全遵从自己喜欢的风格,即使团队的编码指南中没有明确规定。

    沉默对待: 审阅者在审阅请求发出后的很长一段时间内都不提供任何反馈,但会经常做出回应,以保持审阅的 “活跃”。作者必须多次 ping 才能得到任何回复。

    隧道视野: 评审者只看具体修改的行,而不考虑代码的大背景。他们提出的修改在局部上是有效的,但故意与整体设计或架构不一致。

    谩骂: 审阅者对代码进行尖刻的评论,暗示作者缺乏经验和/或不称职,但又不直接说出来(会被指责为刻薄)。

    哲学辩论:评论者在评论中就继承好还是组合好这样的观点进行了长篇大论。在抽象的讨论中忽略了当前的实际问题。

    1. > 挑剔者的赌局: 评审者专注于微不足道的风格问题,如空白、括号位置、变量命名规则等。他们会让开发人员完全遵从自己喜欢的风格,即使团队的编码指南中没有明确规定。

      大多数情况下,自动格式化工具可以解决这些问题。如果你的团队拒绝使用自动格式化工具,那就开始收集在这些讨论中浪费的时间指标,并提交给管理层。

      > 沉默对待: 审阅人在审阅请求发出后很长时间都不提供任何反馈,但却经常回复,以保持审阅的 “活跃”。作者必须多次联系才能得到回复。

      制定一项政策,如果没有回应,则在 N 天后自动批准。

  26. 这很好!我对其中的几条很有感触!

  27. 对我来说,只有一件事是最重要的,那就是大家齐心协力,把好的代码放到系统中。

    如果你工作的环境中,代码审查人员以任何方式凌驾于你之上,那么你将一事无成。如果你希望审查员不审查你的代码,这同样适用。

    我经常看到关于 “吹毛求疵 ”的讨论。如果只是一个很小的地方就能让代码与现有的代码库保持一致,那就去做而不是去反对。如果有一些不明确的地方,那就作为一个团队找到一条规则并加以应用。

    我不知道在不是大家一起工作的环境中,人们是如何工作的。

  28. 关于 “太大了,改小一点!”和 “缺少背景,改大一点!”的评论非常有道理。无论哪种方式,显然都是一个复杂的变化,只要选择一种方式并坚持下去就可以了。

    不要把公关 “太大 ”与它不是原子结构混为一谈–那是另一个话题,是教给初级队友的东西。

发表回复

您的电子邮箱地址不会被公开。 必填项已用 * 标注