今天在公司内部分享了一篇关于 Code Review 的杂文，也算是对最近思绪的一个整理。

注：本文可能只有部分观点是正确的。

正文：

  • 绿色：提倡的、正向的
  • 红色：不对的、反向的

Code Review 的目的

对编码质量把关

  • 完善：让 “原本运行可能有缺陷的一段逻辑” 变得 “减少缺陷”
    • 以不同的视角审视既有实现，往往能发现一些 “未做兜底的边界行为”。
    • 但，“完善的逻辑” 更多还是依靠编码者本身的技术素养，Review 未必能发现所有问题。
  • 增强：让某块逻辑 “变得更好”，寻求业务实现的更优解
    • 了解同一块业务的人，往往会有不同的实现思路。
    • 了解更多同样业务的人，往往会有更长远的组织方式。
  • 可维护：降低每一个节点的熵，避免面条代码，从而使整个系统 “长期可维护”
    • “能用就行” + “能用就行” + ... = “无法再维护”。
    • “无法再维护” 的系统可能并不是因为编码者水平不好，很有可能是因为 “每一次迭代的过程都比较粗糙地完成”。

参与者编码、表达能力的成长

  • 无论是被 Review 还是 Reviewer 都有机会以不同的视角优化自己的编码认知。
  • 比如 Review 多了之后最直观的成长就是：很容易看出哪些代码有实现缺陷。
  • 参与者多赢，在共同的努力下，长期正反馈，可以保持系统长期可持续维护。

参与者身体更棒了

  • 写代码时身心更愉悦，头发掉少了。
  • 沟通更顺利、工作也能更加高效。

Code Review 的特点

不容易量化收益

  • TS 文件中最多出现几次 any
  • 一个 MR 最多包含多少个 commit
  • 一次 Review 中最多出现多少个 comment
  • 一次 Review 中的 comment 是否必须全部 Resolve 才可以合并

上面这些数据无法用来衡量一个人水平行不行、问题多不多；也不应该用来限制 PR 是否可以合并；Code Review 往往不能很精细地表现出它带来的好处，但参与者明显能感受到的是：整个系统的缺陷会减少、会更明显、更有序可控，会更利于持续迭代。

需要安排时间

  • Review 是一个一定会占用时间、值得占用时间、也必须预留出时间来做的事情
  • 我们在排期时应该为 Review 预留出一定的时间（编码 + Review + 修正）

对码不对人

  • 不带情绪，始终围绕代码。
  • comment 表达的信息并不是 “指控” 和 “否认”，它包含 肯定、质疑、交流、记录 ...

但所有的 comment 都是针对编码本身，而不是个体；每一个 comment 都是 “让代码变得更好的契机”，所以我们应敞开胸怀拥抱它。

具体方式

  • 每个人都有一套自己的九阴真经，所以本文没有具体方法。
  • 但可以大体上从 “业务逻辑” 和 “组织形式” 两个维度去看代码，简单说包含：
    • 复用/抽象
    • 内聚
    • 解耦
    • 一致性
    • 优雅程度

高度可读

  • 产出的代码要求高度可读性。
  • 留下的 comment 也要求高度的可读性。

应假设，每一条留言、回复都是对所有参与者的，而不仅仅是那个提交者，所以应对自己产生的信息负责（完整、可读、有效）。

信息完整

  • 由于 comment 本身也有 “记录” 和 “交流” 的含义，所以即便进行了线下的沟通，也应该在线上（可以简略但要严谨完整）描述整个达成共识、解决问题的过程，便于其他人作为参考。
  • 正负面评价都应该有
    • Review 的一个重要目的是 “交流”，不仅仅是 “改进”；好的代码、思路分享出去也是一种成长
    • 所以遇到好的代码请随手 @majunchen 这块代码写的真棒！希望它下次出现在大赏的屏幕上
  • Approve 是结果（达成共识），不是目的
    • 所以 “求”，求的是 “请你花费宝贵的半小时后，让我的程序变得更稳健”，而不是 “求你给我点个 Approve，让我尽早合并，以完成工作任务”。

