代码Review最佳实践

在实际工作中，经常会遇到项目交接或者二次开发的情况，在这个过程中，我们经常会听到“这是什么垃圾代码啊”。有时候我们翻看自己几年前写的代码，也会忍不住鄙视自己。

在软件开发过程中，代码Review是一个可以提高代码质量，统一代码规范，分享技术知识，从而形成增长团队的有效手段。

在代码Review过程中，存在两个角色：

• 提交者。提交者就是代码的提交人，他发起了Review事件。同样也可以称作被审查者。
• 审查者。审查者是对代码进行Review的人。

在本文中，主要涉及了以下内容：

• 为什么要代码Review
• 何时代码Review
• 准备代码Review
• 进行代码Review
• 代码Review示例

动机

通过代码Review可以提供代码质量，并且我们还可以通过代码Review来提高自我的能力。
比如：

• 通过代码Review，审查人员可以看到本次变更的内容：处理TODO，代码优化等。提交者的代码被认可，可以提升自我成就感。
• 可以分享知识：
  • 代码Review可以是提交内容更加明确，并且使团队成员更进一步了解项目，为以后的开发做知识积累
  • 团队成员可以从提交者的代码中学习新的技术、算法等等
  • 通过代码Review，提交者可以从审查人员的评审中获得相关的技术知识
  • 可以增加团队交流，形成增长团队
• 可以形成统一的代码规范，方便阅读和理解
• 审查者因为没有完整的上下文，只看到代码片段，更容易发现问题，提高代码片段的可复用率
• 更容易检查拼写错误
• 可以避免常规的安全问题等

Review什么

对于代码Review什么内容，可以有很多的方面，如：变量命名、代码结构、算法、架构、安全等等。具体内容没有一个统一的标准，但是在一个团队中，是需要形成一个统一的标准的，这样更有益于团队的可持续发展。

什么时候Review

代码需要在测试、CI之后，在合并上线分支之前。测试、CI等确保了逻辑是正确的。因为需要保证线上的代码是最优的，所以Review需要在合并分支之前。

准备Review

提交者需要提交一个便于Review的代码，避免浪费审查者的精力和时间：

• 范围和大小。一次提交Review的代码不应过大，如果太大需要耗费一天的时间，那就说明提交Review的代码不够合理，应分解成多次Review提交。
• 只提交已完成的，并且自检及自测过的代码。提交Review的代码，一定是已经开发完的，否则Review将没有意义。它也一定是经过自测的代码，对没有通过自测的代码进行Review，同样没有意义。
• 重构不应该改变代码行为，同样改变代码行为的不应该包含重构内容。每次提交的变更目标应该是明确的，且是单一的，不能将重构和开发新功能合并到一起提交。

进行Review

代码Review一定要及时，不能因为卡在没有进行Review而影响项目进度。如果审查者时间不允许，应立即告知提交者，让他找其他人对代码进行Review。

作为审查者，有责任执行编码标准并保持质量水准。 审查代码更多是一门艺术，而不是一门科学。 学习它的唯一方法就是去做。 有经验的审查者需要考虑让经验不足的审查者先Review，以此来提高他们的Review经验。

假设提交者遵循上面的指南（尤其是关于自我检查并确保代码可以运行的准则），审查者在代码Review过程中应注意的事项应注意一下事项：

• 目标
  • 这段代码是否达到了提交者的目的？ 每次更改都应有特定的原因（新功能，重构，错误修正等）。 提交的代码是否真的达到了这个目的？
• 提问
  • 函数和类应该存在是有原因的。 当原因对于审查者来说不清楚时，这可能表明该代码需要重写、添加注释等等。
• 实现
  • 考虑一下您将如何解决问题。 如果不同，那为什么呢？ 您的代码可以处理更多（边缘）情况吗？ 它更短、更容易、更清洁、更快、更安全，但在功能上等效吗？ 您发现当前代码未捕获的异常了吗？
  • 您看到有用的抽象的潜力吗？ 部分重复的代码通常表示可以提取出更抽象或更通用的功能，然后在不同的上下文中重新使用。
  • 像对手一样思考，但要对此保持友善。 尝试通过提出有问题的配置、输入数据来破坏他们的代码，从而找出程序里面的漏洞。
  • 考虑库或现有产品代码。 当某人重新实现现有功能时，通常是因为他们不知道该功能已经存在。 有时，有意复制代码或功能，例如，以避免依赖。 在这种情况下，代码注释可以阐明意图。 现有库是否已提供引入的功能？
  • 更改是否遵循标准模式？ 既定的代码库通常表现出围绕命名约定，程序逻辑分解，数据类型定义等的模式。通常希望根据现有模式来实现更改
  • 更改是否添加了编译时或运行时依赖项（尤其是在子项目之间）？ 我们希望保持我们的产品松散耦合，并尽可能减少依赖。 对依赖项和构建系统的更改应进行严格审查。
