如何做人性化的代码审查？从高到低用例子

Posted 2020-11-10 排序和map

（点击上方公众号，可快速关注）

 

编译：精算狗，英文：Michael Lynch

 

最近，我一直在读有关代码审查最佳范例的文章。我注意到这些文章的关注点是找到 bug，而忽略了代码审查其他的部分。用建设性、专业的问题沟通方式？不相关！只要识别出所有的 bug，剩下的部分会水到渠成。

 

我只能假设我读过的这些文章都来自未来，那时候所有的开发人员都是机器人。在那个世界，你的队友欢迎对其代码未经过推敲措辞的批评，因为处理这样的信息能温暖他们冰冷的机器人之心。

我要做一个大胆的假设，你想要在当前世界改进代码审查，此时你的队友都是人类。我还要做一个更大胆的假设，你与同事之间积极的关系本身就是一个目的，而不仅仅是一个可调整的变量来最小化缺陷的平均成本。在这些情况下，你的审查实践会发生怎样的变化呢？

在这篇文章中，我讨论了一些技巧，把代码审查既看作是技术过程，也看作是社会过程。

什么是代码审查?

“代码审查（code review）”这一术语可以指一系列活动，从简单地站在队友身后读读代码，到 20 人与会的单行代码分析。我用这一术语指正式的、书面的过程，但也不像一系列现场代码审查会议那么重大。

代码审查的参与者包括作者以及审查者：作者写代码并把代码送去审查，审查者读代码并决定代码什么时候就绪并入团队的代码库。一次审查可以由多个审查者完成，但是我做了简化的假设——你是唯一的审查者。

在代码审查开始之前，作者必须创建一个变更表。作者想要将源代码并入团队代码库，变更表包括一系列源代码的变更。

当作者把变更表发给审查者时，审查就开始了。代码审查是循环发生的。每个循环都是作者与审查者之间完整的往返：作者发送变更，审查者给予变更的书面反馈。每次代码审查都包括一次或者更多的循环。

当审查者批准了这些变更，审查结束。这通常指的是给出 LGTM，“我觉得不错（looks good to me）”的简写。

这为什么很难？

如果程序员给你发了一份变更表，他们觉得这个变更表棒极了。你又给他们写了一份详细的清单，解释为什么这个变更表并不好。这是需要小心处理的信息。

这是我不想念 IT 的一个原因，因为程序员是非常不可爱的人……比如，在航空业，那些过分高估了自己技术水平的人都死了。

Philip Greenspun，ArsDigita 的联合创始人，引自《Founders at Work》。

作者容易把对其代码的批评解读为暗示他们不是合格的程序员。代码审查是一个分享知识和做工程决定的机会。但是如果作者把讨论理解为个人攻击，这个目标无法达成。

除此之外，你还面临着书面传达想法的挑战，词不达意的风险会更高。作者听不到你的语气，也看不到你的肢体语言，所以清晰地、小心地传达你的反馈更为重要。对一个有戒备心的作者来说，一句无冒犯意味的批注，比如“你忘了关闭文件句柄”，可以被理解成“真不敢相信你忘了关闭文件句柄！你真是个傻子。”

技巧

1. 让电脑做无聊的部分

2. 用风格指南平息风格争论

3. 马上开始审查

4. 从高级别开始，逐步向下

5. 慷慨地使用代码示例

6. 永远别说“你”

7. 把反馈表达成请求，而不是指令

8. 把批注与原则联系在一起，而不是观点

让电脑做无聊的部分

在会议和邮件的干扰下，可用来专注于代码的时间很少。你的精神毅力更是短缺。读队友的代码是认知上的负担，要求高强度的专注。别把这些资源浪费在电脑能做的任务上，尤其是当电脑能做得更好的时候。

空白错误是一个显著的例子。比较一下人类审查者找到缩进错误并与作者一起改正所花费的精力，和仅仅使用一个自动排版工具所花费的精力：

人类审查者需要的精力	排版工具需要的精力
1.审查者寻找空白错误，找到错误的缩进   2.审查者写批注，指出错误缩进 3.审查者重新读批注，确保措辞清晰，不含指责意味 4.作者读批注 5.作者改正代码缩进 6.审查者核实作者适当地处理了批注	无！

右边是空的，因为作者用了一个代码编辑器，每次他们点击“保存”时，该代码编辑器会自动规定空白的格式。在最糟的情况下，作者把代码发出去以供审查，持续集成解决方法报告说空格错误。作者在不需要审查者顾虑的情况下，修正这个问题。

在代码审查中寻找可以被自动解决的机械性任务。以下是常见的例子：