Code Review 应该避免的

  • 人肉语法检查器（浪费时间）
    • 不应该花过多精力关注语法之类的问题，机器能做事应该由机器完成
  • 完全不了解业务（流于形式）
    • 完全不了解业务的 Reviewer，难以做到深入理解，产生的作用有限
    • 所以往往 Review 需要建立在 Reviewer 了解自身业务的情况下
    • 要么提交者需要为 Reviewer 讲解自己的需求及实现，要么尽量寻找同业务线的人深度 Review
  • 不友善、不包含有效信息的交流（无效信息）
    • 不友善的交流 不如 直接当面交流

Code Review 提倡的

Code Review 是生产过程中的一个重要环节，合格的生产过程，更像是一种严谨完善的信息流走向。

  • 文件本身（表达出清晰的维护者、此文件如何工作、在系统中的职责）
  • 软件本身（表达出软件的使用姿势、迭代过程）
  • 提交过程（表达出谁在什么时间完成了什么事，有什么副作用、截图...）
  • 交流过程（表达出你的这行代码为什么（可以）更好，原因是什么）
  • 合并过程（只记录重要的生产信息，而不是一股脑的合并）

理想的 Code Review 流程

1. 按照业务（或其他单位）分批 commit

  • 如果是一个很大的 MR，应该在第一阶段提出，并以 WIP 开头命名 Title；比如一个项目的 Init，其实就是一个节点。
  • 如果是 chore 之类的变动，尽量单独一个 MR，与业务隔离。
  • 按照每次修改的同类型业务分作多批次的 commit，Reviewer 可以通过 GitLab/GitHub 等工具提供的 Filter 能力进行相对精确的 diif。

2. 在 Create MR/PR 界面自己 Review 一遍自己的代码

  • 刚刚完成的代码，自己的第一遍 Review 大概是能发现问题的（我的经验）。
  • 向 Reviewer 交付自己审阅过的代码，也是 对他人的时间负责。

3. 不同深度的 Review 交流

  • 应该保证最少有一个 Reviewer 是了解自己的业务的，从业务的维度去看待实现。
  • 其他 Reviewer 可以适当从组织、编码细节的角度去看待实现。

4. 正确的交流姿势

  1. comment 是面向所有参与者的，所以即便进行了线下的沟通，也应该在线上（可以简略但要严谨完整）描述整个共识或解决方案的过程。
  2. Resolve 是 Reviewer 对被 Review 的人做出的改变表示认可而采取的主动行为；我们无法假设一次改动一定是符合预期的，所以，Resolve 应该是提出问题的人才有的权利和能力。
    • Resolve 代表的是共识，是 “你的这次修改看起来没有问题，我折叠了”，而不是 “我改了，你看不看认不认可不重要，反正我先点关掉了”。
    • 提前 Resolve 这种行为会产生严重的信息干扰，如果一个 MR/PR 有几十上百个 comment，这会干扰 Reviwer 的进度和预期；Resolve 点击后（或因为提交而自动折叠后）原内容部分就被折叠了，但在这个时候问题的参与者/原始发起者很可能都还没看到这次的变动是否符合预期，这时 TA 只能把每一个 comment 再展开来一一核对，所以 Resolve 这个行为需要表达“最终共识”，而不是“我改了”；“我改了”这个信息是通过 Outdated 表达的。
  3. 没有 Resolve 不代表这个 comment 是有问题的
    • 未 Resolve 的 comment 可能是为了 MARK，不被折叠，确保信息可以直接展示。
    • 也可能是为了标记 TODO，是 “这次来不及了，下次再搞，我先记下，不要折叠”。
    • 所以 “是否有 comment 未 Resolve” 也不应该与 Merge 权限挂钩。
  4. Approve 也是两者达成共识的一种表现
    • Approve 的确认人和提交者对 Approve 行为本身共同担有责任
    • 大部分人也不应该拥有直接 Merge 权限
  5. MR/PR 本是一个有非常有价值的载体
    • MR/PR 是可能会被反复翻看的一种 “成长日记”、“产出”；其记录的是一种思想的碰撞，经常新的问题会在旧的 MR/PR 中找到答案、灵感。
    • 和 “字节的文档很多，但能看的很少” 一样，如果把文档当做一次性的去维护，最终真实的阅读价值会无限降低，MR/PR 也是一样的。
      • 流水账，不注重高效严谨的表达
      • 只顾眼下，不会因此而去关注类似的、衍生的场景、问题

