拾贝

代码评审赋魅

    arch     原创·Code Review

先来看一个令无数技术Leader闻风丧胆的项目“死亡”三角,业务压力引发代码质量下降,代码质量下降引发开发效率下降,开发效率下降又加重了业务压力,最终导致业务压力山大,乃至项目烂尾。如何破解?方法有很多,像精简业务需求、增加开发人手、升级技术架构等,很多时候需要多管齐下,但凡打掉这个“死亡”三角中的任何一角,就能终止这个恶性循环,甚至逆转为良性循环。

代码评审(Code Revew,简称CR)的首要打击目标显然是“烂代码”。避免“烂代码”的最好时机是写代码的时候,其次是代码评审的时候。IBM 的 Orbit 项目有 50 万行代码,使用了 11 级的代码检查(其中包含代码评审),结果是项目提前交付,并且只有通常预期错误的 1%。一份对 AT&T 的一个 200 多人组织的研究报告显示,在引入代码评审后,生产率提高了 14%,缺陷减少了 90%。那到底什么是代码评审?如何进行代码评审?继续往下看。

1 CR 祛魅

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

—— 陈皓

下面 8 条有关 CR 的阐述,你觉得哪些是正确的?

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

———————————————————————————————— 请仔细思考 60 秒

3…2…1…时间到,你的答案是几条?很抱歉,在我看来,没有一条是正确的。1、4、5、6 是送分题,显然都是错误的。7 是眉毛胡子一把抓,CR 就像读书,不是所有的书都适合精度,也不是所有的代码都需要评审。8 是任务心态,为了 CR 而 CR,CR 的目的不是完成 CR,而在于提升代码质量,你写代码时也不会一次完成所有功能。比较有争议的是 2 和 3,诚然,正确性和代码规范都是 CR 要关注的方面,但这并不意味着 CR 要保证正确性和代码规范(CR 也没法保证),保证正确性的主要手段是测试(单元测试,集成测试,契约测试,功能测试,自动化测试等),而保证代码规范主要依靠代码规范检查工具(像常用的 checkstyle 和 PMD)。

CR 到底是什么?依我所见,CR 本质上是一种讨论,一种严肃的、专业的、异步的以文字形式呈现的讨论,随意性和情绪化是 CR 的大忌。什么叫随意性?未经审视的评论。什么叫情绪化?因时而异,因人而异。高水平的 CR 首先要忘掉自己。

2 知:CR 的三重境界

技术水平决定了 CR 的下限,认知高度决定了 CR 的上限。所以说 CR 水平高不高,最终还是看认知水平。认识 CR 有三重境界,分别是执行层、团队层和文化层。

2.1 执行层:昨夜西风凋碧树,独上高楼,望尽天涯路

第一层为执行层,顾名思义就是通过如何做来认识 CR。以下列举 CR 时需重点关注的六个方面,并辅以相应的例子便于理解。

1)关注代码规范。命名是第一位的,一个令人费解的命名背后往往隐藏着一个设计纰漏。其他诸如空白字符、换行、注释等问题,也会影响代码的可读性和可理解性。

2)避免重复代码。编程法则第一条,Don’t repeat yourself. 重复代码是万恶之首,重复代码人人得而诛之!

3)降低圈复杂度。什么是圈复杂度?简单来说就是代码中 if/case/for/while 出现的次数。圈复杂度越高,BUG 率越高。如果一个方法的圈复杂度达到 3 或者更高,那么 CR 时就要多看两眼。

4)关注性能问题。性能问题虽然不常见,可一旦暴雷往往就是大问题。CR 时看到循环,记得多留一个心眼。

5)关注分布式事务。涉及远程服务调用,或者跨库更新的场景,都应考虑是否存在分布式事务问题,以及适用何种处理方式,是依赖框架保证强一致性,还是记录异常数据保证最终一致性,抑或是直接忽略?

6)关注架构设计。代码有代码规范,架构有架构规范。面对一个新功能的 MR(Merge Request),除了检查架构规范,还应推敲其架构设计,比如是否符合整洁架构三原则,无依赖环原则,稳定依赖原则,稳定抽象原则。

除了线上 CR,还有一种特殊的线下 CR 方法,就是跳过 MR,直接拉取代码,进行整体 CR,将评审意见在代码中标记为 TODO 或者 FIXME,然后 @ 相关开发进行改进。这样做最大的好处,就是避免受单个 MR 的影响,掉入只见树木不见森林的陷阱。

2.2 团队层:衣带渐宽终不悔,为伊消得人憔悴

接下来再看第二层,如何从团队视角认识 CR。前面说了,CR 本质上是一种讨论,培根说过「读书使人完整,讨论使人完备」,从个人到团队,CR 分别意味着什么?

  • 提升自我觉察能力:这是从个人视角来看,当你知道你写的代码会有另一双眼睛来审阅,那你写代码时就会保持一份警觉,放弃天知、地知、我知、你不知的幻想,认认真真写好每一行代码。

  • 建立良好开发节奏:这是从团队视角来看,CR 是团队的同步器,每个人既是自己 MR 的作者,又是别人 MR 的评审者,从 MR 到 CR,再从 CR 到 MR,构成了每个工作日最动听的乐章。

  • 高频次的团队活动:这也是从团队视角来看,CR 既然是讨论,那么就不仅仅是一个人的事,而是一种团队活动,一种高频次、高质量、低成本的极具性价比的团队活动。

2.3 文化层:众里寻他千百度,蓦然回首,那人却在,灯火阑珊处

最后是文化层,CR 既是传帮带文化的重要组成,又是工程师文化的日常体现。

  • 传帮带文化的重要组成:资深工程师 CR 初级工程师的代码,可以给予高频次、高质量的指导;初级工程师 CR 资深工程师的代码,可以欣赏、学习高手如何把玩代码,取其精华去其糟粕。

  • 工程师文化的日常体现:协作、高效、进取、影响力,这些在各大互联网公司的工程师文化中频频出现的关键词,无一不与 CR 紧密相连。不夸张的说,工程师文化香不香,就看 CR 做的好不好。

3 行:CR 高效之法

认识完 CR,我们再来探讨一下如何高效的进行 CR。在我看来,高效 CR 首先有赖于以下几个客观条件和主观条件。

客观上来看,和谐的工程师文化清晰的代码规范是高效 CR 的两块基石。所谓和谐的工程师文化,就是说团队对代码秉持开放的心态,不藏着掖着,以写好代码为荣,以写坏代码为耻,持续关注代码质量。而清晰的代码规范,一方面提高了代码的可读性,另一方面也统一了编码风格,极大的减少了不同代码风格对评审者注意力的干扰。

主观上来看,对评审者而言,第一要端正态度,保持谦卑的心态,人非圣贤孰能无过,择其善者而从之,其不善者而改之。第二要谨记评审的对象是代码,而不是人,你写下的每一条评审意见都应基于客观事实和数据,做到有理有据,不带个人情绪。

基于我多年 CR 的实操经验,结合Google Code Review Developer Guide,我整理了一些高效 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 不是只能提意见,看到优雅的代码不要吝啬你的表扬。

4 小结

2019 年 Stack Overflow 的一份调查报告显示,超过 7 成的程序员会把 CR 当做日常工作的一部分,近 1/3 的程序员每周在 CR 上花费 2~3 个小时,还有 1/3 的程序员每周花费 4~5 个小时。心里默默算一下,你是在拖后腿还是领路者?如果你还没做过 CR,那么赶紧行动起来;如果你已经在 CR,很好,请继续保持。一花一世界,一叶一菩提,码中自有乾坤。CR,走起!

5 参考