代码审查 我看代码审查 by 老万

laofo · 2017年05月03日 · 最后由 laofo521@gmail.com 回复于 2017年05月03日 · 1 次阅读

我看代码审查(一):工具的变迁 先讲一个悲伤的故事。

读博的时候,我给一门编程课做过助教。有一天,我收到了这样一份作业:

#define ZERO 0 ... #define FIVE 5 #define SIX 6 … if (size > threshold) return SIX; else if (x < ZERO) return FIVE;

我问这位同学:你这是搞哪样?他说,老师在课堂上讲过,不要在程序里面直接用魔法数 (magic numbers,就是没头没脑冒出来的数值)。

显然,这位同学没有理解问题的真谛。不用魔法数,是因为读程序的人不知道它们背后的含义,会不知道作者到底想要干什么。从阿拉伯数字换成英文,换汤不换药,对这个问题没有任何帮助。正确的做法是给常数一个代表它意义的名字。这样,读程序的人就可以轻松搞懂它到底是干什么的。从上下文看意义很清楚的常数,不必再取名字。比如上面这段代码可以改写为:

const int invalid_input_error = 5; const int capacity_exceeded_error = 6; … if (x > threshold) return capacity_exceeded_error; else if (x < 0) return invalid_input_error;

试想,代码库中充满 #define FIVE 5 这样的代码,维护它的人一定是欲哭无泪。这就是为什么我们要有代码审查(code reivew):代码在提交之前要经过同事评审,确定它符合一定的质量要求,正确性、效率、可维护性都要及格;否则就是埋下了一颗定时炸弹。

今天码工们对于代码审查这个制度应该都不陌生了:在绝大多数公司这都是一条起码要求。然而,在二十年前这还是一个新鲜概念。也许是我孤陋寡闻,在 1998 年之前,无论是在学校做论文还是打工,我和周围的同学同事从来都是文责自负,写什么都是自己说了算,代码库里充斥了各种奇葩段落。我在出国前甚至连版本控制(version control,就是把一个代码库的所有版本放在一个数据库当中,可以方便比较各个版本的差别,出了问题能回滚到以前的版本)的概念都没有,整个系统只有一个最新版本,一存盘以前的版本就被覆盖了,现在想来后怕不已。

1998 年夏天,我到微软总部做实习生,第一次接触到了版本控制和代码审查这种高级文明,惊为天物。它们给我的印象如此深刻,以至于在三年后回母校的时候,我还把它们当作在微软学来的先进经验在曾经工作过的科大恒星公司(现在是上市的科大国创)大力宣讲,同学们忽闪着大眼睛,听得相当好奇。

代码审查是依赖于版本控制系统存在的。没有版本控制,所有的源文件都只存最新版本,没办法和旧版本对比,审查也就无从谈起。我最早接触到的版本控制系统是在微软用到的 Visual Source Safe 。这是微软开发工具 Visual Studio 早期的一个部件,只支持小规模的代码库。微软自己的代码量对它来说是太太太大了,所以大部分代码是用另一个性能更高的命令行式系统 Source Depot 来管理。不过微软不同产品有不同的代码库,我实习时在的 MSIT(微软内部技术)组的代码量小,用 Visual Source Safe 就可以了。

如何审查呢?在本世纪才开始工作的朋友可能不会相信:每次写完一个功能,我就把我的 mentor(负责指导我实习工作的同事)请过来,把代码打开,鼓动如簧之舌,介绍我改动的目的和实现方式。同事提点问题挑点刺,我再当场修正。总之把问题都摆到桌面上来,下面就好进行了。你来我往几次,最后大家都满意了,就按下一个按钮,把新版本正式加入到代码库中。

