simon

Code Review Guide - 代码审核规范#

Created: June 22, 2021 5:47 PM

Base on Google Code Review Practice and Zaihui Practice

术语 Term#

为简化描述，先统一术语缩写

• TLDR - Too long, Dont Read 太长不读的版本
• Author - The author of this PR 代码作者
• CR - Code Review 代码审查，也简称为 Review
• PR - Pull Request, 代码合入请求，包含 Change List 代码变更
• Reviewer - 审查代码的人
• WIP - Working In Progress 开发中，不可以被合入
• Comment
  • 本文有两种 comment
  • 代码中的 comment，是为注释
  • CR 过程中留下的 comment，是为改进意见
• Code Base - 代码库
• Deadline - 截止日期
• Judgement Call - 依据你工程师的良心进行的个人选择
• LGTM - Look good to me 看起来不错
• NIT
  • NitPick
  • 鸡蛋里挑骨头，指可以被忽略的改进建议
• SG - Style Guide, 代码风格规范
• UT - Unit Test, 单元测试
• Emergency - 紧急情况，可以放弃部分 CR 标准
  • Hotfix
  • 出现不合理的 dealine 时，但 deadline 过后，也应当补上缺失部分

目的 Purpose#

• 进行高质量快速的 Code Review，用以持续提高 Code Base 的代码质量
• 通过阅读代码或留下 comment，传授或学习新的语言、框架、设计模式等知识

原则 Standard#

• 保证 Code Base 的整体代码健康
• 满足原则 1 的情况下，尽可能的 approve PR，即便它不完美
• 持续优化比完美更重要
• 尽可能留下多的改进意见 comment，如果改进不是必须的，用 'nit:' 开头以表示不是必须改进。
• Review comment 应当尽量提供一定的改进方案，但也应当鼓励 author 发现更好的方案。
• 除非是紧急 PR，否则 PR 不应该损害 Code Base 的整体质量，何为紧急后续有描述

关注要点 What to look for#

总结 Summary-TLDR#

• 代码需要很好的设计
• 功能性应当对代码的使用者友好
• 任何的 UI 变动都必须合理且良好
• 任何并行编程都要保证其安全性
• 代码应该尽可能的简单
• 开发者不应该提前编写现在还不要的功能
• 代码应该有合适的 UT
• UT 需要有良好的设计
• 开发者应该给所有东西一个简洁清晰的命名
• 代码注释应该简洁和有用，并且解释原因，而不是解释是什么
• 代码应该被合适的文档化
• 代码要符合代码风格规范

设计 Design#

• 与其他代码的交互设计合理与否？
• 与整合系统的整合性如何？

功能性 Functionality#

• PR 的作者目的是什么？
• 这个目的对系统是好的还是不好的？
• 并发编程的是否必要，以及是否会触发死锁 (deadlock) 或者竞态（race condition）。尽量避免使用会造成死锁或竞态的并发模型。

复杂度 Complexity#

• PR 是否比起应当的过度复杂？
• 定义
  • 如果当前的设计，导致使用或者修改这些代码变得更容易引入 bug，则说明其设计过于复杂
  • 过度设计 over-engineering
    • 开发者将代码设计的比其需要的更通用
    • 或者加入了系统当前并不需要的功能
    • 未来的功能尽量留在未来解决，因为当这个需求真的需要时，其相关的客观环境和场景可能已经发生变化，导致提前写的代码并不适用。
    • PS: 提前开发没有必要，但为未来的需求在设计上留有余地是有需要的
• 分析角度
  • 独立的代码行
  • 函数
  • 类

测试 Testing#

• 通常情况下，测试必须和增加的功能代码在同一个 PR 中，除非是紧急情况
• 测试设计
  • 准则
    • 正确
    • 合理
    • 有用
    • 有效
  • 需要思考的问题
    • 数据准备是否和真实情况完全一致？不要只构造部分数据
    • 所有成功的路径是否都覆盖到？
    • 所有 400 的可能性是否都有针对的测试？返回的错误信息是否有校验与预期一致？
    • 成功的请求，除了返回数据一致以外，数据库内的数据是否也是准确？
    • 代码被破坏时，测试是否会正常的失败？
    • 如果代码发生变化，是否会出现虚假的正确（false positives）?
      • 例如为了让 UT 通过，设置一些特殊的数据，表面 UT 通过，实际有 bug
    • 每个测试是否都有简单有效的校验？
    • 测试是否合理的切分成不同的测试方法？
    • 测试也是代码，因此不要因为他们不是主要分支，就允许他们进行不必要的复杂 (complexity)

命名 Naming#

• 开发者是否每个命名都是好的？
• 好的命名需要
  • 包含足够的信息描述它是什么或做什么
  • 但又不至于过长导致难以阅读

代码注释 Comment#

• 原则
  • 简洁
  • 可理解
  • 必须
