前端工程化系列六：Code Review

在维护较大规模项目时，我们非常有必要投入足够的时间精力构建一套健壮且适用的自动化质检系统，这能以非常低的成本自动化地规避许多低级错误，但工具都是有能力边界的，许多规范规则之外的质量问题依然强依赖于人工参与。在集成合码阶段，这意味着需要由代码作者之外的工程师参与审阅代码的变化情况，确认无误后方可合入主干分支，也就是所谓的 Code Review。

1. 邀请 Reviewer

严格来说，Code Review(简称 CR)并不是工程化手段而是一种管理方法，具体的实操方式比较灵活多变，例如有些团队会以需求粒度定期发起线下 CR 会议，线下聚集团队成员集中讨论变更代码的实现好坏，直至与会大部分成员通过后方可合入主干；有些团队则要求代码开发完毕后在 Gitlab/Github 系统上创建 Merge Request，之后由 Reviewer 审阅通过后合入主干。

图例：Github Pull Request 功能示例。

无论哪种方式，重点一是必须有合适的第三者参与 Review，二是 Review 动作必须形成流程卡点，直至审阅通过后才可合入。那么，谁是合适的 Reviewer 呢？在真实工程中会有许多具体的评估标准，不一而足，但有几个值得参考的维度：

1. 技术专家：也就是团队中技术能力比较强，经验比较丰富，并且对代码质量有较高要求的工程师，这部分同学往往能从纯粹技术视角考察代码实现的优缺点、可读性及潜在风险等，例如识别出某些因缺少 useMemo 调用导致数据重复计算的情况；或者，识别出某些新增三方依赖的质量问题并给出更佳的解决方案；又或者，识别仓库中不断重复出现的代码模式，并推动提升为通用基础模块；
2. 业务专家：团队中对特定业务规则理解度比较深的同学，通常由团队负责人或业务骨干担任，这部分同学能从业务视角考察代码实现是否能正确满足业务预期，特别是对一些边界场景的处理是否合理，例如是否缺失对某些错误场景的交互处理；表单字段的校验规则是否满足业务约束；部分代码逻辑是否可复用代码仓库某些存量模块等等；
3. 模块专家：对本次提交所涉及变更模块熟悉度最高的工程师，一般是模块的原作者或者核心维护者。上述技术专家、业务专家擅长从宏观视角考察变更的合理性，但聚焦到具体模块时可能会因为缺失相关历史上下文而无法给出确切的反馈，此时若有模块专家补位则能结合历史背景给出参考性更强的意见建议。

注意，这三类角色只是一种模糊的分工模式，并非始终泾渭分明，在不同场景下可能常常出现角色互换，关键点还是在于 CR 时，我们期望尽可能引入擅长不同能力维度的角色，能从不同视角对代码做出合理评审，并最终推导出更立体更具建设性的参考意见，帮助每一次变更达到更好的质量水平。

2. Code Review 最佳实践

CR 最主要的目标就是在工具边界之外借助人为审查方式保证代码质量，但“人”天然具有高度随机不稳定性，同一件事情常常会因为经验、环境、情绪、压力等条件不同而产生不同反应，在 CR 时并不总能始终如一产出高质量的审核意见，为了应对这种不稳定性，通常可以在团队范围内整理一些文档性质的 CR 规范，明确约定 Review 时需要做什么、关注什么等等。

网络上已经有非常多公开资料探讨这方面的话题，有兴趣的同学推荐阅读《The Standard of Code Review》一文，这里不赘述细节，聊一些我个人认为比较重要的最佳实践：

• 没有完美的设计！CR 过程不应该吹毛求疵，只要代码能满足业务需求并且大体上能提升 —— 甚至仅仅只是不降低整体代码质量，那么就应该倾向于通过；
• CR 时应侧重于代码实现的整体设计合理性，而不应过度关注具体代码行、变量名等细节(此类问题应通过自动化工具约束)；
• 收到 CR 邀请时，对于不熟悉的模块，或不熟悉的技术栈应及时拒绝，鼓励代码贡献者找到更合适的 Reviewer 完成审查动作；
• 应该将 CR 看作工作中重要组成部分，每天预留足够多的时间专注在 CR 任务上；
• 关注业务、性能、错误等方面的埋点是否能够覆盖关键业务路径，这是非常容易遗漏的点；
• 如果你在某个 Merge Request 中识别到明显优点 —— 代码结构、设计、风格、注释，任何方面的优点都应该广而告之，正面肯定这些优点并鼓励后续提交者学习这些良好实践；
• 当发现问题时，原则上应该接受“业务为先”，后置解决问题，但应该有一些方式方法追踪这些 CR 发现的问题是否得到妥善解决，例如在代码中添加 TODO 备注；
• 始终保持礼貌，这并不是什么难事！