任务	自动解决方法
验证代码的构建	持续集成方法，比如 Travis 或者 CircleCI
证实通过了自动测试	持续集成方法，比如 Travis 或者 CircleCI
验证代码空白与团队风格一致	代码排版器，比如 ClangFormat (C/C++ 排版器) 或者 gofmt (Go 排版器)
识别未使用的输入或者变量	代码 linter，比如 pyflakes (Python linter) 或者 JSLint (javascript linter)

自动化使你作为审查者能做出更多有意义的贡献。当你能忽略一整个类别的问题，比如输入的排序或者源文件命名的约定，你能够关注更有趣的事情，比如函数错误或者可读性缺陷。

自动化也能给作者带来好处。自动化使作者用几秒钟发现粗心的错误，而不是几小时。即时反馈使得从错误中学习更容易，修正错误的代价也更小，因为作者脑海中还有相关的背景。另外，如果他们不得不听到自己犯下的愚蠢错误，对自尊心来说，从电脑那听到要比从你那听到更容易被接受。

和你的团队一起将这些自动检查加入代码审查的工作流程中（例如，在 Git 中的 pre-commit hooks 或者 Github 中的 webhooks）。如果审查过程要求作者手动运行这些检查，你会损失大部分好处。作者总是会忘记一些情况，迫使你继续审查简单的问题，而这些问题本来就能被自动处理。

用风格指南平息风格争论

关于风格的争论浪费了审查的时间。一致的风格确实重要，但是代码审查不是争论花括号位置的时候。在审查中消除风格争论的最佳办法是，遵守一个风格指南。

好的风格指南不仅定义了像命名习惯或者空白规则这样的表面元素，而且定义了怎样使用给定编程语言的特征。比如，JavaScript 和 Perl 都包含了一些功能——他们提供了许多实现相同逻辑的方法。风格指南定义了做事的唯一方法，这样不会以一半队员用了一组语言特征而另一半队员用了完全不同的一组特征收尾。

一旦有了一个风格指南，你就不需要浪费审查循环，来跟作者争论到底谁的命名习惯最好。只要遵从风格指南然后继续就行。如果你的风格指南没有指定某个特定问题的约定，那它一般都不值得争论。如果你遇到一个风格指南未涉及的问题，它又重要到需要讨论，和团队一起推敲。然后把决定加到风格指南，这样你们永远不需要再进行一次这个讨论。

选择 1：采纳一个现存的风格指南

如果从网上搜索，你能找到已发布的风格指南可供使用。Google 的编程风格指南是最知名的，但是如果它的风格不适合你，你可以找到其他的指南。通过采纳一个现存的指南，不需要从头创造一个风格指南的大量花费就能继承其好处。

坏处是组织为他们自己特别的需要优化其风格指南。比如，Google 的风格指南在使用新语言特征上比较保守，因为他们有一个巨大的代码库，其中的代码要在所有东西上运行，从家用路由器到最新的 iPhone。如果你们是一个只有一个产品的四人小组，你可能选择在使用前沿语言特征或者扩展时更大胆。

选择 2：不断创造你自己的风格指南

如果你不想采纳现存的指南，你可以自己创造一个。在代码审查中每产生一次风格争论，向整个团队提问来决定官方约定应该是什么。当你们达成共识，把决定编进风格指南中。

我倾向于将团队的风格指南作为源控制下的 Markdown（例如 GitHub 页面）。这样，对风格指南的任何改动都需要通过普通的审查过程——某人得明确批准改动，而且团队中的每个人都有提出疑虑的机会。Wikis 和 Google 文件都是可接受的选择。

选择 3：混合方法

合并选择 1 和选择 2，你可以采纳现存的风格指南作为基础，然后用本地风格指南来扩展或者覆盖这个基础。一个好例子是 Chromium 的 C++ 风格指导。它用 Google 的 C++ 风格指导作为基础，但是在其上添上自己的改动和附加。

马上开始审查

将代码审查视为高优先级。当你真正阅读代码并反馈时，慢点来，但是要马上开始审查——最好在几分钟内开始。

如果队员发给你一个变更表，这可能意味着直到你完成审查前，他们会卡在其他工作上。理论上，源控制系统使作者能建起新的分支，继续工作，然后从审查中把变动合并进新分支。实际上，一共有大约四个开发者能够高效地做这件事。其他人要花很长时间来清理三方差异，以致于抵消掉了等待审查完成这段时间里的进步。

你马上开始审查，就创造了一个良性循环。你的审查时间完全变成了一个与作者的变更表大小和复杂度相关的函数。这激励作者发送短小、范围狭窄的变更表。对你来说这样的变更表审查起来更容易，也更愉悦，所以你能更快地审查，循环继续。

