og

Code Review 衍生的所思所想

https://static.surmon.me/thumbnail/03-15-code-review.jpg

今天在公司内部分享了一篇关于 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 之类注释的也许可以避免重复维护,但是 exampledescription 这些都依旧很实用。

详细可以参考: 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:

        
  • 1
  • 2
  • 3
  • 4
  • 5
  • 6
  • 7
  • 8
  • 9
  • 10
  • 11
  • 12
  • 13
  • 14
  • 15
  • 16
  • 17
  • 18
  • 19
  • 20
### XXX 应用 **Feature** - 增加了 XXX 模块的 XXX 业务 - ... **Improve** - 优化了 XXX 功能 - ... **Bugfix** - 修复了 XXX 的问题 - ... **截图** ... - [ ] 我确认已在描述中补充了截图(如果有) - [ ] 我确认此 MR 已关联了相关的 Meego 链接 - [ ] 我确认自己的改动没有对全局产生预期外的副作用影响

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

完。

Article created at 2021/3/15 amin category code|3003 views.

Related tags:study,work,code

Article address:https://surmon.me/article/170

12comments
  • 学习者12
    学习者12Windows10Chrome89中国-北京市#1998

    请问您的云服务器是哪一款配置

  • Hooyim
    HooyimMac OS11.0.1Chrome89中国-杭州市#2003

    大佬牛逼

  • Observer
    ObserverWindows10Chrome89中国-北京市#2006

    请问您的云服务器是哪一款配置

  • Observer
    ObserverWindows10Chrome89中国-北京市#2007

    希望您有时间能回复一下

  • Surmon
    SurmonMac OS11.2.3Chrome89中国-北京市#2009
  • Thememockup
    ThememockupAndroid10Chrome89印度尼西亚-雅加达#2012

    Its any language available for this article?

  • Chetan Jain
    Chetan JainLinuxx86_64Chrome89印度-德里#2015

    Great Work

  • edgar
    EdgarMac OS10.14.6Chrome90中国-杭州市#2056

    静态文件是用的全站加速产品吗

  • FCUK
    FcukMac OS10.15.6Chrome89中国-上海市#2057

    把七牛的搬运到字节了?

  • Surmon
    SurmonMac OS10.15.7Chrome90中国-张家口市#2058

    reply:

    1. UGC 内容 OSS + CDN
    2. CODE 内容 Global CDN
  • Myc
    MycWindows10Edge91中国-北京市#2098

    拿来学习一下 !

  • lts
    LtsWindows10Chrome92中国-深圳市#2130
            
    • 1
    • 2
    • 3
    • 4
    • 5
    • 6
    • 7
    %0Anamespace%20Inv.Base.BLLFactory%0A%7B%0A%A0%A0%0A%7D%0A%0A
anonymous