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