现实中的代码评审

代码审查| 2018-04-10

曾经写过一点关于代码评审（code review）的文章，比如这篇和这篇，现在觉得关于它的认识又有了不少更新。软件工程的技术和实践分成两部分，一部分是和书本知识一致的，大约占一半，这部分基本上在大学里就可以学，自学只要方法得当、刻苦努力也可是途径；但是第二部分来自于实际团队、经验，内容通常无法从书本当中获得，而且难说对错，不同的人和不同的经历造就了不同的认识。代码评审就是第二部分颇具槽点，可以大加讨论的典型。

代码评审是展现个性和性格的途径

我本人特别反对一种颇为常见的观点，就是“一个良好运作的项目，不同的人，应该写出一样的代码”。我非常理解这种观点的初衷，一个良好规范约束的团队中，不同的人写出来的代码应当一致。但实际上，能真正这样做的团队，我还根本没有见到过。所谓的一致代码，仔细品味，发现不同的工程师写出来就是不一致。因此“一致”这个词一定是在一定程度内的大体描述而已，并非越一致约好。其实，代码的创造本就是具备个性的。毫无疑问我们应当遵从规约，应当符合习惯，但是一旦试图过度使用它们去约束代码，不但难以落实，而且容易产生无趣、低效和矛盾的氛围。

再回到代码评审上。代码评审本身，以及基于评审意见的来回沟通，和代码本身比起来，要更难以，也更不应该要求一致。我见过许多代码评审的风格，有人喜欢挑小毛病，有人喜欢展开观点夸夸其谈，有人喜欢给实际例子来证明自己的看法……无论哪一种风格，我都不觉得有什么大的问题。但是，就我个人而言，我可以谈谈我自己的代码评审风格：

我会关注三种问题，需求和业务上的问题、代码结构的问题、代码风格格式的问题。

前两种可能存在阻碍我ship代码的“严重”问题（说“ship”就意味着认可代码具备了push到主线分支的条件了）。我已经记不清多少次和代码作者因为这样的问题争论了。争论是个艺术，有时候并不一定能够达成一致，有的人比较容易被说服，有的人则更坚持己见。我不想说哪一种更好，但是确实这是代码评审中展现风格的事实——都是就事论事，但有人害怕或者不喜欢得罪人，就会显得push over一点；有人则不在乎那么多，坚信自己的想法更合适，就会显得defensive一点。我可能属于后者，似乎在职业生涯的各种阶段都会有和我出现代码评审冲突的事例，但是在某些情况下我也会选择disagree and commit（不同意但是执行团队达成的意见）。我相信有些团队会喜欢我的backbone，也会有团队讨厌我的这种风格。下面的内容，也多为基于自己风格的表述。

坚定自己的意见，但是委婉地表达

关于这一点，我也在努力地改进。可以提及的点很多，技巧也很多，但是老实说，冲突还是不可避免。在我经历的几乎所有的团队中，有时候会有老好人，但是基本上所有的老好人都缺少对于原则的坚持。沟通是门艺术，在代码评审中也一样体现。

最重要的一条，只针对代码，不针对人。这条很简单，都知道对事不对人的重要性，但是要非常小心不能违背。
对于大多数代码风格格式上的问题，我都会标注这个问题是一个picky或者nit的问题（“挑剔的问题”，这是我在Amazon的时候学到的方式）。这样的好处在于，明确告知对方，我虽然提出了这个问题，但是它没有什么大不了的，如果你坚持不改，我也不打算说服你。或者说，我对这个问题持有不同的看法，但是我也并不坚信我的提议更好。
使用也许、或许、可能、似乎这样表示不确定的语气词（包括使用虚拟语气）。这样的好处是，缓和自己观点表达的语气。比如：“这个地方重构一下，去掉这个循环，也许会更好”。
间接地表达质疑。比如说，看到对方用了一个参数3，但是你觉得不对，但又不很确定，可以这样说：“我有一个疑问，为什么这里要使用3而不是其他值？”对方可能会反应过来这个值选取得不够恰当。
放上例子、讨论的链接，以及其它一些辅助材料证明自己的观点，但是不要直接表述观点，让对方来确认这个观点。比如：“下面的讨论是关于这个逻辑的另一种实现方式，不知你觉得是否会稍简洁一些？”
先肯定，再否定。这个我想很多人一直都在用，先摆事实诚恳地说一些同意和正面的话，然后用however一转，说出相反的情况，这样也就在言论中比较了pros和cons，意味着这是经过trade-off得出的结论。
……

