如何完成一次高质量的Code Review?

(2020版)

 

Emac

2020/8

 

项目“死亡”三角

早餐店的故事

我们应该谨守“及早发现、及早解决”的准则,如此我们便能在生产流程中价值最低的阶段修正问题。因此,我们应该在蛋商送蛋之前让他们把坏的蛋挑出来,而不是让我们的顾客在餐桌上发现它们。

- 安迪·格鲁夫

CR 水平自测

CR 水平自测

  1. 搞形式主义,存粹是浪费时间

CR 水平自测

  1. 搞形式主义,存粹是浪费时间
  2. CR 是保证程序正确性的手段

CR 水平自测

  1. 搞形式主义,存粹是浪费时间
  2. CR 是保证程序正确性的手段
  3. CR 是保证代码规范的手段

CR 水平自测

  1. 搞形式主义,存粹是浪费时间
  2. CR 是保证程序正确性的手段
  3. CR 是保证代码规范的手段
  4. CR 是 Leader 的事,跟我没关系

CR 水平自测

  1. 搞形式主义,存粹是浪费时间
  2. CR 是保证程序正确性的手段
  3. CR 是保证代码规范的手段
  4. CR 是 Leader 的事,跟我没关系
  5. 我只看指给我的 CR,其他 CR 跟我没关系

CR 水平自测

  1. 搞形式主义,存粹是浪费时间
  2. CR 是保证程序正确性的手段
  3. CR 是保证代码规范的手段
  4. CR 是 Leader 的事,跟我没关系
  5. 我只看指给我的 CR,其他 CR 跟我没关系
  6. 没有时间 CR,直接 Merge

CR 水平自测

  1. 搞形式主义,存粹是浪费时间
  2. CR 是保证程序正确性的手段
  3. CR 是保证代码规范的手段
  4. CR 是 Leader 的事,跟我没关系
  5. 我只看指给我的 CR,其他 CR 跟我没关系
  6. 没有时间 CR,直接 Merge
  7. CR 必须一行不落从头看到尾

CR 水平自测

  1. 搞形式主义,存粹是浪费时间
  2. CR 是保证程序正确性的手段
  3. CR 是保证代码规范的手段
  4. CR 是 Leader 的事,跟我没关系
  5. 我只看指给我的 CR,其他 CR 跟我没关系
  6. 没有时间 CR,直接 Merge
  7. CR 必须一行不落从头看到尾
  8. CR 必须一次完成

CR 水平自测

  1. 搞形式主义,存粹是浪费时间
  2. CR 是保证程序正确性的手段
  3. CR 是保证代码规范的手段
  4. CR 是 Leader 的事,跟我没关系
  5. 我只看指给我的 CR,其他 CR 跟我没关系
  6. 没有时间 CR,直接 Merge
  7. CR 必须一行不落从头看到尾
  8. CR 必须一次完成

我个人认为代码有这几种级别:1)可编译,2)可运行,3)可测试,4)可读,5)可维护,6)可重用。通过自动化测试的代码只能达到第3)级,而通过Code Review的代码少会在第4)级甚至更高。

- 陈皓

Subtitle

CR 的重境界

认识CR:第 1 重境界

  • 关注代码规范
  • 避免重复代码
  • 降低(圈)复杂度
  • 关注性能问题
  • 关注分布式事务
  • 关注架构设计

认识CR:第 2 重境界

  • 提升自我觉察能力
  • 建立良好开发节奏
  • 高频次的团队活动

认识CR:第 3 重境界

  • 传帮带文化的重要组成
  • 工程师文化的日常体现

CR “精彩”问题集锦

空白字符使用

命名不当

注解使用不当

位置不当

性能问题

架构规范

架构设计

整体Review

CR 最佳实践

  • 依据个人偏好每天固定几个时间段专门用于 CR,我的习惯是出门前和下班前。CR 耗费的脑力丝毫不亚于编码,甚至更高,CR 过程中需要高度集中注意力。清醒的头脑和无干扰的环境,是提出高质量的评审意见的秘诀。
  • 除了固定时间段,任务切换期间也是 CR 的好时机。
  • 每次 CR 尽量控制在 15~30 分钟以内,超过 30 分钟应休息一会。
  • 收到 MR 之后,先判断一下 MR 的性质,如果是 Bug Fix 类型的 MR,应尽快评审,如果是新功能MR,则可以等待下一个 CR 窗口。
  • 从收到 MR 到 CR 结束,最长不要超过 1 个工作日
  • 开始 CR 之前先要搞清楚 MR 要解决的问题背景。
  • CR 就像读书,先看目录(改动的文件列表),再精读重点章节(包含核心业务逻辑的代码),最后扫读剩余章节。
  • 如果改动的文件数量较多,可以打开 IDE,切换到源分支,方便在 CR 过程中随时打开相关代码进行阅读。
  • 评审核心代码时,如果发现严重问题,应立刻终止 CR,找 MR 提交者当面讨论。
  • 如果 MR 提交者对评审意见提出异议,评审者应找提交者当面讨论,避免在评论区互踢皮球
  • 合并代码之前应确保所有评审意见都被妥善处理。
  • 记得点赞。CR 不是只能提意见,看到优雅的代码不要吝啬你的表扬。

CR 不是提意见,而是探讨

不是任务,而是基本能力

Made with Slides.com