这个列表还可以很长很长，重点在于我们应该始终保持积极、开放、严谨、专业、认真的态度参与每一次 Code Review 任务，应该将 CR 看作工作中与 Coding 同等重要的任务，充分发挥职业精神，保证仓库代码始终往更好的方向发展 —— 虽然这很理想主义。

3. 保持 Small MR

如果严格遵循 CR 要求，单个 Merge Request 中的代码变动量越大，审核时则必然需要邀请更多 Reviewer，花费更多时间做出评审，这会使得评审过程变得繁琐而漫长。更严重的问题在于，人类的专注力是有限的，单次 CR 需要 Review 的内容越多，越容易疲惫、注意力分散，进而忽略一些本该引起重视的质量问题，致使问题代码进入主干分支降低整体质量。

所以毋庸置疑的，Merge Request 体积越小越好！但现实中有时候很难做到这一点，例如某些新增的功能特性本身就是很复杂，按照传统的线性开发思维必然是需要等待所有功能开发就绪、测试无误后才能合入上线，开发周期长代码变动大，最终 CR 成本必然很高。以我个人体感而言，排除工具自动生成的代码， MR 变更控制在 500 行以内是比较理想的尺寸；2000 行已经是认知极限了，需要花费不少心力仔细阅读；超过 10000 行基本是无法 Review 的。

不幸的是，我经常遇到超过 10000 行的 MR。。。

应对这种情况最有效的策略是：分而治之，也就是将原本巨大变更拆分成若干个粒度更小的变更，分多次合入主干，有许多方式方法能达到这一目的，例如：

• Feature-Based Development 模型：虽然在功能完整开发完成之前不能合入主干分支，但我们可以在 master 之外为本次功能设定“开发主干”分支 —— 例如 integration/xxx，之后所有变更小步快跑地频繁提交 Small MR 到该分支，这些小 MR 都严格执行常规 CR 流程，经过 Reviewer 审核通过后才能合入开发主干分支，完成所有功能开发、测试动作后，再将 integration 分支一次性合入仓库主干；

• Feature Flags：上述 FBD 是一种容易理解容易执行的分拆策略，但不够优雅，后期的集成测试成本依然比较高；另一个理论可行的方案是基于 Feature Flags 将不完整的变更提前合入主干分支中。这里说的 Feature Flags 本质上是一种配置动态下发系统，之后在代码中根据下发的开关值判断是否启用某一段代码，形如：

  import { getFlags } from "@flow-infra/bot-flags";
  
  export const format = (val: string) => {
  const FLAGS = getFlags();
  if (FLAGS["enable.foo.bar"]) {
    return val + "abc";
  }
  return val;
  };

  借助 Feature Flags，我们可以将原子需求合理地拆解成若干个小型开发任务并提前合入主干，并在代码中通过 Feature Flags 值判断是否开启相关功能，之后即使这部分并不完整的功能代码被带上线，只要确保相关 Feature Flags 保持关闭状态，就可以保证这部分代码不会影响线上表现，从而达成 Small MR 效果；

PS: 这看似简单，但基于 Feature Flags 实现 Small MR 并不是一件容易的事情，首先需要具备 Feature Flags 基础设施；其次需要改变线性的开发思维与习惯，将开发任务拆解成若干小任务离散执行，配合不同开关值控制线上运行效果保证质量稳定性；最后，遵循 FF 模式后，如果没有及时删除 FF Key 反而非常容易引入大量 if-else 代码致使最终可读性下降。

当然，这些问题理论上都是可以通过工具解决的，但需要比较强大的基建团队投入比较多时间开发，务必结合团队情况仔细斟酌。

