谷歌最佳实践 - 代码审核方法

来源

代码审核时我们应该审核什么

注意:在考虑下面的原则时,切记要根据《代码审核标准》进行考虑。

设计

代码审核中最重要的事情就是考虑一下变更提交的整体设计。变更提交中各个部分的关联交互是否合理?这些变更是应该在代码基线中,还是应该提交到支持库中?这些变更是否能够与系统内的其他部分很好的整合?现在是不是加入这个功能的合适时机点?

功能性

变更提交是否实现了开发者的真正目标?开发者期望的对于代码的使用者是否有好的效果?这里的使用者包含终端用户(会受到变更提交的影响)和开发者(会在将来使用到这些代码)。
大部分情况下,我们都希望开发者在提交代码审核前能够事先做好充分的测试。但是作为审核者你还是要考虑边界情况,并发问题,尝试像用户一样思考,确保你审核的这些代码中不会产生任何bug。
当有必要时你需要验证变更提交的内容 - 当变更对用户交互产生影响,比如UI变更。有些时候单纯读代码是很难真正理解变更产生的影响。类似这样的变更,如果你直接从代码很难直接运行,可以要求开发者提供功能的演示。
另外一方面在审核代码,要特别重视类似并发等特别重要的概念,防止存在引发死锁或者竞争条件的写法。这类的问题即使运行代码也是比较难发现的,常常需要开发者和审核者仔细思考确保问题不会发生。(当可能存在竞争条件或者死锁时,尽量不要采用并发模型。这会导致代码审核或者理解都变得更加复杂。)

复杂度

变更提交的内容是否必须这么复杂?每一级都要这样检查:单行代码是否过于复杂?方法函数是否过于复杂?类是否过于复杂?“过于复杂”通常意味着”不能被读代码的人快速理解“。同样也意味着“当开发者尝试修改代码时更容易引入bug”
一种常见的复杂化就是过度设计,将代码过度通用化,或者加入目前系统不需要的特性。审核者需要特别警惕这种过度设计。鼓励开发者针对当下需要解决的问题进行处理,而不是认为将来会需要解决的问题。将来的问题应该在出现的时候才需要解决,那时候你才能真正看清它实际的特征和需求。

测试

针对变更内容,应该提供单元测试、集成测试或者端到端测试。大体上,应该在变更提交中包含测试代码,除非变更提交是因为处理紧急问题。
确保代码中的测试是正确、合理、有效的。测试代码不是为了测试本身,我们基本不会针对测试写测试代码,我们自己来保证测试代码是有效的。
当代码异常时测试是否真的会失败?如果基于测试进行代码变更,是否能够产生负的正向反馈?是不是每个测试方法内的断言都是简单有用的?测试代码在不同的测试方法中的分布是否合理?
切记测试代码也是代码,也同样需要维护。不要因为这些测试代码不在主代码中就允许它非常复杂。

命名

开发者有没有设计良好的命名?良好的命名能够充分体现自己是什么或者做了什么,反之则会造成代码不具备可读性。

注释

开发者有没有用良好表述的语言写了清楚的注释?是否所有的注释都是必须的?通常有用的注释是解释某些代码存在的原因,而不是某些代码在做什么。因为如果代码不能清楚的自描述,你就应该把代码处理的更简单一些。可能存在一些例外(例如:正则表达式,复杂算法常常需要注释来描述具体的工作内容),但是大部分的注释应该是说明代码不能包含的内容,比如做这个决定的原因。
在审核变更前先看一下注释也可能会有帮助。也许有一个待办项应该移除掉,一个注释的建议与实际的变更冲突等等。
注意:注释和类、模块、方法等的文档不同,文档是为了表达代码的目的,如何使用,使用时会产生何种效果。

样式

在谷歌,我们提供了所有主要语言的《样式指南》,甚至包括大部分的低频语言。要确认变更提交是遵守了样式指南的。
如果你希望能够改进某些在样式指南中没有提到的样式点,在你的评论前加上”Nit:“的前缀,这样开发者就知道这是一个改进点而不是强制的。不应当因为个人的样式喜好而拒绝变更提交。
开发者不应当在主样式调整中混合其他提交。这样会使得变更提交中具体变化了什么内容,使得合并与回滚也更加复杂,还会引发其他问题。例如一个开发者希望重新格式化代码,那就应当将重新格式化的部分提交一次,其他的变更在这次之后另外提交一次。

