如何进行 code review

写了个如何做 code review 的内部文档，改一改了贴过来。

code review 前需要知道的事情

为什么 code review 很重要？

• 尽早发现错误
• 保持项目长远的可维护性

在不同的阶段修复 bug 的成本是不同的。单元测试中发现，集成测试中发现，发版本前跑回归测试发现。由用户测试过程中发现，单个用户上了生产环境后遇到，大量用户使用之后发现。越早将 bug 扼杀在摇兰中，产生的损失是越小的。那么在 code review 时就把 bug 揪出来，这无疑是最早期的阶段，成本最低。

随着项目越来越复杂，后期的维护会是越来越困难的。如果不对代码质量把关，代码就会慢慢腐烂，直到最后无法维护。

贵司关于 code review 的一些指导性的原则

• review 代码比自己写代码更重要

现代软件开发是需要团队合作的，如今的软件已不再是一个人的艺术作品。为了加快开发速度，使开发过程可以 scale，避免阻塞非常重要。如果组织中的成员都优先编写自己的代码，那么等待 code review 的人会被阻塞，并且会有越来越多的 PR 阻塞。最终大多数开发人员都被 block，整个团队开发速度受到影响。

• 合并 PR 是作者的责任

很久以前，我们就如何加快合 PR 合并进行了激烈的争论。review 和 PR 作者，谁更应该对加速 PR 合并负责。

结论是，大多数人同意 PR 作者应该推动他的 PR 合并。他是最想将 PR 合并进去的人。既然他提了 PR，代表他是希望这个改变发生的，所以他应该尽力让别人 review 代码。负责 review 的人应该要及时 review PR 并给出反馈，但是推动 PR 合并不是他的责任。

• 2 LGTM

贵司大部分的项目，都要求 2 个 LGTM　才能被合入主干分支。有时候确实有一些例外，但默认是 2 个人 review 并同意。这可以保证即使在原 PR 作者离职，仍然有人熟悉代码，项目仍能继续。发生这种情况时，review 代码的人就是成为该代码的当前第一负责人。

• 不要给自己不理解的代码点赞

reviewer 应该足够了解代码改动，达到能够向另一个开发者详细地解释所有改动细节的程度。记住：对自己 review 的代码负责，这是关乎自己声誉的。

• 每个 PR 不超过 300 行

我们鼓励“小步快跑”的模式。300 行基本是正常人一个小时内能看懂的大小。如果 PR 过大，review 者有权力要求 PR 作者拆分 PR。对于难以拆分的 PR，review 可以要求作者开个会议讨论一遍代码改动，直到 review 的人能完全理解。

• 一个 PR 只做一件事

为了使 PR 更容易理解并更快地合并，这也是非常鼓励的。

例如，错误修复应该在单独的 PR 中，重构应该在单独的 PR 中。无论它有多小，比如仅仅更新了库的依赖，甚至更改一行修复错别字，都是只做一件事并且做对。

code review 时的原则

• 尊重事实和数据，而不是个人偏好
• 软件设计永远是 trade off，没有所谓的完美

每个人都来自不同的技术背景，拥有不同的知识。每个人有自己的个人喜好。重要的是，code review 不应该基于个人的偏见。

一个常见的错误是过分关注拼写错误，代码 style 这些。style 是很重要，但是让机器人检查就行了。人比机器人聪明，他应该关注机器人不能做的事情：

• 有没有逻辑错误
• 有没有使代码可维护性降低

有时候，做取舍可能很困难。比如，有一个特殊优化，可以提高 30% 的整体性能。但是，它引入了一个不同的特殊代码路径，会导致以后的每个新功能都针对这条代码路径特殊处理，维护负担很大。这个 PR 合不合呢？

遇到一个很紧急问题，现在有个 PR 可以修复，但是合了影响稳定性，可能有引入其他 bug 的风险。这个 PR 合不合呢？

答案总是“视情况而定”。软件设计更像是一种艺术而不是技术。它与审美，与对代码的品味有关。所以都是做取舍，有时并没有完美的解决方案。

尽管如此，对于一个改动是否应该合入，还是有一个通用标准：

• 虽然不能彻底解决问题，它至少比当前要好一点点
• 可以减轻用户遇到的问题
• 这个改动本身不会引入新的 bug

那么 PR 就是可以接受的。当然，贵司的 code review 会有一些基础要求。对于 bug 修复类的 PR，必须要带测试。对于性能相关 PR，需要有性能测试的结果。

在 code review 时需要关注哪些东西

设计

最重要的是整体设计。阅读 PR 的描述，如果不太容易理解，请编码人员改进它。它是否与系统的其余部分集成良好？现在是添加此功能的好时机吗？

功能

这个 PR 是想解决什么问题？这个功能对用户是否有用？

大多数情况下，在 review 之前 PR 应该是通过了测试的。但是，有时候还没跑通测试，也可以读代码想一想这个功能做得对不对。

必要时，可以验证一下。只是阅读代码，有时很难理解某些改动，比如 UI 的修改。如果不方便在自己环境验证，可以让作者演示一下。

逻辑

code review 的时候关注逻辑很重要，尤其是涉及到并行的代码。这类问题很难通过运行代码来检测，通常需要仔细的思考。

人脑就像一台特殊的机器，很适合在状态不多的时候，验证逻辑的正确性。状态多了就不行了，所以复杂性是软件开发最大的敌人。