想象一下你的队员要执行一个新特征，这个特征要求 1000 行代码变更。如果他们知道你能在大概 2 小时内完成一个 200 行的变更表的审查，他们可以把特征拆分成各包含 200 行的变更表，然后在一两天内检查完整个特征。但是，如果无论大小你都要花一天来完成所有的代码审查，现在就要花一周时间才能检查完整个特征。你的队员不想傻坐一周，所以他们被激励着去发送更大的代码审查，比如每个包含 500 到 600 行。这样审查起来花销更大，反馈也更差，因为记 600 行变更表的背景要比 200 行变更表难。

一个审查循环的最大周期应该是一个工作日。如果你正在处理一个更高优先级的问题，不能在一天内完成一个审查循环，让你的队员知悉并给予他们把审查交给别人的机会。如果你一个月被强制回绝审查超过一次，可能意味着你的团队需要放慢脚步，这样你能保持理智的开发实践。

从高级别开始，逐步向下

在一个既定的审查循环中，你写的批注越多，让作者感觉受打压的风险越大。准确的界限随开发者的不同而不同，但是一个审查循环中 20 到 50 个批注一般是危险区的开始。

如果你担心把作者淹没在批注的海洋里，约束你自己在早期循环中反馈高级别的问题。注意重新设计类接口或者拆分复杂函数这样的问题。等到这些问题都解决了再去处理低级别的问题，比如变量命名或者代码评论的清晰度。

一旦作者整合了你高级别的批注，低级别的批注可能会变得无意义。把低级别的批注推迟到后期的循环中，你可以把自己从小心措辞的工作中解救出来，也免得作者处理不必要的批注。这个技巧也细分了审查过程中你所关注的抽象层，帮助你和作者用清晰、系统的方法完成变更表。

慷慨地使用代码示例

在一个理想的世界里，代码作者会感谢收到的每一次审查。这是他们学习的一个机会，也能防止他们犯错。事实上，有许多外部因素能导致作者负面地解读审查，怨恨你给他们批注。可能他们正面临着截止日期的压力，所以除了立刻不经审查的批准以外的东西都感觉像阻碍。可能你们没怎么在一起工作过，所以他们不相信你的反馈是好意的。

一个让作者对审查过程感觉良好的方法是，在审查中找机会送他们礼物。所有开发者都爱收到的礼物是什么呢？当然是代码示例啦。

如果通过写一些建议的改动来减轻作者的负担，就证明了作为审查者，你对时间很慷慨。

比如，想象一下你的一个同事不熟悉 Python 的列表推导（list comprehension）特征。他们给你发送了包含以下代码的审查：

urls = []

for path in paths:

  url = ‘https://‘

  url += domain

  url += path

  urls.append(url)

 

回复“能用列表推导（list comprehension）简化这个吗？”会使他们苦恼，因为现在他们得花 20 分钟搜索他们之前从没用过的东西。

收到像以下这样的批注他们会更开心：

考虑用像这样的列表推导（list comprehension）来进行简化：