文档

如果一次变更影响到用户的构建、测试、相互引用、发布代码等,需要确认有没有更新关联的文档,包括 READMEs(项目自带文档),知识库页面,以及其他关联生成的文档。如果变更删除或者过期了代码,确认相关的文档是否被删除。如果文档没有更新或提交,要求要补齐。

每行代码

仔细查看你分配到审核的每行代码。类似数据文件,生成代码,或者很大的数据结构之类的有时可以大概看一下,但是手写的类、方法或者代码段绝对不要大概看一下,假设不会有太多问题。有些代码就是比其他代码需要更加仔细的审核,这就是你作为一个审核者需要决定的,但是至少确保你理解了代码的逻辑。
如果发现看代码很困难并且导致审核的速度变慢,你应该让开发者明白问题所在并且等他们澄清整理之后再尝试审核。在谷歌我们只雇佣像你这样优秀的软件工程师。如果你都无法理解代码,那么其他的开发者也同样很难理解。当你要求开发者理清代码时,也是在帮助今后其他开发者能够理解代码。
如果你能理解代码,但是你觉得自己还不够资格来做某部分代码的审核,确保有另外一位有资格的审核者能够负责,例如安全、并发、可访问性、国际化等复杂问题。

整体

将变更提交放到一个完整的上下文中来查看一般会更有帮助。代码审核工具一般都是展示变更附近的代码。有时候你是需要查看整个文件来确认这些变更是合理的。例如,你只看到添加了四行代码,但是当你查看整个文件会发现这四行是在一个五十行的方法中,而这个方法应该要拆分到更小的方法中。
同样将变更提交放到系统的上下文中作为一个整理来思考也是很有帮助的。这份变更是否会提高代码的健康程度还是是的系统更加复杂、更少的可测试之类的问题?决不接受降低系统健康程度的变更提交。大部分系统都是因为不断添加的小变更使得整体系统的复杂度不断提高,所以阻止新增提交中的复杂度提升是很重要的。

闪光点

如果在审核中发现了代码中的闪光点,请务必告诉开发者,特别是他们用很棒的方式实现了你评审中的要求。审核者往往关注于问题,但是他们也应该欣赏和鼓励一些优秀的实践,这些行为在有些情况下更有价值,换言之就是要告诉开发者哪些做对了比告诉他们哪些做错了更重要一些。

总结

当审核代码时,你应该确保:

  • 代码应该是经过良好设计的。
  • 功能都应该是对于用户有帮助的。(引用代码的开发者和终端用户)
  • 任何UI修改都是合理并且展示良好。
  • 所有的并发操作都要是安全的。
  • 代码不应该过于复杂。
  • 开发者专注于眼前的问题,而不要考虑实现将来可能要解决的问题。(不要过度设计)
  • 代码中包含合理的单元测试。
  • 测试的设计都是合理的。
  • 所有的命名都很清晰合理。
  • 注释清晰有效,大部分都是解释为什么,而不是做了什么
  • 代码有合理的文档(谷歌内部文档平台是g3doc[1])。
  • 代码符合样式指南。

确保你审核了每一行代码,联合了整体上下文,确保你通过的代码时提高了代码的健壮性,并且在开发者的闪光点上给了足够的鼓励和关注。
下一篇:变更提交审核时的建议路径


  1. 参考Quora的这篇问题What is g3doc,谷歌内部的工程文档平台,并且和版本控制系统结合。