• 内容
  • 是代码所无法表达的内容
  • 应该出现的内容
    • 决定背后的原因
    • 为何这些代码会存在
    • todo 未来需要做的事，以 todo 开头进行 comment
  • 不应该出现的内容
    • 不应该解释代码在做什么
      • 代码功能解释应该由代码自己来表达
      • 如果代码不够简洁，需要文字描述来说明其功能，那么代码需要被简化
      • 例外：正则表达式或者复杂算法依赖详细的 comment 解释
    • 不应该解释代码应该怎么用，有什么表现
      • 这是文档该做的事情

风格 Style#

• 公司内部，每一种使用的主要语言都要有对应的强制统一的合理风格规范 (style guide，SG)
• SG 是绝对的权威。PR 应该符合 SG，SG 不应该适应 PR。
• 如果你想提出 SG 中不存在的代码风格改进，需要加上 “nit”，表名其不是必须的。不应该因为个人的代码风格偏好，拖慢 PR 通过的速度。
• 必须的 style 应该加入到 SG 中
• 如果进行大量的代码风格修改，必须单独提出 PR，不可以与其他功能代码混合。

一致性 Consistency#

• PR 是否与 SG 一致？
• 如果 SG 的内容只是推荐，而不是强制要求，那是否修改就依赖 Judgement call
  • 但还是优先偏向符合 SG，除非改动后会使代码可读性下降

文档 Document#

• 何时需要文档？
  • 当 PR 改变了代码的使用、测试、交互或发布流程
  • PR 删除或弃用 (deprecated) 代码，考虑删除相关文档
  • 如果发现文档缺失，请要求原作者添加文档！
• 需要修改的文档
  • README
  • 任何相关的文档

每一行 Every Line#

• 请阅读每一行你被分配到要 review 的代码
• 你可以扫读的内容
  • 数据文件
  • 生成的代码
  • 大型数据结构
• 不可扫读的内容
  • 人为书写的 类、方法、代码块
• 部分代码值得获得更多的关注
  • 这依赖 reviewer 的 judgement call
  • 对于不想给太多关注的代码，你也必须至少确定理解这些代码在做什么
• 如果你发现代码过长或过于复杂，妨碍了你的 review 速度和理解速度
  • 你应当立即告知 PR 作者
  • 等待 PR 作者简化或者申明清楚代码作用后，再继续进行 code review
• 如果你理解这些代码，但感到自己的能力不够评审其中部分的代码
  • 请确保邀请一个你认为能力足够的 reviewer，加入到 code review 中

上下文 Context#

• 添加的代码块往往只是部分逻辑，需要结合上下文的代码来阅读
• 如果上下文不多，可以在代码评审工具中展开查看
• 如果上下文过于复杂，则需要将代码下载到本地，使用 IDE 或其他工具查看

好实践 Good Things#

• 通常，review 留下的 comment 是提出批评改进意见的
• 但，当你发现代码中有很棒的东西时，请不要吝啬用 comment 发出你的赞美，表达你对 PR author 的感激，鼓励大家使用更好的实践。
  • 比如：“牛逼啊”
  • 比如：“这段代码写的很精妙，学习了”
  • 除了赞美词，要是有些言之有物的赞美就更加棒啦，可以和作者产生共鸣

如何看一个 PR#

总结 - TLDR#

• 符合我们的 PR 规范
• 这个 PR 变动是否合理，PR 不合理需要立即打回
• 检查 PR 的主要部分的设计，出现设计问题，需要立即反馈
• 以代码逻辑链的顺序看完余下部分

Step zero: 是否符合必要的规范#

• 是否关联的 Jira ticket
• commit message 是否符合规范
  • Jira-ticket(tag): description
  • tag:
    • feat 功能
    • fix 修复
    • ut 单元测试
    • mi 表改动
    • refactor 重构
    • inf 基建
• 是否 rebase 了目标分支

Step one: 对变动进行一个大致的了解#

• 查看 PR 描述，了解其大致在做什么
• 改动本身是否 make sense
• 如果感到不合理，需要立即反馈
  • 反馈时，先肯定对方的工作，再给出不接受的理由，并提出下一步该怎么做
• 出现持续的不合理 PR 时，需要反思一下开发流程（团队内部和延伸团队）
  • 使这些不合理的 PR，在开发前就被叫停

Step two: 检查 PR 的主要部分#

• 找到主要代码逻辑文件
• 如果找不到，就询问作者，或者要求作者进行 PR 拆分
• 先检查主要的设计问题，如果出现问题，必须立即反馈，并停止当前的 CR
  • 一般开发者提出 PR 后，会继续开发，如果基于这个 PR 开发，需要尽快叫停，及时止损
  • 设计问题需要大量的时间来修复，大部分功能都有一个 deadline，提早提出修改，有利于赶上 deadline