一些很常见的可以在代码评审中提及的问题

这样的问题其实有不少，多数和实现的技术无关，但是又很容易不小心略过。它们多数时候是问题，当然也有时候不是。

比如说，我最痛恨的之一，职责过于宽泛或者职责不清的类或模块。我无数次见过这样的类：Helper，或者Utils（注意，它们都没有模型或者模块前缀）。考虑项目的规模，大多数情况下，这样的类很容易变成一个越吹越大的气球，似乎什么东西都可以往里搁，是个十足的违反单一职责原则的糟糕设计。
比如说，在线程使用，容器使用，连接管理……中，缺乏上限控制的设计，在一些情况下导致资源使用过度膨胀。生产者和消费者的队列设计，一旦消费者挂掉或者跟不上，队列里越堆越多，没有拒绝机制。
再比如说，缺少分包、分层，所有的东西都叠在一起，十几个，甚至几十个类文件并列在同一个文件夹下面。
再再比如说，代码缺乏设计，流水账结构，面条型代码，或者简单铺陈几个过程式的方法，这种修改往往代价还不小，自然修改的落实率低，因而令提出问题的人也颇为头疼。
……

避免一次评审提及过多的问题

少数情况下，手里拿到一份实在是问题多到令人发指的代码，这时候需要扼制自己想骂人的冲动。先抓问题的主干，不要去挑那些细枝末节的毛病，因为很有可能，这些代码会被改得面目全非，或者重写。写一大堆评审意见，反而容易淹没最重要的问题。

说说容易，但是这个度有时候并不好掌握。我的习惯是，在明确代码要解决的问题以后，先快速走读一遍代码，判断我是应该粗略地抓主要矛盾呢，还是应该严格地挑刺呢。我也见过一些别的方式。

不一样的评审要求

我始终觉得，代码评审的要求不应该完全一致。不要过度追求公平。对于新项目代码，以及新员工写的代码，要适当严格一些。

新项目的代码是形成后续风格和质量的模板。我们也许都听说过“破窗户原理”，在一块干干净净的代码田地里，新耕种的代码才容易保持一致，也可以避免一些“为了和现有代码风格保持一致”而导致质量妥协的情况出现。

新员工代码的高质量要求则更多地在于对他们良好习惯养成的负责。软件工程师良好职业习惯的养成，代码可以说是最重要一环，而前三个月的影响举足轻重。

还不如不做的评审

有些情况下，代码评审是非常耗时费力的工作。特别是对业务背景不熟悉，对实现技术不熟悉，或者干脆是对方提交上来一大堆修改，阅读非常费力。我不知道哪一种是最难的，但是如果因为这些原因很难做到良好的评审，我会在评审中说明，或者放弃一些评审的工作，保证评审的质量。

代码评审是建立在团队中影响的好方式。一方面是业务逻辑，一方面是代码结构，我反对在两方面都难以给出足够清晰的评审意见的时候，提一堆风格方面的次要问题。否则很容易达成负面影响。

先谈到这里，我想还有不少可以涉及的内容，而它们都难以在课堂和书本上学到。争论才有意思，实践出真知大抵如此吧。你有什么一致的看法，或者反驳，欢迎和我讨论。

本文文字及图片出自 www.raychase.net