这种面对面交流的方式虽然简单易学,但是存在不少问题: 必须要两人同时在场才能进行。有时为了凑到一起,不得不打断正在做的事,增加了任务切换的成本。现代社会为什么出轨这么多?因为技术的发展实现了撩妹异步化,不用面对面,机会多了啊!要提高代码改动的吞吐量,必须实现审查异步化。 对于复杂的项目,经常需要深入的思考才能真正理解一个系统要解决的问题和它的设计实现,然后提出中肯的意见。突然面对面,啥事都要当场拍板,没有时间做深层次的思考,难以形成高质量的意见,往往沦为走形式。 好的代码,应该是无需解释就清晰明白的。原作者在旁边给你讲懂了,不代表这个代码让人一看就懂。而且被作者牵着走,容易犯同样的错误。这种审查方式,没有给审查人一个先独立思考自行理解的机会,会漏掉一些可读性的问题,甚至重大设计问题。 有的审查人碰到自己不太懂的地方,不好意思当面承认,也不可能当场学习调查,只好稀里糊涂盖个橡皮图章。就像奥斯卡影帝小李子在电影《有本事你来抓我啊》(Catch Me If You Can)中演的那个百变骗子,冒充外科大夫的时候,别人问他的意见他总是先反问别人怎么看,然后说 “我也这么看!”(I concur!),别人听了很受用,就不再深究。这样屡试不爽。 审查的过程没有形成文字记录,后来的人只看见代码这么改了,却不一定搞得明白这么改的原因。甚至当事人过一段时间自己都不记得了,多年以后看得泪流不止。就像那个老笑话所言:张丞相好草书而不工,时人皆讥笑之,丞相自若也。一日得一句,索笔疾书,满纸龙蛇飞动。使其侄录之。当波险处,侄罔然而止。执所书问曰:“此何字也?” 丞相熟视久之,亦不自识。诟其侄曰:“汝胡不早问,致余忘之。”

一般说来,如果项目简单程序员水平很高,这种方式还行得通;碰上复杂的项目或是程序员水平参差不齐的情况,结果堪忧。

当时微软有位著名的华裔程序员 Raymond Cheng,他写了一个工具来解决这个问题。这就是 bbpack(build buddy pack)。它可以把对多个源文件做的修改打成一个包,然后通过邮件发给审查人,让审查人在自己的机器上把包打开,不慌不忙地比较新旧版本的区别,甚至关上门悄悄问谷歌,把问题搞懂。等审查人把问题都想清楚了,再把意见写下来发给作者。这个工具解决了面对面的最大痛点,据我所知在微软用得很广泛。

我在谷歌上班之后很长一段时间,用的是类似的方式。虽然强于面对面,这还是有很多不足。最大的困扰是代码和意见是在两套完全隔离的系统中(前者在版本控制系统,后者是邮件),很容易脱节。一个复杂的改动,可能涉及到很多文件,每个文件也可能改好几个地方。所以,审查人可能会提很多意见。如何搞清楚哪条意见是针对哪段代码呢?我们采取的是原始的方式:直接写下文件名和行号。比如有一天我的审查人说:

老万,这个 photo/view/people.cc 文件的第 135 行到 137 行,应该做一个优化,把人脸按颜值先排个序再显示,确保你和马云的头像都出现在倒数第一页,这样可以提升用户体验。

(好吧,我们程序员排序,缺省都是从低到高的。不过马云是怎么回事?)

那好,我去做了这个改动,然后就发现,这个文件行数变多了,因为以前三行的代码,现在改成了三十行。于是,那个文件后面所有的行号都不正确了,要相应地往后挪。如果审查人要求我在 10 个文件中改 20 个地方,光是数行号就够我忙活的了。所以,我被迫养成了一个习惯来适应这种原始的生产方式,就是倒着改:先解决文件尾部的问题,再顺着往前改。这样即便后面代码增加了或者删除了,不影响前面的行号。然而这种做法不一定总是可行,很多时候做一件事需要修改不同的文件,为了完成文件 A 尾部的一个修改,不得不同时修改文件 B 的开头和文件 C 的中间,所以没法保证所有文件都是从后往前改的。意见多的时候,还是经常数不清行号。此外,一次代码审查经常需要好几轮的讨价还价,一次改不好再改一次。所以不光是要比较两个版本,而是每轮都有一个版本。如何记得哪些意见已经被处理过了,哪些还是正在处理的?双方的负担都很重,效率低下。

