怎样做一个合格的 CodeReview

引子

CodeReview 是一件很需要耐心和技术功底的活动。

  • 不了解业务,很容易沦为代码风格检查;

  • 代码量比较大,很容易走马观花,放过业务细节上的纰漏;

  • 技术功底不够,很容易放过性能或安全上有漏洞的代码;

  • 代码细节不严格追究,似乎达不到 CR 的力度;严格追究,来回沟通好几趟。

  • 需要设定标准和底线。底线达不到,坚决不通过,不因任何问题而妥协。

最难的是,CodeReview 是一项需要全局观的活动。哪怕仅有一行代码改动,也需要在更大的业务语境里去审查其是否恰当;如果有多行改动,恐怕要结合该业务的所有代码去分析。因为,改动的代码有可能单看上去没有问题,但结合已有代码及逻辑,就可能潜藏错误或漏洞。

BUG 无处不在。以人类现有的理解力和审查力,基本不可能避免大型软件里的 BUG 的产生(你无法看到看不到的 BUG)。能够做到的是: 限制 BUG 产生的影响使之微乎其微。

必要前提

  • 阅读 CodeReview 相关的技术文档及说明,熟悉相关业务逻辑及改动意图;

  • 代码提交者保证遵循必要的代码规范和风格;有极少量违反可以提醒,大量违反直接打回;

  • 代码提交者保证提交之前编译通过,单测全部通过;

  • 代码提交者保证功能的自测通过;

  • 代码提交者自己先行 review 一遍,纠正低级错误;

  • CodeReview 重点检查必要规范、关键逻辑、质量层面。

两个敏感素质

无论技术水平如何,业务熟悉度如何,CodeReview 的人应当具备两个基本敏感素质:

  • 影响面评估敏感。 对代码涉及的影响面敏感,即使只有一行代码;

  • 对代码潜在问题敏感。看到一行代码,能敏锐意识到可能存在的问题:NPE、健壮性、性能、可维护性、安全等。

常用检查项

规范与风格

  • 命名内容: 是否清晰直观表达意图;是否有粗略不贴切的问题;是否与项目约定一致;

  • 命名风格: 变量/方法名 - 驼峰式;常量 - 大写+下划线;(新手容易犯)

  • 注释: DO 属性是否有加业务含义注释;(建立团队约定)

  • 魔数: 使用常量定义;(情不自禁都会犯,要坚持零容忍)

  • 拼写错误: 要杜绝拼写错误,尤其是两个字符对调的情形(零容忍);


逻辑层面

  • 参数校验: 参数格式、参数内容、参数合法性校验(必须);

  • 异常:异常是否被捕获并适当处理;(必须)

  • 关键代码: 要细读,反复推敲;(必须)

  • 模板代码: 是否有拼写错误;(必须)

  • 算法:是否有详细说明、引用出处;(推荐)

  • 流程:复杂流程是否有相应文档说明;(推荐)

  • 注释:不明显的逻辑、workaround 方案、临时方案、很难用简单单词表达的复杂业务含义等要加注释说明(建议);

表达层面

  • 拥挤的代码:多行挤在一行最好能分拆为多行;(推荐)

  • 嵌套 if-else: 是否能够解开打平;是否可使用策略模式来分离;(推荐)

  • 费解的代码:理解起来有点绕弯,不够直观清晰地表达意图;(建议)

  • 建议的方式:可以给出更好的代码实现建议;(建议)

质量层面

  • NPE: NPE 无处不在,要严加防范;API 调用、数据库查询、反射获取的对象、多级嵌套对象等返回的对象务必要做判空检查(必须);

  • 健壮性: 主要检查越界、字符串解析异常;split, indexOf 等方法要注意越界异常;Json 解析要注意非法格式及非法数据校验(必须);

  • 重复代码: 重复代码,是否可以抽离子函数或模板处理(重复代码敏感);

  • 可维护: 散落在流程里的领域对象行为;过长的参数;过长的方法;方法内部修改入参等行为;

  • 统一性: 同一个概念,是否创建了多个不同类来重复表达;

  • 单测:是否有必要的单测;单测是否覆盖主要逻辑;

  • 性能:是否有循环调用 API 或访问数据库的做法; 是否面临大数据量场景(是否有加索引措施、SQL 编写是否适当等);是否需要加缓存(本地还是分布式);

  • 并发: 是否有多线程访问的可能性;是否有不受控的线程或线程池创建;是否有加同步访问;同步手段是否恰当;并发性能是否能够接;是否存在死锁风险;

  • 安全: 越权访问;敏感信息明文;SQL 注入; 动态代码执行是否有恶意参数校验;

  • 日志: 是否有必要的日志信息(info, warn, error);是否简洁得当;异常是否捕获并仔细打印了业务ID 相关的日志;


方法

  • 可以先初步过一遍,检查下是否明显有以上问题,及时反馈;

  • 进一步再深入业务核心,去审查业务逻辑是否有问题;

  • 将模板代码与关键代码区分开;

  • 关键代码要仔细研读;

  • 可以在开发的停顿间隔之间进行;

  • 建议至少在 IDE 里审查一下改动,在 IDE 里看更有感觉,在页面上看由于缺失上下文语境,容易遗漏;

  • 把程序运行起来,亲自试一试,或许你会有一些和他们测试时不同的操作,发现一些他们遗漏的问题。

建议

  • 对于一些比较紧急的改动会留下改进建议,但可以通过,通过后续代码提交解决遗留的问题;

  • 可以对Review的评论进行分级,不同级别的结果可以打上不同的Tag,比如[required]必须修改,[recommend] 推荐用法,[suggestion] 可选建议,[question]有疑问需要澄清;

  • 可以考虑团队统一安装某个代码检查插件,提交之前先解决插件指出的问题,减少 CR 成本;

  • 比较大的改动,可以分批提交,比如分成多个分支;

  • 评论宜正面积极,给予建议而不是指责;

  • 对于新手,更多是给予代码规范上的建议;对于高级工程师,更多给出业务和质量层面的提醒;

  • 不要吝啬你的赞扬;好的代码也有必要拿出来让大家一起学习。

参考文章

posted @ 2021-06-06 22:23  琴水玉  阅读(104)  评论(0编辑  收藏  举报