需要这样来要求自己：“不用机器来运行代码，把代码在大脑里面放一遍”　或是　“review 完代码就能确定某段逻辑一定是没 bug 的”。

复杂性

PR 是否相对于它解决的问题过于复杂了？某一行代码是否太复杂？函数太复杂？类是否太复杂？

太复杂通常意味着 “代码可读性差，无法快速理解”。这也可能意味着 “开发者在尝试调用或修改此处代码时，可能会引入 bug”。

还有一类复杂性是过度设计，开发者希望使代码比需要的更通用，或者添加了目前系统不需要的功能。code review 应该特别警惕过度设计。应该鼓励解决眼前需要解决的问题，而不是推测未来可能需要解决的问题。

测试

根据变更要求进行单元、集成或端到端测试。PR 应该附带单元测试。

确保 PR 中的测试正确、合理并且有用。测试是否涵盖主要的 case？　是否可读？ PR 是否会降低整体的测试覆盖率？

想一想这段代码可能会破坏的方式。当代码被破坏时，测试真的会失败吗？ 如果代码发生变化后，测试是否会产生误报？ 每个测试是否做出简单而有用的断言？ 不同测试方法之间的测试是否适当分开？

记住，测试代码也是需要维护的。不要仅仅因为它是测试代码，就可以随意一点。

最后不要忘掉的一点，估算单元测试运行的时间。会不会拖慢 CI ？

命名

开发人员是否为所有东西都选择了好的命名？ 好的命名能充分传达它是什么或做什么，又不会因为太长而影响阅读。

注释

是否有注释？　注释是否用英文写的？ 所有的注释是否都是必要的？ 通常注释解释某些代码存在的原因，而不应该解释代码在做什么。如果代码不够清晰，无法自解释，那这段代码应该变得更简单。有时有些例外（比如，正则表达式和复杂算法通常会从解释它们正在做什么），但大多数注释用于代码本身不可能包含的信息，例如决策背后的推理。

查看 PR 之前的评论也很有用。也许可以删除一个 TODO，或是"建议不要进行此更改"的评论等。

请注意，注释与类、模块或函数的文档不同，后者应该表达一段代码的目的、应该如何使用以及使用时的行为方式。

风格

确保 PR 遵循适当的 style guide。对于 Go 和 Rust 都编译器链自带的内置工具。

如果是对于 style guide 没的规定到的地方，提建议性而非强制性的意见，并说明这条建议会对可读性有帮助，不要仅仅根据个人喜好来阻止 PR 提交。

PR 的作者不应边在一个 PR 中同时改动内容和修改排版。前面说过，一个 PR 只做一件事情。

一致性

如果现有代码风格与规范不一致怎么办？ style guide 是绝对权威：如果 style guide 有什么要求，PR 应该遵循规范。

在某些情况下，style guide 会提出建议而不是要求。在这些情况下，判断新代码是否应与建议或周围代码一致。倾向于遵循规范，除非局部不一致会太混乱。

如果没有其他规则适用，作者应保持与现有代码的一致性。

无论哪种方式，都鼓励作者提交错误并添加用于清理现有代码的 TODO。

文档

如果 PR 更改了用户构建、测试、交互或发布代码的方式，请检查它是否也更新了相关文档，包括 README 和任何生成的参考文档。如果 PR 删除或弃用代码，请考虑是否也应删除文档。如果缺少文档，应该要求作者添加。

错误处理

留心错误处理。这是最容易出现 bug 的地方之一。测试是否覆盖错误处理路径？若是后台类型的服务，发生 panic 否会使整个进程退出？代码是否正确处理了资源释放？对 Go 程序而言，尤其是涉及 goroutine 并发的代码，出错后是否出现 goroutine 泄漏或内存泄漏？或者 channel 卡死？

有时候，错误处理的代码路径很难覆盖到，但如果 error 真的发生了，很难查 bug。那么需要错误注入，来模拟覆盖错误处理的代码分支。

性能

PR 是否会对性能产生任何影响？ PR 中选择的算法是否合适？有时候，热点路径上看似无关的微小改动，也有可能会影响到性能。

如何编写代码审查评论

尊重 PR 作者

好的 reviewer 是有同理心的。你会 review 其它人代码，而你的同事也会 review 你的代码。如果把 review 视为一个学习过程时，那每个人都会成长。反之，你觉得写代码的人 SB 的时候，review 你代码的人大概率也是这么想。

提出问题而不是批评

措辞非常重要。可以提出建设性的问题，而不是用命令的口气要求对方修改。比如，“这个地方用 xxx 会不会更好一点？”，而不是 "这个地方改成 xxx"。

真诚的表扬

大多数 reviewer 只关注代码哪里不对，其实应该对于哪些做得对，做得好的多表扬和鼓励。有时候，告诉他们做对了什么更有价值。

小节

• 善待 PR 作者，但并不是放低 review 标准
• 提出问题而不是断言
• 尊重、包容并耐心地对待比你了解的少的人
• 多表扬
• 作者思路跟自己不一样时，不一定是错的
• 代码风格的问题以文档为标准

加快代码审查过程

通常，快速响应意味着更好的体验。

我们在这块仍有很大改进空间的地方，解决这个问题可能单独需要一篇文章讨论。这里有一些建议：

• 确保在接下来的几个工作日内关注 PR 的更新
• 定期查看Github 通知，随时了解PR 的更新
• PR 更新后，开始另一轮审查或给予 LGTM