5. 正确的合并姿势

所有人在合并的时候都使用 Rebase + Squash（合并提交）

  • Rebase 代替 Merge 可以使整个 Soure Tree 层级、分支清晰。
  • Squash 使整个 Source Tree/Line 的信息 “简明扼要”。

比如 monorepo 下有很多个子应用，如果有一个 MR/PR 包含了 5 条 commit，每一条的 commit message 都是类似 feat: XXX page done 那么不使用 Squash， 就会导致 Master 分支的 Line 中出现的这 5 条信息，这会造成：

  1. 信息冗余，在 monorepo 层面更多需要知道的可能是：“哪个子应用做了一件什么事”，而不是 “子应用是怎么做的”。
  2. 可能无法表达所在应用、模块信息，commit 信息可能并不是规范的
  3. 同时，Squash 要基于一个事实才有意义：正确完整地使用 MR Title 表达此次的修改信息；如果这次的 MR/PR 的 Title 本身就是 feat: XXX page done 那 Squash 也没啥用，因为合并后，从 Master 看依然不知道你这次提交到底是在哪个应用下做的改动。

Code Review 的简单总结

  1. Code Review 的本质意义是：提前人为检阅代码以减少软件缺陷，其次是讨论、记录、分享。
  2. 至少需要一个最了解目标业务的人做 Reviewer，不然就会流于形式。
  3. Code Review 的过程就是一个软件的迭代、成长、修正的过程，comment 记录的是 CHANGLOG 里没有的“思考过程”；comment 不是一次性的流水账，是有回溯意义的，他们是有被不断 “翻看”、“参考” 的需求，comment === 探讨/解决 !== 有个必须解决的问题没解决。
  4. Code Review 是一个一定会占用时间、值得占用时间、也必须预留出时间来做的事情。
  5. 拒绝 Review 的理由有千千万，迭代快是最常见的一个，但是后果是：重构时、拆分时、抽象时、换人接手时的举步维艰，为此买单的是我们每一个参与者。
  6. 如果有能力追求极致，Review 本身应该实施成 “编码中的行为艺术”，使每个参与它的人都能快速、清晰、地明白当时讨论的上下文，并得到有价值的信息，及有价值的思考。
  7. 靠自觉意义不大，只有 “程序正义” 才能相对保证质量，所以规范极其重要。

一些简易规范

该提交的生产资料

  • 机器生产的代码，不应该被看作 “受版本托管的生产资料”，所以不应该有 “经常提交 .dts” 这种事情存在。
  • 比如一些需要用工具生成的类型定义，是不应该一次次覆盖生成再提交的。

CHANGELOG

一个冰箱、洗衣机都会有说明书，说明书就是产品的一部分，一个软件也是。

CHANGELOG 和 README 就是一个软件的说明书，它们分别记录了一个软件的打开、使用方式，以及其至今为止的生命周期中经历过什么。

软件应该和冰箱洗衣机一样，不作 “下一个维护、消费代码的人一定有充足经验” 的假设。

CHANGELOG 在 GitHub 中有 Release Note 作为代替，但类似 Release Note 的记录形式并不随软件本身迁移，就像是你造的冰箱的使用说明书是电子版的，一个人必须要有 Kindle 才能看，这并不合理。

所以我认为：一个完整的软件应包含 “跟随软件本身的” CHANGELOG 和 README。

git 与 JSDoc

之前听大家说可以通过 VSCode 的插件精准地看到某个文件的某一行的最后修改的信息，所以 File header 并不必须，但我认为很有必要。

git 记录的信息是：

  • {谁} 在 {什么时候} {因为什么原因} {做了什么事} {会产生怎样的结果/副作用}
  • {commiter} {time} {commit message & description}

但 git 并不会记录：

  • 这个文件的 owner 是谁，我有问题咨询如何第一时间找到最了解它的人。
  • 这个文件在这个文件夹中承担的什么作用，它是一个公共模块吗？被多少文件潜在消费？