©著作权归作者所有,转载或内容合作请联系作者
  • 序言:七十年代末,一起剥皮案震惊了整个滨河市,随后出现的几起案子,更是在滨河造成了极大的恐慌,老刑警刘岩,带你破解...
    沈念sama阅读 205,132评论 6 478
  • 序言:滨河连续发生了三起死亡事件,死亡现场离奇诡异,居然都是意外死亡,警方通过查阅死者的电脑和手机,发现死者居然都...
    沈念sama阅读 87,802评论 2 381
  • 文/潘晓璐 我一进店门,熙熙楼的掌柜王于贵愁眉苦脸地迎上来,“玉大人,你说我怎么就摊上这事。” “怎么了?”我有些...
    开封第一讲书人阅读 151,566评论 0 338
  • 文/不坏的土叔 我叫张陵,是天一观的道长。 经常有香客问我,道长,这世上最难降的妖魔是什么? 我笑而不...
    开封第一讲书人阅读 54,858评论 1 277
  • 正文 为了忘掉前任,我火速办了婚礼,结果婚礼上,老公的妹妹穿的比我还像新娘。我一直安慰自己,他们只是感情好,可当我...
    茶点故事阅读 63,867评论 5 368
  • 文/花漫 我一把揭开白布。 她就那样静静地躺着,像睡着了一般。 火红的嫁衣衬着肌肤如雪。 梳的纹丝不乱的头发上,一...
    开封第一讲书人阅读 48,695评论 1 282
  • 那天,我揣着相机与录音,去河边找鬼。 笑死,一个胖子当着我的面吹牛,可吹牛的内容都是我干的。 我是一名探鬼主播,决...
    沈念sama阅读 38,064评论 3 399
  • 文/苍兰香墨 我猛地睁开眼,长吁一口气:“原来是场噩梦啊……” “哼!你这毒妇竟也来了?” 一声冷哼从身侧响起,我...
    开封第一讲书人阅读 36,705评论 0 258
  • 序言:老挝万荣一对情侣失踪,失踪者是张志新(化名)和其女友刘颖,没想到半个月后,有当地人在树林里发现了一具尸体,经...
    沈念sama阅读 42,915评论 1 300
  • 正文 独居荒郊野岭守林人离奇死亡,尸身上长有42处带血的脓包…… 初始之章·张勋 以下内容为张勋视角 年9月15日...
    茶点故事阅读 35,677评论 2 323
  • 正文 我和宋清朗相恋三年,在试婚纱的时候发现自己被绿了。 大学时的朋友给我发了我未婚夫和他白月光在一起吃饭的照片。...
    茶点故事阅读 37,796评论 1 333
  • 序言:一个原本活蹦乱跳的男人离奇死亡,死状恐怖,灵堂内的尸体忽然破棺而出,到底是诈尸还是另有隐情,我是刑警宁泽,带...
    沈念sama阅读 33,432评论 4 322
  • 正文 年R本政府宣布,位于F岛的核电站,受9级特大地震影响,放射性物质发生泄漏。R本人自食恶果不足惜,却给世界环境...
    茶点故事阅读 39,041评论 3 307
  • 文/蒙蒙 一、第九天 我趴在偏房一处隐蔽的房顶上张望。 院中可真热闹,春花似锦、人声如沸。这庄子的主人今日做“春日...
    开封第一讲书人阅读 29,992评论 0 19
  • 文/苍兰香墨 我抬头看了看天上的太阳。三九已至,却和暖如春,着一层夹袄步出监牢的瞬间,已是汗流浃背。 一阵脚步声响...
    开封第一讲书人阅读 31,223评论 1 260
  • 我被黑心中介骗来泰国打工, 没想到刚下飞机就差点儿被人妖公主榨干…… 1. 我叫王不留,地道东北人。 一个月前我还...
    沈念sama阅读 45,185评论 2 352
  • 正文 我出身青楼,却偏偏与公主长得像,于是被迫代替她去往敌国和亲。 传闻我的和亲对象是个残疾皇子,可洞房花烛夜当晚...
    茶点故事阅读 42,535评论 2 343

推荐阅读更多精彩内容

  • 来源 代码审核标准 代码审核的核心目的是保证谷歌代码在不断的改进发展过程中还能持续保证健康。所有代码审核的流程与工...
    陈晨_软件五千言阅读 388评论 0 1
  •   自从 2000 年以来,Web 开发方面的种种规范、条例正在高速发展。Web 开发过去曾是荒芜地带,里面东西还...
    霜天晓阅读 495评论 0 1
  • 安昕瑜,蔡新宇,孔庆振 翻译 2017年1月31日 弗格斯·亨德森 <fergus@google.com>(工作)...
    senju阅读 3,614评论 0 2
  • 第二章 顾明月的心愿(4) 云月,不,顾明月莞尔一笑,从秋千上跳下来,整了整自己身上穿着的蓝底碎花百褶裙的裙摆,抬...
    酱油的日常生活阅读 756评论 0 4
  • HashMap也是我们使用非常多的集合,它是基于哈希表的 Map 接口的实现,以key-value的形式存在。在H...
    杨柳滔滔阅读 3,356评论 3 106