• 易读性与风格
  • 考虑一下您的阅读经验。 您是否在合理的时间内掌握了这些概念？ 流程是否合理，变量和方法名称是否易于理解？ 您是否能够跟踪多个文件或功能？ 您是否因名称不一致而推迟？
  • 该代码是否遵守编码准则和代码样式？ 代码在样式，API约定等方面是否与项目一致？ 如上所述，我们更喜欢使用自动化工具解决代码规范。
  • 此代码是否有TODO？ TODO只是堆积在代码中，并且随着时间的流逝变得陈旧。 让作者在GitHub Issues或JIRA上提交记录，并将发行号附加到TODO。 建议的代码更改不应包含注释掉的代码。
• 可维修性
  • 阅读测试。 如果没有测试，应该进行测试，请提交者写一些测试。 真正不可测试的功能很少见，而不幸的是，未经测试的功能实现很常见。 自己检查测试：它们是否涵盖了有趣的案例？ 它们可读吗？ CR是否会降低总体测试覆盖率？ 考虑一下此代码可能如何破解。 测试的样式标准通常与核心代码不同，但仍然很重要。
  • 此CR是否存在破坏测试代码，登台堆栈或集成测试的风险？ 这些通常不作为预提交/合并检查的一部分进行检查，但是让它们崩溃对每个人来说都是痛苦的。 要查找的特定内容是：删除测试实用程序或模式，配置更改以及工件布局/结构更改。
  • 此更改会破坏向后兼容性吗？ 如果是这样，此时可以合并更改，还是应该将其推送到更高版本中？ 中断可能包括数据库或架构更改，公共API更改，用户工作流更改等。
  • 此代码是否需要集成测试？ 有时，单独使用单元测试无法对代码进行充分的测试，尤其是当代码与外部系统或配置交互时。
  • 留下有关代码级文档，注释和提交消息的反馈。 多余的注释使代码混乱，而简短的提交消息使将来的贡献者迷惑不解。 这并不总是适用，但是高质量的评论和提交消息将使他们自己付出代价。 （想想您曾经看到过出色的或真正可怕的提交信息或评论。）
  • 外部文档是否已更新？ 如果您的项目维护自述文件，CHANGELOG或其他文档，是否已对其进行更新以反映更改？ 过时的文档可能比没有文档更令人困惑，并且将来对其进行修复要比现在进行更新要花费更多的成本。
• 安全
  • 验证API端点是否执行与其余代码库一致的适当授权和身份验证。 检查其他常见弱点，例如弱配置，恶意用户输入，缺少日志事件等。如有疑问，请向应用程序安全专家咨询Review。
• 评论
  • 简洁、友好、可操作的。不要忘了赞扬简洁、可读、高效、优雅的代码。 相反，拒绝或不批准代码Review并不粗鲁。 如果更改是多余的或无关紧要的，请拒绝并说明。
• 面对面Review
  • 对于大多数代码检查而言，基于异步差异的工具（例如Reviewable，Gerrit或GitHub）都是不错的选择。 当在同一台屏幕或投影仪前亲自进行或通过VTC或屏幕共享工具远程执行时，复杂的更改或具有不同专业知识或经验的各方之间的评论可以更有效。

示例

在以下示例中，建议的评论注释在代码块中由 // R：... 注释标识。

命名不一致

class MyClass {
  private int countTotalPageVisits;  //R: 变量命名不一致
  private int uniqueUsersCount;
}

方法签名不一致

interface MyInterface {
  /** Returns {@link Optional#empty} if s cannot be extracted. */
  public Optional<String> extractString(String s);

  /** Returns null if {@code s} cannot be rewritten. */
  //R: 应该协调返回值：在这里也使用Optional <>
  public String rewriteString(String s);
}

类库使用

//R: 使用Guava's MapJoiner替换以下方法
String joinAndConcatenate(Map<String, String> map, String keyValueSeparator, String keySeparator);

个人倾向

//R: nit: I usually prefer numFoo over fooCount; up to you,
//  but we should keep it consistent in this project
int dayCount;

Bugs

//R: 代码处理numIterations+1的情况，如果是故意这样处理，是否考虑变更numIterations值
for (int i = 0; i <= numIterations; ++i) {
  ...
}

架构疑虑

//R: I think we should avoid the dependency on OtherService.
// Can we discuss this in person?
otherService.call();

总结

通过有效的代码Review，可以提高项目代码质量，使团队开发人员形成统一风格，并同步项目细节。同时还可以提高团队人员的知识，提升自我。