File header 其实是 git 信息的一种互补，用于告诉别人，即便这个文件被很多人改了很多次，但你是最了解这个文件的人，下次有其他人在这里发现了问题可以第一时间找到你（或者你作为中转），而不是根据不那么精准的 git 提交记录信息（并且是依赖工具的）去挨个问。

如果说 git 是一种 “记录”，JSDoc 则是对文件本身的 “解释”。

所以 File header 和 JSDoc 都是有用的；

  • JSDoc 一个文件的说明书
  • README.md 一个软件、模块的说明书
  • CHANELOG 一个软件的的更新记录表

它们都是一个软件本身的一部分，所以有很强的存在价值。

JSDoc 以前很重要的一些参数都被 TS 的类型代替了，所以 params return 之类注释的也许可以避免重复维护，但是 example、description 这些都依旧很实用。

详细可以参考： TypeScript + JSDoc = better-docs

TS 的 any 类型

另外，在任何的 TS 项目中可能都需要一个基本类型叫 declare type TODO = any 而不是无区别的 any 泛滥。

注意：这并不是某种社区标准，只是在实践过程中总结出的 “有用” 的一种 “技巧”。 相关讨论

绝大部分时候我们的 TODO 变成 any 后就真的一直 any 了，以至于到了后来我们已经分不清哪些 any是需要去完善的，哪些 any 是真的只能 any 的。

注释信息 TODO: 和 TS 类型别名 declare type TODO = any 的区别：

  • TODO:是一个可以用于标记 TODO 信息的注释标识，可以通过 IDE 搜索，或一些辅助插件统一地统计、检索整个系统中的 TODO 注释。
  • declare type TODO = any 是一个 TS 中的类型别名， 是 “需要完善但目前没有条件完善的类型”，而原始 any 则很可能是 “本身就不需要消费/关心的类型”，这个类型别名的意义就在于区分代码系统中的哪些 any 真的就本该是 any，哪些 any 是当下偷懒/没条件需要今后补齐的 any。

统计方式之别：

  • 通过 VSCode 搜索 TODO: 得到的就是这个项目中真正的 TODO 注释的数量
  • 通过 TS 查看 TODO 这个类型的引用方，来统计有多少个类型是 TODO （需补齐）状态

编码注释

严谨的注释格式可以高效地对系统信息进行分级。

注释统一和分级：「怎么注释」 不如 「大家都用同一种格式来注释」 更重要。

建议格式：{全大写前缀}{半角英文冒号}{空格}{内容}

  • TODO: 需要 XXX @somebody
    • 后面加自己的名字是因为多人维护的系统，每个人都会留 TODO，但多了都不知道哪个是需要你去 do 的事
  • MARK: 此处可能需要优化
    • 一种很低级别的备注信息
  • 其他，可补充...

comment 分级

不必要太复杂，有简单易理解的标识就足够

  • MARK: XXX
    • 一些备注；一般是一些不需要 “解决” 但需要 “关注” 的备注信息，比如此处要 at 一个其他人，给自己、他人做标记之类的
    • e.g. MARK: 下次这里也这么做 @surmon
  • NIP: XXX
    • 不重要；改了更好，不改也没关系（来自 Google not important）
    • e.g. NIP: arr?.length 比 arr && arr.length 更简洁
  • XXX
    • 直叙观点，一般是 “期望改动” 或 有争议的地方
    • e.g. 为什么把枚举改为数字了？有何隐情？

预置 MR 模板

通过预置 MR 模板，可以约束提交者保证自己合进 Master 信息是规范的。

e.g.

Title：Feature: <XXX_App> <something>

Description：

        
        

          
1234567891011121314151617181920

          
### XXX 应用

**Feature**
- 增加了 XXX 模块的 XXX 业务
- ...

**Improve**
- 优化了 XXX 功能
- ...

**Bugfix**
- 修复了 XXX 的问题
- ...

**截图**
...

- [ ] 我确认已在描述中补充了截图（如果有）
- [ ] 我确认此 MR 已关联了相关的 Meego 链接
- [ ] 我确认自己的改动没有对全局产生预期外的副作用影响

附曾经参与的有参考意义的 PR 链接 。

完。