除此之外，在实际项目中还可以根据工程上下文设计出更具针对性的操作方案，不一而足，重点在于我们需要始终重视 Code Review 环节，需要采取适当的措施降低审核动作的成本，保证效率与可操作性，即使需要为此持续付出一些人力成本。

4. 构建 CR 文化

代码审查（Code Review）是在自动化质检工具之后保障代码质量的最后一道防线。良好地执行代码审查不仅能够及时发现代码中存在的设计不合理、结构混乱、违反架构规范、代码重复等问题，还能有效促进团队成员之间的技术交流。这些都是被业界广泛认可和接纳的优点。因此，许多团队都愿意投入时间和精力来制定代码审查规范。然而，不幸的是，我所见过的大多数团队在这方面的执行效果并不理想。

原因很简单，CR 带来的多数是长期、潜在且无法量化的收益，但当下却需要付出许多时间成本去执行。CR 反馈的问题“可能”可以提升质量，但也意味着当下需要花时间修复问题，反而可能影响上线节奏，甚至因此 delay。这是一个典型的回报率问题，未来的回报要在未来才有可能显现，这是不确定的，当下的投资却都是实实在在的付出，那么一旦遇到人力、时间、进度等方面的压力，CR 必定是最先被抛弃的环节。

这正是许多软件工程团队需要面对的问题：如何确保团队成员按规范严格执行 CR，及时发现代码问题？

问题出在人身上，解题的关键也同样在人身上，我认为每一位优秀、敬业的工程师都应该有足够的技术审美与代码洁癖，应该将可读性、可维护性、整洁优雅等特性看作关键的质量需求，而不仅仅堆代码满足功能需求，并在以此律己之余向外辐射提高对整体代码的质量要求。如果团队中有超过 20% 的工程师能符合上述特质，并且文化上足够开放包容，容许甚至鼓励为技术发声，那么整体技术氛围应该是比较强的，对功能需求之外的各项质量要求必然也会更重视，更有可能严格执行 CR。因此，如果条件允许，应该尽可能引入更多优秀的工程师加入团队，鼓励他们充分发出质疑的声音，久而久之自然能形成严谨务实的工程师文化。

当然，基于各种客观条件约束，这个建议很难被广泛采纳，但可以退而求其次在当下环境中逐步构建起这种重视代码质量的工程师文化，有不少比较容易执行的举措，包括：

• 建立 MR 红黑榜：假设团队内已经有了一些 MR & CR 规范，那必然有一些严格遵循规范的，也必然有一些不按规范行事的，定期整理这些分布在两端的 Case，收集记录成册，明确指出做的好与做的不好的点并在团队内广泛分享，行为一旦有了固定的、标准化的评价体系时，参与者也就更容易做出符合要求的动作；

• 收集优秀 Comments：CR 时最主要的交互方式就是在 Merge Request 上留下评论记录，团队管理者可以定期收录一些高质量的评论，同样的指出做的好的点并广泛分享，鼓励同类行为。

• 量化评估 CR 过程：“理论上”的最好方式莫过于收集 CR 过程方方面面的数据，以此建立一套数据模型，用量化指标持续评估 CR 质量，以此评估审核行为的优劣。但实际上，我们很难从数据中找到置信度足够高的北极星指标，因为 CR 是一种极具个性的主观活动，充满随机不确定性，几乎无法通过数据方式精确表述 CR 结果的好坏，不过可以退而求其次，通过数据评估 CR 过程是否足够认真合理，例如可以聚焦在如下指标：

  • CR 时间：虽然时间越长不代表质量越好，但反过来说如果 CR 时间很短，与变更代码行不成正比(例如 1W 行变更仅用 30s 就 approve)，那必然证明 Reviewer 没有花费必要时间耐心、仔细阅读代码；
  • Comments 数量：理论上，细致认真的代码审核必然会引发许多思考，可能是不解、疑惑、意见、建议等等，在许多 CR 系统中这些思考最终会通过 Comments 方式与作者产生互动，所以一般而言，Reviewer 产出的评论越密集通常意味着 CR 过程越是仔细思考过；
  • Reviewer 数量：类似的，参与 CR 的评审者 —— 特别是互动频率高的评审者越多，变更代码越是有可能被仔细审查。