urls = [‘https://‘ + domain + path for path in paths]

 

这个技巧并不局限于单命令程序。我会经常建立我自己的代码分支，向作者展示概念的一个大型证明，比如拆分一个大型函数或者增加一个单元测试来覆盖一个附加边界情况。

为清晰、无争议的改进保留此技巧。在上面列表推导（list comprehension）示例中，极少有开发者会拒绝减少 83% 的代码行数。相反，如果你写了一个冗长的示例来演示某个变动“更好”，而这个变动是基于你自己的个人品味（比如，风格变动），代码示例让你看起来固执己见，而不是慷慨大方。

限制你自己在每个审查循环中只写两到三个代码示例。如果你开始为作者写整个变更表，这标志着你觉得作者没能力写自己的代码。

永远别说“你”

这听起来挺怪异的，但是听我说：永远别在代码审查中使用“你”这个字。

在审查中做的决定应该是基于什么能让代码更好，而不是谁出的主意。你的队员在他们的变更表中倾注了大量心血，而且很可能为自己的工作感到骄傲。他们听到对其工作的批评，自然反应是摆出防御和保护的姿态。

组织反馈所使用的措辞，以最小化激起队员戒备心的风险。讲清楚你是在批评代码，而不是程序员。当作者在评论中看到“你”这个字，会将他们的注意力从代码转移到自己身上。这增加了他们把批评私人化的风险。

考虑一下这个无害的评论：

你拼错了“successfully”。

作者可以把这个批注理解成两种不同的意思：

• 理解 1：嗨，好家伙！你拼错了“successfully”。但是我还是觉得你聪明！那可能就是个笔误。

• 理解 2：你拼错了“successfully”，笨蛋。

把这个跟省略了“你”的批注比较一下：

sucessfully -> successfully

后者是一个简单的修正而不是对作者的审判。

幸运地是，在重新写反馈时避免使用“你”并不难。

选择 1：用“我们”替换“你”

你能重命名这个变量，让它更具有描述性吗？比如 seconds_remaining。

变成：

我们能重命名这个变量，让它更具有描述性吗？比如 seconds_remaining。

“我们”加强了团队对代码的集体责任。作者可能跳槽到一个不同的公司去，你也可能，但是拥有这个代码的团队会一直以不同的形式存在。当你明显期望作者自己做某些事的时候，说“我们”听起来会比较傻，但是傻要比指责好。

选择 2：移除句子的主语

另一个避免使用“你”的方法是用省略句子主语的简化句子：

建议重命名为更具有描述性的名称，比如 seconds_remaining。

你可以用被动语态实现相似的效果。我在技术写作中一般会避免像瘟疫一样使用被动语态，但是它是个有用的方法来避免使用“你”。

变量应该被重命名为更具有描述性的名称，比如 seconds_remaining。

另一个选择是把它表述为一个问题，用“……如何”或者“……怎么样”开头：

把变量重命名为更具有表述性的名称怎么样？比如 seconds_remaining。

把反馈表达成请求，而不是指令

代码审查相对平常的交流来说，要求更多的机智和谨慎，因为存在高风险把讨论转变成私人争论。你会期望审查者在审查中表示出礼貌，但是奇怪地是，我发现他们走向了另一个方向。多数人永远不会对同事说“给我订书机，再给我拿瓶汽水。”但是我看到过无数审查者用类似的指令来表达反馈，比如，“把这个类移到一个单独的文件里。”

宁可在反馈中恼人地绅士。把批注表达成请求或者建议那样，而不是指令。

比较用两种不同方式表达的同一个批注：

表达成指令的反馈	表达成请求的反馈
把 Foo 类移到一个单独的文件里。	我们能把 Foo 类移到一个单独的文件里吗？

人们喜欢掌控自己的工作。向作者提出请求给他们带来自主意识。

请求也让作者礼貌地反馈更容易。可能他们的选择是有合理的。如果把反馈表达成指令，来自作者的任何反馈都像违反指令。如果你把反馈表达成请求或者问题，作者能简单地回答你。

比较对话的好斗程度，取决于审查者怎么表达他们的初始批注：

表达成指令的反馈（好斗的）	表达成请求的反馈（合作的）
审查者：把 Foo 类移到单独的文件里 作者：我不想这么做，因为那样就离 Bar 类太远了。客户几乎总会一起调用他们。	审查者：我们能把 Foo 类移到单独的文件里吗？ 作者：可以，但是那样就离 Bar 类太远了，以及客户一般会一起使用这两个类。你觉得呢？

看看当你构建虚拟对话来证明观点把批注表达成请求而非指令的时候，对话变得多么有礼貌。

把批注与原则联系在一起，而不是观点

当你给作者写批注时，既要给出变更建议，也要给出变更的理由。“现在，这个类既负责下载文件，也负责解析文件。我们应该依照单一责任原则，把它拆分成一个下载类和一个解析类。”这么说会更好，而不是说“我们应该把这个类分成两个。”

让你的批注有原则性的立足点，这样能让讨论走向更积极的方向更有建设性。但你有一个具体的原因，比如“我们应该把这个函数写成私有函数，来最小化 public 借口类”，作者就不能简单地回复“不，我倾向于我的方法。”更确切地说，他们可以，但是因为你演示了改动如何满足目标，而他们只陈述了一个偏好，他们会看起来很傻。

软件开发既是艺术也是科学。你不可能永远都能用确定的原则来明确表达代码到底哪里出了问题。有时候代码只是难看或者不符合直觉，不容易确定为什么。在这些情况下，解释你能怎么做，但是保持客观性。如果你说“我发现这不容易理解”，这至少是个客观的陈述；相反，“这莫名其妙”是一个价值判断，不一定适用于所有人。

尽可能以链接的形式提供支持证据。团队风格指南的相关部分是你能提供的最佳链接。你也可以链接到语言或者库的文件。高票 StackOverflow 回答也行，但是离权威文件越远，你的证据变得越不稳固。

第二部分：即将上线

敬请期待其他小技巧，包括：

• 处理特别大的代码审查

• 识别给予表扬的机会

• 尊重审核的，以及

• 化解僵局

由 Samantha Mason 编辑。插图来自 Loraine Yow。感谢 @global4g为这篇文章的早期版本提供宝贵的反馈。

觉得本文有帮助？请分享给更多人

关注「算法爱好者」，修炼编程内功