325 lines
15 KiB
Markdown
325 lines
15 KiB
Markdown
本文的主要读者是那些需要扮演 Code reviewer 的人，即需要 review 别人代码的人，以提供建议、指导和要求来如何开展 Code review 的工作。
|
||
|
||
最后一节是写给 code reporter，比如 PR/MR 的 owner。
|
||
|
||
下文中以 MR 指代提请 merge 的 code patch 请求。
|
||
|
||
---
|
||
|
||
## 1 Principle
|
||
|
||
- 目标：Code review 的主要目的是确保逐步改善代码库的整体质量和健康度。
|
||
- 负责制：Reviewer 应该对被 review 的 MR 负责。这不一定要规范到流程中，但 reviewer 自身应当有这个意识。
|
||
- 时效性：Reviewer 应该有责任保障 MR 在 review ready 后，到 MR merge 要足够快速，从而避免延期。
|
||
- 避免过分严格：Reviewer 应当倾向于批准 MR 通过，只要 MR 能够在某些方面改进代码库的质量，并且代码是有意义的。而不是一味追求代码完美。
|
||
- 评论：MR 合入前，应当确保已解决所有 review 阶段提出的 comments。Comment 可以拒绝（备注原因并达成一致），但不可以忽略。
|
||
- 指导性：Code review 对于交流代码实践、设计原则和加深项目理解有益。Comments 可以是指导性的（不要求修改），但请注明 Nit。
|
||
- 避免个人偏好：基于技术事实讨论，而不是基于主观感受。
|
||
- 尊重非标准风格：如果 MR 中存在 Coding Style 中没有约束的设计，可以参考代码库现有的类似风格。但不应当强加 reviewer 的主观建议。
|
||
- 避免冲突：developer 和 reviewer 有冲突时，建议另行讨论，但需要将达成一致的结果记录在 MR 中。可能要有项目 owner 的参与，但要避免 MR 延期。
|
||
- 用户：Code review 过程中提到的用户，既包括软件的最终用户，也包括其他在相同代码库中进行开发工作的 developer。
|
||
- 积极的反馈：Reviewer 应当积极响应 review 请求，并给予正向的反馈，以互相激励良好的 Code review 流程。
|
||
|
||
## 2 Review Point
|
||
|
||
### 2.1 设计
|
||
|
||
- 代码是否经过精心设计并适合软件系统？
|
||
- 集成方式是否正确？
|
||
- 现在合入这个功能是否是正确的时间？
|
||
|
||
### 2.2 功能
|
||
|
||
- 代码的行为是否和意图一致？
|
||
- 实现是否可以匹配需求？
|
||
- 接口变更（如库函数、外部类）通知到相关人员了吗？变更是否合理？
|
||
- 是否会引入随机问题？如并行导致的竞争，随机数测试等
|
||
- 对于无法理解的代码，可以要求 developer 澄清。对于 reviewer 来说，不能快速理解代码是正常的。澄清也会帮助所有 reviewer
|
||
|
||
### 2.3 复杂度
|
||
|
||
- 代码可以调整成更简单的实现吗？
|
||
- 其他开发人员能否轻松理解并使用这块代码？
|
||
- MR 是否可以拆分？不相关的功能避免在一个 MR 中
|
||
- 绝大多数情况，1 个 MR 只包括 1 个 commit
|
||
|
||
### 2.4 测试
|
||
|
||
- 代码是否带有正确、合理且有用的测试用例？
|
||
- 紧急情况可以省略测试用例，但之后应当提交专门的测试 MR 补充
|
||
- 代码仓已有的测试用例需要维护，修改测试用例应当有确切的原因
|
||
|
||
### 2.5 命名
|
||
|
||
- 代码中的变量、类、方法是否有符合项目风格的命名规范？
|
||
- 名称是否清晰有意义？
|
||
- 名称是否难以阅读，或和其他名词有歧义？
|
||
|
||
### 2.6 注释
|
||
|
||
- 是否没有必要注释，或是否滥用注释？
|
||
- 注释是否冗余？是否有错误？（注释需要维护）
|
||
- 注释不应该用于解释代码的逻辑（代码应该可以自解释）。有些例外，比如正则表达式和复杂算法
|
||
- 代码的背景、决策思路应当用注释说明
|
||
- 标记注释很有用，比如 TODO，BUG，FIXME，ISSUE，HACK，WARN，NOTE
|
||
- 对外的接口函数和接口类，库的头文件，应当有必要的注释说明用法和行为
|
||
|
||
### 2.7 风格
|
||
|
||
- 代码是否满足项目规定的代码风格？
|
||
- 功能性 MR 不要混合对不相关代码的风格变更。风格变更应另提 MR
|
||
|
||
### 2.8 文档
|
||
|
||
- 代码实现是否需要补充新的设计文档和测试文档？
|
||
- 是否需要修改现有文档？包括删除已经过时的错误文档
|
||
|
||
### 2.9 上下文
|
||
|
||
- 不要只关注修改的代码，也要关注修改代码的上下文，以提出改善性建议。
|
||
- 不要容忍微小的破坏系统健康度的 MR，任何系统都是由小的坏的变更而逐渐变差。
|
||
|
||
### 2.10 好的设计
|
||
|
||
- 除了查找 MR 中的问题以外，也要留意其中好的设计
|
||
- 提交 comments 反馈给 developer，鼓励良好的实践，也促进互相学习
|
||
|
||
## 3 Review Step
|
||
|
||
刚开始尝试 review 的 new reviewer，可以参考这个 review step。
|
||
|
||
### 3.1 全面了解
|
||
|
||
查看 MR commit message，了解 MR 大致要做什么。
|
||
|
||
- 指出不必要的 MR 变更（给予对 developer 充分的尊重）
|
||
- 指出 commit title 和 message 中的错误和建议
|
||
- 查看关联 JIRA，了解问题背景
|
||
- 阅读测试用例，了解 MR 影响了什么。以及检查测试相关的问题
|
||
|
||
### 3.2 主要检查
|
||
|
||
查看代码文件的主要内容。
|
||
|
||
- 如果不能快速定位代码的关键部分，请 comment 要求 developer 澄清
|
||
- 判断 MR 中是否有和当前 MR 无关的内容。比如风格修改，多余功能实现
|
||
- review 关键代码的设计
|
||
- 发现问题时请立即 comment，避免 developer 基于问题已经在做更多工作
|
||
|
||
### 3.3 次要检查
|
||
|
||
在主要内容没有意见之后，检查其他代码。
|
||
|
||
- 按照文件顺序重新浏览所有变更。
|
||
- 检查注释，命名规范等细节。
|
||
- 检查代码风格是否符合规定。
|
||
- 检查是否需要提供或变更文档。
|
||
|
||
## 4 Efficiency
|
||
|
||
Reviewer 应该尽快响应 MR 的 Code review 工作。
|
||
|
||
通常我们也希望 review 的过程足够快，也就是尽快 merge 代码，但这建立在很多因素的基础上。
|
||
|
||
### 4.1 review 低效的问题
|
||
|
||
- 降低团队效率。很多工作是基于当前 MR 展开的，MR 的 merge 过慢会阻塞其他开发工作
|
||
- 抱怨流程。review 流程会被抱怨，甚至对 reviewer 个人的抱怨，这会让团队成员的开发积极性受到挫败
|
||
- 代码健康度。developer 提交的其他 MR 可能会因此变得庞大、冗余，从而影响代码仓的质量
|
||
|
||
### 4.2 review 应该有多快
|
||
|
||
- Reviewer 的响应时间应该在 1 天内
|
||
- Reviewer 应该立即响应 developer 主动要求的 Code review，这是 Reviewer 的职责
|
||
- Reviewer 应该在每天都找一个时间，对仓库中所有 MR 进行 review
|
||
- 通常一个 MR 会经过多次 review，developer 也希望当天能得到多次 review
|
||
- 对于跨时区的 reviewer 和 developer，reviewer 应该在 developer 第二天上班前完成审查，并可以利用带评论的 LGTM
|
||
|
||
### 4.3 权衡
|
||
|
||
Review 请求会打断 Reviewer 自己的工作流，所以需要权衡。但通常还是需要积极响应主动请求。
|
||
|
||
- 选择在 Reviewer 自己的工作中断点 review 代码。比如构建代码时、午餐晚餐前后
|
||
- 如果没有时间，也应该对 developer 给予快速反馈，并建议其他 reviewer
|
||
- 可以提供一些初步 comments，并提出 need change
|
||
- 但不要为了快速 review，而妥协了代码质量和标准。记住，reviewer 要对自己的每一笔 review 负责
|
||
|
||
### 4.4 大型 MR
|
||
|
||
如果 MR 的代码量比较多。预估 review 的时间会超过 20 分钟。
|
||
|
||
- 尝试和 developer 沟通，是否可以精简 MR
|
||
- 如果无法精简 MR，尝试请 developer 提交 Code review 会议，集体做 review。developer 需要现场阐述 MR 的设计和细节
|
||
|
||
### 4.5 紧急 MR
|
||
|
||
紧急 MR 比如：
|
||
|
||
- 确定性的 commit revert
|
||
- 修复影响最终用户生产的错误
|
||
- 有 ETA 要求的功能，在即将到达 ETA 前
|
||
- 法律问题相关
|
||
- 明确的系统安全漏洞
|
||
|
||
看似紧急但不紧急的 MR 比如：
|
||
|
||
- 想赶在周末前合入，下周能开展新工作
|
||
- 跨时区的 review
|
||
- commit revert 之后，导致的新问题
|
||
|
||
紧急 MR 应该这么做：
|
||
|
||
- 优先关注 review 过程的速度
|
||
- 先确保正确性，其他 review 发现的问题，如格式、测试用例等，可以要求 developer 之后补充 MR 修改
|
||
- 紧急 MR 合入后，应当再次对该 MR 做细致的 review，并要求 developer 补充
|
||
- 让所有项目组成员了解紧急 MR 的合入事件，并明确可能带来的风险
|
||
|
||
尽量避免发生紧急 MR，这也有 PM 的责任，处理紧急 MR 应该极力避免项目积累技术债务。
|
||
|
||
## 5 Comments
|
||
|
||
提交 Code review comments 也需要 reviewer 注意。
|
||
|
||
### 5.1 保持友善
|
||
|
||
不必说，在工作沟通中，都应该尽可能注意自己的措辞会给对方带来的压力和抵触，尤其是在挑别人毛病的时候。
|
||
|
||
### 5.2 技术性解释
|
||
|
||
提供更多的技术性解释，来说明你的疑问或顾虑，比单纯的抛出问题更有利于 developer 处理。
|
||
|
||
### 5.3 指导性 comment
|
||
|
||
reviewer 虽然并没有义务帮助 developer 提供新的解决方案，但这并不意味着 reviewer 对提高 MR 的代码质量没有作用。
|
||
|
||
reviewer 可以适度的提供自己的建议方案，这通常有利于互相学习和加深对软件系统的理解。
|
||
|
||
毕竟，code review 的第一目标是保障代码质量，第二目标是提高软件开发人员的技能。
|
||
|
||
### 5.4 解释性请求
|
||
|
||
对于不能快速理解的 MR，可以请求 developer 阐述 MR 中的主要内容和关键代码。
|
||
|
||
同时，对于 developer 提出的问题，reviewer 也应该给予充分的解释。
|
||
|
||
通常来说，请求解释无可厚非，这并不代表着 reviewer 个人水平如何。只有 developer 自己对自己编写的代码最熟悉。更充分的解释也有助于其他 reviewer 的 review 工作。
|
||
|
||
### 5.5 同步线下的讨论
|
||
|
||
很多时候，在 review 过程中，可能 reviewer 和 developer 会线下（或者线上 1 by 1 会议）讨论 MR 的细节。这是没问题的。
|
||
|
||
但需要在讨论结束后，将讨论的主要结论同步在 MR 中，以方便其他 reviewer 阅读和了解 review 的过程。
|
||
|
||
当然，也有助于将来回溯 MR 合入过程中的细节。
|
||
|
||
### 5.6 Nit
|
||
|
||
Nit 是指 review 过程中，发现的一些小的，无关紧要的问题，reviewer 并不要求必须在当前 MR 做修改。
|
||
|
||
Nit 是重要的，鼓励 reviewer 和 developer 在 code reviewer 时多使用。
|
||
|
||
可能使用 Nit 的地方：
|
||
|
||
- 注释和变量中的小错误，比如语法错误和拼写错误。
|
||
- 不易理解的一些符号。
|
||
- 可以将代码提炼和改善的地方。
|
||
- 冗余的注释说明。
|
||
- reviewer 认可当前方案，但想分享他建议的更好方案。
|
||
|
||
## 6 冲突
|
||
|
||
Code review 的过程中，很容易对一些技术细节产生不同的观点。应当避免持续冲突影响了 MR 的合入效率。
|
||
|
||
- 讨论时，要始终注意自己的措辞是否会伤害到对方
|
||
- developer 通常对代码的细节，包括项目的细节，比 reviewer 了解更多和详细
|
||
- 讨论不应该过于频繁。应该留出独立思考的时间
|
||
- 如果代码存在严重问题，则 reviewer 不能妥协让步
|
||
- 无论什么时候，都应该对事不对人
|
||
- 避免妥协 “之后再处理” 的措辞。时间过的越久，“之后再处理” 就越不可能发生。（紧急 MR 例外）
|
||
- 如果 developer 一定坚持 “之后再处理”，那么要有 JIRA 跟踪，以避免后续遗忘。同时，代码中应当留下充分的解释，如 TODO 注释
|
||
|
||
## 7 MR 建议
|
||
|
||
本节内容是写给任何需要扮演 developer 角色的人员，提供一个开发代码和提交 MR 的实践指导。
|
||
|
||
### 7.1 重视 MR message
|
||
|
||
MR message 并不只是一个标题。不要省略详情内容，这是很不负责任的。
|
||
|
||
MR message 应当和 git commit message 保持一致，有利于回溯问题。
|
||
|
||
MR message 中应该包括典型的关键字，按照不同仓库的要求，这些关键字可能写在标题行，也可能写在详情文本中。这些关键字通常会作为将来检索相关内容的依据。
|
||
|
||
避免拼写错误和语法错误。
|
||
|
||
以下是一些具体说明：
|
||
|
||
标题行：
|
||
|
||
- 简洁地描述 MR 做了什么
|
||
- 注意格式要求，比如不要超出格式要求的宽度、首字母大写且行尾不加标点
|
||
- 标题行要添加必要的关键字（遵从仓库规则）。如 llvm 中会使用 [ARM]，[MIPS] 来标注 MR 的类型
|
||
- 尽量避免笼统地总结 MR。比如一个反例：”Fix a bug of mma“。应该让其他人通过标题了解到 MR 独特的地方
|
||
|
||
详情文本：
|
||
|
||
- 应该包括对问题的详细描述、为什么要做这个事情、为什么用这种方法
|
||
- 还要包括需求背景和 JIRA 链接
|
||
- 如果需要，还可以包括目前方法的缺陷分析、未来计划、基准测试状态以及设计文档位置
|
||
|
||
### 7.2 尽可能小的 MR
|
||
|
||
一个 MR 应该只解决一个事情。小的 MR 会有很多优点：
|
||
|
||
- Code review 更快。这体现在 reviewer 可以更快的理解代码和排查可能存在的问题。当然也更容易得到 approve
|
||
- 大的 MR 会审查不彻底（人的惰性使然），对 reviewer 的要求也更高。小 MR 更容易发现问题
|
||
- 被 Disapproved 之后，调整更方便
|
||
- 合入更方便，不容易和其他 MR 产生冲突
|
||
- 如果意外需要 revert，也会更方便，带来的影响会更小
|
||
- 编写小型 MR 也有助于 developer 梳理软件逻辑
|
||
|
||
多大代码量的 MR 算小型 MR 通常由代码仓的特征和所有 developer 和 reviewer 的大致判断决定，但通常几千行的 MR 就不能算小型 MR。（人脑的缓存通常也只能记忆这么多代码来完成 review）
|
||
|
||
### 7.3 可以接受的大 MR
|
||
|
||
有些情况下，大型 MR 也能接受，如：
|
||
|
||
- 增加或删除一个文件的完整代码
|
||
- 对某个相同特征的问题，在很多位置做修改
|
||
- 仓库初始配置
|
||
|
||
一个大型 MR 通常需要更多的时间和精力来 review，也会需要额外开展 Code review meeting 进行集体 review。
|
||
|
||
所有 reviewer 和与 MR 相关的 developer 都应当明确 MR merge 可能带来的风险和对项目的影响。
|
||
|
||
### 7.4 测试用例
|
||
|
||
大多数功能性的 MR 的合入都应该配有测试用例。避免后续补充测试用例的做法。
|
||
|
||
可以有单独的测试用例作为独立 MR 合入，但这些用例应该是对之前已存在代码的一个测试补充或对测试代码的整体重构。
|
||
|
||
以下情况可以接受没有测试用例：
|
||
|
||
- 紧急 MR 可以不添加测试用例，但紧急情况处理完毕后，应该额外提交测试用例 MR
|
||
- 格式调整、文档修改等 MR
|
||
- 可解释的其他不添加测试用例的情况，经 reviewer 接受
|
||
|
||
### 7.5 不能破坏构建
|
||
|
||
任何一个 MR 都应该是自完备的。它不能破坏代码仓的构建。
|
||
|
||
如果有几个相互依赖的 MR，也应该确保找到一种方法，让每个 MR 都能通过构建。功能可以不完备，但构建不能 fail。
|
||
|
||
另外，CI 的测试应该要保障不出现这种问题。
|
||
|
||
### 7.6 回复 Comments
|
||
|
||
永远记得 Code review 的目的是提高整个代码库的质量。当看到 comment 时，先控制自己反对的冲动（这种冲动其实是人之常情），想想为什么有这个 comment。
|
||
|
||
保持礼貌和感激。reviewer 也会做到这一点。
|
||
|
||
尝试澄清代码，以对 reviewer 有疑问的地方展开解释。并且也应该主动思考自己的代码为什么没有被其他人所轻易理解，并尝试改进。
|
||
|
||
Developer 也可以要求 reviewer 澄清他们的 comment。
|
||
|
||
可能会需要解决冲突，请参见上文 **冲突** 小节。 |