今天在公司内部分享了一篇关于 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. 正确的交流姿势
- comment 是面向所有参与者的,所以即便进行了线下的沟通,也应该在线上(可以简略但要严谨完整)描述整个共识或解决方案的过程。
- Resolve 是 Reviewer 对被 Review 的人做出的改变表示认可而采取的主动行为;我们无法假设一次改动一定是符合预期的,所以,Resolve 应该是提出问题的人才有的权利和能力。
- Resolve 代表的是共识,是 “你的这次修改看起来没有问题,我折叠了”,而不是 “我改了,你看不看认不认可不重要,反正我先点关掉了”。
- 提前 Resolve 这种行为会产生严重的信息干扰,如果一个 MR/PR 有几十上百个 comment,这会干扰 Reviwer 的进度和预期;Resolve 点击后(或因为提交而自动折叠后)原内容部分就被折叠了,但在这个时候问题的参与者/原始发起者很可能都还没看到这次的变动是否符合预期,这时 TA 只能把每一个 comment 再展开来一一核对,所以 Resolve 这个行为需要表达“最终共识”,而不是“我改了”;“我改了”这个信息是通过 Outdated 表达的。
- 没有 Resolve 不代表这个 comment 是有问题的
- 未 Resolve 的 comment 可能是为了 MARK,不被折叠,确保信息可以直接展示。
- 也可能是为了标记 TODO,是 “这次来不及了,下次再搞,我先记下,不要折叠”。
- 所以 “是否有 comment 未 Resolve” 也不应该与 Merge 权限挂钩。
- Approve 也是两者达成共识的一种表现
- Approve 的确认人和提交者对 Approve 行为本身共同担有责任
- 大部分人也不应该拥有直接 Merge 权限
- 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 条信息,这会造成:
- 信息冗余,在 monorepo 层面更多需要知道的可能是:“哪个子应用做了一件什么事”,而不是 “子应用是怎么做的”。
- 可能无法表达所在应用、模块信息,commit 信息可能并不是规范的
- 同时,Squash 要基于一个事实才有意义:正确完整地使用 MR Title 表达此次的修改信息;如果这次的 MR/PR 的 Title 本身就是
feat: XXX page done
那 Squash 也没啥用,因为合并后,从 Master 看依然不知道你这次提交到底是在哪个应用下做的改动。
#Code Review 的简单总结
- Code Review 的本质意义是:提前人为检阅代码以减少软件缺陷,其次是讨论、记录、分享。
- 至少需要一个最了解目标业务的人做 Reviewer,不然就会流于形式。
- Code Review 的过程就是一个软件的迭代、成长、修正的过程,comment 记录的是 CHANGLOG 里没有的“思考过程”;comment 不是一次性的流水账,是有回溯意义的,他们是有被不断 “翻看”、“参考” 的需求,comment === 探讨/解决 !== 有个必须解决的问题没解决。
- Code Review 是一个一定会占用时间、值得占用时间、也必须预留出时间来做的事情。
- 拒绝 Review 的理由有千千万,迭代快是最常见的一个,但是后果是:重构时、拆分时、抽象时、换人接手时的举步维艰,为此买单的是我们每一个参与者。
- 如果有能力追求极致,Review 本身应该实施成 “编码中的行为艺术”,使每个参与它的人都能快速、清晰、地明白当时讨论的上下文,并得到有价值的信息,及有价值的思考。
- 靠自觉意义不大,只有 “程序正义” 才能相对保证质量,所以规范极其重要。
#一些简易规范
#该提交的生产资料
- 机器生产的代码,不应该被看作 “受版本托管的生产资料”,所以不应该有 “经常提交
.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:
- 1
- 2
- 3
- 4
- 5
- 6
- 7
- 8
- 9
- 10
- 11
- 12
- 13
- 14
- 15
- 16
- 17
- 18
- 19
- 20
- 21
### XXX 应用
**Feature**
- 增加了 XXX 模块的 XXX 业务
- ...
**Improve**
- 优化了 XXX 功能
- ...
**Bugfix**
- 修复了 XXX 的问题
- ...
**截图**
...
- [ ] 我确认已在描述中补充了截图(如果有)
- [ ] 我确认此 MR 已关联了相关的 Meego 链接
- [ ] 我确认自己的改动没有对全局产生预期外的副作用影响
附曾经参与的有参考意义的 PR 链接 。
完。
namespace Inv.Base.BLLFactory { }
拿来学习一下 !
把七牛的搬运到字节了?
静态文件是用的全站加速产品吗
回复:
Great Work
Its any language available for this article?
希望您有时间能回复一下
请问您的云服务器是哪一款配置
回复:
在这个页面- t6 (无性能约束实例)
大佬牛逼
请问您的云服务器是哪一款配置