怎么办?程序员是这个星球上最爱开发工具的生物,那我们就做个更好的工具呗。几年后,Python 语言(按 codeeval.com 的统计,Python 已经连续 5 年是最流行的编程语言,超过了 C/C++, Java, C#, …...)的发明人 Guido 老大加入了谷歌。他老人家一看,这么烂的工具怎么能忍!作为投名状,他开发了一个基于网络的代码审查系统。因为 Guido 是河南,咳咳,荷兰人,他给系统取了个自己的老乡,荷兰画家 “猛钻”(Mondrian)的名字。这个系统大家用了都说好。后来 Guido 把它开源的时候,改了个名字。他还是心系祖国,这次用的是一位荷兰建筑师的名字,叫 Rietveld (关于这段历史,更多故事请看 https://www.gerritcodereview.com/about.md )。相比之下,我做系统只会取像 Google Test 这样直白的名字,真是一个没有教化的野蛮人。

Mondrian 长这样,真的不是张震投胎?我不信我不信!(图片来自 wikipedia)

这个 “猛钻” 代码审查系统,比之通过邮件来交流的方式有天壤之别。它直接和版本管理系统集成,在浏览器上同步显示审查意见和代码,从审查开始到结束的 N 个版本,可以任选两个对比。每条意见,系统会开一条线索跟踪,可以不断追加后续意见。处理完的时候,可以点一个小框确认,下一轮审查这条线索就不再出现。对于我这样的审查话痨来说,这真是一款极大提高了生产力的神器。Guido 离开谷歌好几年了,但是 Mondrian 的升级版本还在被不断使用改进,我每天都在上面杀掉不少时间。

下一次,我想谈一谈多年审查代码积累的一些技巧和小经验。敬请关注。

我看代码审查(二):修炼的要点

衡量程序员能力的维度有很多,包括设计、编程、领导、沟通、写作、颜值等等。其中编程能力做为程序员的安身立命之本,我觉得是最重要的一个维度。代码写得好,要饭要到老?那是写不来代码的同学黑我们的!代码写得好,不一定能成为大牛,但是我没有见过哪个大牛程序员是不屑于写好代码的。有人说,设计难道不是更重要吗?是的,设计能力很重要,但说到底这是是编程多了建立起来的一种感觉。世界上本来并没有路,代码写多了,就有了套路。而设计,就是知道什么情况下该用什么套路。所以,归根结底代码要写得好。

程序员在编程能力的进化途径上,我认为要经历四个阶段:刚出学校的初级程序员,需要解决的是温饱,啊不,是代码的正确性。就是说让代码干咱们想让它干的事儿,不要干咱们不想让它干的事儿。至于干得好不好,跑得有多快,长得好不好看(我是说代码)......,他们一时还顾不上。中级的程序员,要追求系统的效率和稳定性。代码不光要干正确的事,还要吃得足够少(少占内存、CPU、文件等资源),跑得足够快,在满负荷的情况下不要崩溃。高级的程序员,在此基础上还要保证代码的可维护性和可扩展性,降低开发成本,提升系统对需求改变的应对能力。这样,有一天他以身殉职倒在生产线上的时候,同事们在洒下鳄鱼的眼泪的同时,还可以踏着他的遗体继续前进。高级之后,要成为大牛,必须要有卓越的眼光,知道什么问题是最值得他花时间去解决的,写哪些代码能产生最大的绩效。这里说的绩效,不一定是炫酷的新功能,也包括提高系统性能和代码质量的优化、重构。我发现,处于入门境界的程序员,一般容易忽视重构的重要性,也不知道怎样重构才好。境界提升之后,又要警惕走入另一个误区,花太多时间重构系统。如何最搞笑,咳,最高效地用好自己的时间,如何时候都是一个重要的技能。

不明就里的群众可能会以为代码审查是一种高级技师对低级技师的培训。其实,代码审查是一个相互学习的过程。这些年我通过审查别人的代码学到了不少先进经验,也把我的浅见传播到了千家万户(就是黄老千家和我家)。我们程序员只有工分(就是干一个小时挣多少钱)不同,在审查代码的时候并没有高低贵贱之分。像我,就审查过我歌资深院士 Jeff Dean 的代码,值得炫耀。吗?我有拿出来到处说吗?

审查人的职责之一,是把代码的作者往高层次带。好的审查人,不光要指出代码中的不足,还要用最能被作者接受的方式指出,帮助作者更快地进步,把团队建好。和写代码一样,审查代码也是一门手艺,需要不断的学习、用心,才能精进。我审查代码十多年,不敢说自己审得多好,起码踩过不少坑,有一些经验。把这些经验写下来,能让大家少踩几个坑,我就欣慰了。

先讲几条代码作者需要注意的事项:

  • 不要在设计取得共识之前请求代码审查。设计和实现,是两个层次的东西,前者是画蓝图,后者是施工。如果一个系统的蓝图画得毛扎扎或者是想到哪造到哪,造出来的东西多半是老虎的头配猪的脖子加鸡的翅膀和乌龟的壳,怎么看怎么不是个玩意儿。所以,大多数情况下要早早审查系统设计,然后才开始编码。问题没有想清楚就动手?可以,那是在摸索阶段。设计没搞好就一鼓作气做出一个系统或者模块,远看也能跑,近看是玩具,徒靡弹药止增笑耳。有的人急性子,恨不得审查人一言不发盖个戳就好,这样可以马上转战下一个功能,把悲伤留给同事,升职归于自己。结果,他们做了很多无用功。废半天劲造好的豪宅,被发现千疮百孔,只得推倒重来。早年我做 Google Test 的时候,老板给我招了个新人帮忙。这位同学初来乍到,想做个大功能扬名立万,哗一下给我送了一个大的 CL(🆑,即 change list,指逻辑上相关联的一组改动。在版本管理系统中提交代码以 CL 为单位。)。我一看,不好,好多设计上的错误呢。于是我们就展开了讨论。这一开始就把不着边儿了,来回有一两百封邮件才搞定(此处并无使用夸张修辞手法)。现在想来,我也犯了一个错误:邮件两三次之后发现进展不快,就应该马上停下来改成开会,在一个高带宽的信道把认识统一了,再谈执行。

  • 先做好自己的功课,不要把一个半成品甩给别人审查,让别人当你的拼写检查器。这是对对方时间的尊重。唯一例外的情况是对方案的大方向还不确定时,可以先发一个征求意见稿,但这种情况一定要先讲清楚,免得对方在细节上浪费太多时间。一般来说,我在把一个 CL 送出去之前会先做以下这些事情:

    • 确保一个 CL 不要太大。动不动就几千行,让人望而生畏。一般来说,每个 CL 只做一件事,这样看起来省力而且出了问题回滚方便。我有个经验公式:一个 CL 的复杂度和它大小的 1.5 次方成正比。这个 1.5 是怎么来的?首先,如果一个 CL 中每行改动都是独立无关的话,那么每一行单独审查就好了,不必考虑它们之间的依赖性和交互作用。这种情况下审查的工作量是线性的。要是全部改动都是息息相关的,我们就不能把它们孤立起来看,而是每一行都要考虑它如何会和其它行发生联系,在一起会不会起化学反应。这样审查的工作量就大大提升了,是平方关系。考虑到实际情况介于两者之间,就算 1.5 次方好了。从这个假设,我有一个推论:把一个大的 CL 拆成两个 CL,审查起来更省力。有人说你这不是扯吗,改成小包装,总量并没有减小,工作量怎么可能下降?是这样的,一个 CL 不是随便拆开的,而是要有一定的逻辑性,保证拆分后的每个 CL 都是一个逻辑自洽的单位。拆分人把这个分解的工作做了,审查人就省事了,代码容易看懂,bug 也容易抓到。
    • 确保编译通过,测试通过。这点看起来很蠢吧,可是我还是看见有人会送编译不过的代码给别人审查。
    • 符合公司的编码规范和格式。有人说我司没有这些劳什子规定,那是不是就不用管这些了?雅蠛蝶!代码库没有一个统一的风格,工作起来是极辛苦的,因为要不停地花费能量在猜测作者用意上面。如果贵司还没有认识到风格的重要性,正是你力挽狂澜改变公司文化的好机会,还愣着干嘛,建立一个规范,你就是工程副总裁了。
    • 注释清晰易懂有条理,读之如坐春风,尤其不能有自己能发现的拼写错误和语法错误。做到这点并不难,只要先去读一个博士学位就好了。等你熟读百篇 paper,再被导师和审稿人虐五年,自然可以不分时间场合引言结语张嘴就来而且严丝合缝。没时间读博士?那么写个博客公众号也是极好的。看到好的段子,咳,文章,用心想一想好在何处,你从中能学习到什么新技能,再在自己写作时强行使用几次,水平肯定进步。总之要用心。
    • 每个 CL 都有个描述区,用来写关于这个 CL 的总注释。如果说一个 CL 是一篇论文,这个描述就是论文摘要。有经验的工程师会在这里简明扼要地写清楚这个 CL 要解决的问题和方案,方便日后检索。可是有很多同学不把这个 CL 描述当回事,千篇一律地写个 “修一个 bug” 或者是 “改进系统性能” 之类。这样一来增加审查的难度,二来以后系统出问题找原因的时候也会多好多事。
    • 如果这个 CL 很复杂,两段话说不清楚要干啥,那就另外写一个设计文档,在 CL 描述里指向这个文档。这不光帮助审查人理解,也让自己理清思路。很多时候,在试图把一件事表述清楚的过程中就会暴露出很多问题。能把一个东西写清楚,给别人讲懂了,才是自己真正懂了。

在代码审查过程中,要举一反三:如果别人给你指出一个错误,那你就主动把这个 CL 中类似的错误都改了。不要让别人像挤牙膏一样挤你。大家都是成年人了,配合一下更愉快。

最后,对负责任的审查人心存感激。他们挑你的代码的毛病,不是要跟你做对,而是在帮助你提高。虽然被人指出不足会有些不爽,要把这看做学习的机会,而不是任性地骂娘。

做为审查人,也有一些需要留心的地方:

  • 不要拖沓。要知道,你不 OK,别人就没法提交代码。如果你实在是忙不能审查一个 CL,早点告诉别人,他们可以马上另请高明。不要等到最后一刻才说对不起。我一般会在一个工作日之内回复一个 CL。

  • 有全局观。局部的最好往往不能带来全局的最优。如果一段代码只是一个试验性的原型而且时间很紧,那么精益求精的代价可能是影响了项目进度甚至引发连锁反应,让你的下游项目翻船。如果一个 CL 改动的是以后会被反复用到的基础模块,那就值得反复斟酌。

  • 懂得妥协的艺术。写代码一大半是技术,一小半是艺术。说到艺术就没有一定之规。你的蜜糖,可能是他的砒霜。如果只是个人喜好,没必要强加于人。你可以解释一下你这么做的好处,如果对方愿意照办当然好,如果对方坚持己见,只要不是很难逆转的决定,就随他去吧。一来同事之间没必要为了艺术品味伤了和气;二来你也有可能是错的;三来一件事情的决定权最终还是应该交给这件事的具体负责人:将心比心,我们每个人都不喜欢当被别人牵线的木偶吧;四来很多时候人不撞南墙是不懂得回头的,你得让他为自己的错误付出代价,他才会真正体会你的良苦用心,就像在爱情中受伤的众生。像我一开始也是疾恶如丑,只认道理不讲人情,后来发现效果并不好。现在的我,百密一疏,在不关键的地方网开一面,这次我让你,下次你再听我的,皆大欢喜。

  • 先扬后抑。批评别人的代码之前,先恭维几句。哪怕是一坨 shi,也可以夸它形态优雅,软硬适中。人是喜欢夸的动物,三天不夸,上房揭瓦。如果太直白,被批评的一方会觉得很受伤。所以提意见的时候保持谦恭的语气,与其说 “你这段代码性能太差”,不如说 “我觉得可以试试这种做法,性能可能会更高”。关于这一点,我做得也不够好,经常忙起来了就顾不上照顾同事的情绪,说得太直。让我们共同提高吧!

  • 看人下菜碟。对高级职称的代码作者,要求可以相应的高一些,因为这是在他们的能力范围之内。对于初级同事,如果用同样的标准,那可能会彻底摧毁他的三观。而且也不可能一夜之间让一个二级程序员成长为宇宙间独一无二没有人有资格评判的大神。人的时间和精力都是有限的,提的意见太多他能记住的可能反而会更少。不如抓大放小,一次解决一个问题。只要事情是在向好的方向发展就 OK。未竟的事业,写个 TODO 或者记录一个 BUG 下次再做就好了。

  • 出来混总是要还的。技术债务积到一定程度会爆发。而且大家做同一个项目,在代码库里乱扔乱放增加他人工作难度,不是缺乏公德就是能力不够。所以我都尽力阻止一个 CL 引入新的技术债务,除非是在紧急情况下。

  • 宣告式的(declarative)代码最美。软件开发,是一门关于抽象的学问。做房地产的都知道,房子最重要的三要素是:位置、位置、位置。做软件也有最重要的三点:抽象、抽象、抽象。什么是抽象?就是从具体、繁杂的操作中,抽取出高层次的概念,把实现的细节隐藏起来。我这个 “抽象” 的定义,本身可能过于抽象了。举个例子吧:计算机能直接理解的只有 0 和 1 组成的机器语言。这种语言,离机器近离业务远,用来直接写程序非常不现实,开发效率极低。把机器指令的编码方式抽走,改用英文指令名,就成了汇编语言,抽象程度比机器语言高一层,对程序员稍微友好一些。然而这还是太低级,离大多数业务隔了太多层,所以我们还要在上面包一层更抽象的高级语言,比如 C++,比如 Java。通用的高级语言运用起来方便多了,但相对于业务逻辑,经常还是不够抽象。比如 “数一数在一个文件中 ‘伟大、光荣、正确’ 出现的次数” 这个问题,用 C++ 实现起来要涉及到相当多的细节,包括文件的打开和关闭,内存缓冲区的分配,字符串的匹配,计数器的累加,等等等等。如果所有逻辑都在一个函数内实现,就会很复杂(参见前面的万氏 1.5 次方复杂度定理),读起来不能快速 get 到要点。如果把一个个步骤包成小函数,取上含意清楚的名字,再在主函数中调用,主函数读起来就会很贴近业务逻辑,也就是说抽象程度更高了。代码容易读了,就不容易隐藏 bug。我在读博士时的最大收获,就是提高了对代码的审美力,对如何抽象有了更好的把握。简单地说,最好的代码应该是 “宣告式的”(declarative),就是说它专注于告诉你它要干啥(what?),而不是具体如何干(how?)。见惯了好的,差的代码就能一眼识破了。

  • 自动化测试。人非圣贤,除了某大神,谁都不能保证自己写的代码不会出错。即便今天没错,明天加一个新功能的时候也可能不小心搞坏已有的功能。自动化测试的用处,就是在一定程度上保证重构、改进系统的时候已经有的功能不会被破坏。有了这个保证,我们改造系统就可以更大胆,步子迈得大一点也不会扯着蛋。虽然写测试和维护测试花了一些时间,长期地看开发的进度是快了而不是慢了 ---- 当然,也有测试写得不靠谱得不偿失的情况,所以前提是测试要得法。写好测试也是一种高端的技能,需要用心学习提高,那里的山路十八弯,那里的水路九连环,这里就不展开说了,以后有机会再细聊。

  • 自上而下。一个 CL 可能有很多层次上的问题:设计问题、正确性问题、效率问题、可读性问题......如果设计要改,很多代码可能都要推倒重写,这时候死抠这些代码的风格并无教育意义之外的意义,白白增加了工作量。我一般会先看有没有重大设计问题,如果没有,再看 class 和函数的合约(就是使用者和实现者之间的约定)是否合理(如果是 C++ 程序,意味着先看头文件)。这两样都没大问题了,再关注实现的细节问题。

下一次,我们会讲讲代码审查执行层面的一些事,欢迎关注。

http://mp.weixin.qq.com/s/dNsuoubAd7Da-SjYPD0iew

需要 登录 后方可回复。