Step three: 以合理的顺序查看余下的 PR#

• 找出 PR 的逻辑链
• 按照逻辑链 Review
• 可以先读 UT，利于了解设计理念

快速的 CR#

总结 - TLRD#

• 即使很忙碌也要即使反馈
• 最长反馈时间在一个工作日内
• 如果你正在专注工作，请不要中断手上的工作
• 大型的 PR 应当被拆分成小的 PR，如无法拆分，也必须完成 step one 的 review
• 代码的复杂度导致 CR 速度下降时，需要立即反馈作者进行简化或给出说明

原因#

• 过慢的 CR 会降低团队效率，阻塞其他人的开发
• 过慢的 CR 可能导致开发者抵触 CR 流程
• Code base 的代码质量会受到影响
  • 一方面提升代码不能有效的合入
  • 另一方面，过慢的 CR 会降低开发者的开发质量

应当多快？#

• 除非你在专注的进行一项任务，否则接到 CR 请求时应当立即开始 review
• 最长反馈时间不可以超过 1 个工作日

速度 VS 打断#

• 保证不被打断比 CR 的速度重要
• 因为从打断恢复为专注状态需要大量的时间
• 应当选择一个专注中断的点，开始 CR

快速响应#

• 即使你很忙，也应当有个及时的响应
• 如果真的非常忙，可以先提供一些大致的意见，也就是上文的 step one

跨时区 CR#

• 保证在对方下班前完成 CR，不然 CR 流程就会因为时差大大的延长

LGTM with comments#

• LGTM 意味着可以合入
• 需要注意的是，LGTM 的本意是，这段代码符合 **“我们”的标准，而不是 **“我”
• 有的时候 LGTM 也有 comment，但不影响给 approve
  • reviewer 对开发者有信心，知道其后续会解决这个问题
  • 余下的 comment 不关键，不是必须要解决

大型 PRs#

• 要求 author 将 PR 拆分成小的 PR
• 如果无法拆分，那么尽量过一下整体设计，即 step one
• 保证降低代码质量的前提下，不阻塞开发这的下一步流程

Author 在 CR 中应当做什么#

关联一个 Jira ticket#

• PR 的内容要与 Jira ticket 一致
• 出现不同 task，需要新建 ticket 关联，单独提 PR

以 feature branch 提出#

• 不应当用自己远端中与主要分支同名的分支提出 CR
• 也不应用 alpha-only, free-only 这种非稳定分支提测
• 应当在自己的远端，基于目标分支为 code base，创建的 feature branch 进行开发和 CR

以 Code Review 的角度审视一遍自己的 PR#

• 详情参照第四节

完成冒烟用例自测#

• 提出 CR 请求前，需要完成自测。
• 测试大佬会提供冒烟用例 checklist，需要全部完成。

遵守 commit message 规范#

• 一个 PR，一个 ticket，一个 commit
• 关联 Jira ticket
• 记得移除 WIP 的 prefix
• 格式：见第五节的 step zero

保证 pipline 通过#

• code style
• test
• build

指定必须要 review 的 assigner#

• 对于新的功能逻辑，除了 author，还需要一位 backup 人员作为主要的 reviewer，这样同样的逻辑就不止一个人知晓
• 对于旧逻辑的改动，需要 assign 给原始代码的作者进行 review，因为对方更加了解这部分的业务逻辑和历史问题。原作者的信息可以通过 git blame 获知

跟进 CR 的进度，提供必要的帮助和反馈#

• 一旦 CR 提出，author 就有义务和责任推动 PR 快速被合入 code base
• 如果代码逻辑链路比较长，可以通过 PR 描述 或 直接给 reviewer 解释的方式，帮助 reviewer 快速理解主要逻辑链。但要注意，尽量减少对 reviewer 的打断，只有当 reviewer 表示开始 CR 才进行口头描述。
• 对于 Reviewer 提出的 comment，都需要以 comment 的形式回复，给出是否修改的回复。如果不准备修改，需要提供合理的理由。如果 reviewer 还是不予同意，则需要线下讨论或引入第二个 reviewer 进行仲裁。
• 如果 PR 在 CR 提出后一个工作日内还没有被 review，第二日晨会的时候需要提出。

PR 合入后，应尽快提测#

• PR 合入表示这段代码可以进入测试流程
• 如果是前后端联合开发，一般由前端提测。纯后端开发功能由后端提测。
• 但本着分步提测的原则，后端 PR 合入后也可单独提测，这点需要和测试提前沟通确认

Review 的开始#

原则：

• reviewer 的精神状态要完备
• reviewer 不处于心流的工作状态
• reviewer 和 author 可以高效地互动

结论：

• CR 需要有个固定的时间
• 推荐在晚饭后的一个小时

Review 的提出和进行流程 @Ehco zhou#