评审的艺术——谈谈现实中的代码评审

曾经写过一点关于代码评审(code review)的文章,比如 这篇这篇 ,现在觉得关于它的认识又有了不少更新。软件工程的技术和实践分成两部分,一部分是和书本知识一致的,大约占一半,这部分基本上在大学里就可以学,自学只要方法得当、刻苦努力也可是途径;但是第二部分来自于实际团队、经验,内容通常无法从书本当中获得,而且难说对错,不同的人和不同的经历造就了不同的认识。代码评审就是第二部分颇具槽点,可以大加讨论的典型。

代码评审是展现个性和性格的途径

我本人特别反对一种颇为常见的观点,就是“一个良好运作的项目,不同的人,应该写出一样的代码”。我非常理解这种观点的初衷,一个良好规范约束的团队中,不同的人写出来的代码应当一致。但实际上,能真正这样做的团队,我还根本没有见到过。所谓的一致代码,仔细品味,发现不同的工程师写出来就是不一致。因此“一致”这个词一定是在一定程度内的大体描述而已,并非越一致约好。 其实,代码的创造本就是具备个性的。毫无疑问我们应当遵从规约,应当符合习惯,但是一旦试图过度使用它们去约束代码,不但难以落实,而且容易产生无趣、低效和矛盾的氛围。

再回到代码评审上。代码评审本身,以及基于评审意见的来回沟通,和代码本身比起来,要更难以,也更不应该要求一致。我见过许多代码评审的风格,有人喜欢挑小毛病,有人喜欢展开观点夸夸其谈,有人喜欢给实际例子来证明自己的看法……无论哪一种风格,我都不觉得有什么大的问题。但是,就我个人而言,我可以谈谈我自己的代码评审风格:

我会关注三种问题,需求和业务上的问题、代码结构的问题、代码风格格式的问题。

前两种可能存在阻碍我 ship 代码的“严重”问题(说“ship”就意味着认可代码具备了 push 到主线分支的条件了)。我已经记不清多少次和代码作者因为这样的问题争论了。争论是个艺术,有时候并不一定能够达成一致,有的人比较容易被说服,有的人则更坚持己见。 我不想说哪一种更好,但是确实这是代码评审中展现风格的事实——都是就事论事,但有人害怕或者不喜欢得罪人,就会显得 push over 一点;有人则不在乎那么多,坚信自己的想法更合适,就会显得 defensive 一点。 我可能属于后者,似乎在职业生涯的各种阶段都会有和我出现代码评审冲突的事例,但是在某些情况下我也会选择 disagree and commit(不同意但是执行团队达成的意见)。我相信有些团队会喜欢我的 backbone,也会有团队讨厌我的这种风格。下面的内容,也多为基于自己风格的表述。

坚定自己的意见,但是委婉地表达

关于这一点,我也在努力地改进。可以提及的点很多,技巧也很多,但是老实说,冲突还是不可避免。在我经历的几乎所有的团队中,有时候会有老好人,但是基本上所有的老好人都缺少对于原则的坚持。沟通是门艺术,在代码评审中也一样体现。

  • 最重要的一条,只针对代码,不针对人。这条很简单,都知道对事不对人的重要性,但是要非常小心不能违背。
  • 对于大多数代码风格格式上的问题,我都会标注这个问题是一个 picky 或者 nit 的问题(“挑剔的问题”,这是我在 Amazon 的时候学到的方式)。这样的好处在于,明确告知对方,我虽然提出了这个问题,但是它没有什么大不了的,如果你坚持不改,我也不打算说服你。或者说,我对这个问题持有不同的看法,但是我也并不坚信我的提议更好。
  • 使用也许、或许、可能、似乎这样表示不确定的语气词(包括使用虚拟语气)。这样的好处是,缓和自己观点表达的语气。比如:“这个地方重构一下,去掉这个循环,也许会更好”。
  • 间接地表达质疑。比如说,看到对方用了一个参数 3,但是你觉得不对,但又不很确定,可以这样说:“我有一个疑问,为什么这里要使用 3 而不是其他值?”对方可能会反应过来这个值选取得不够恰当。
  • 放上例子、讨论的链接,以及其它一些辅助材料证明自己的观点,但是不要直接表述观点,让对方来确认这个观点。比如:“下面的讨论是关于这个逻辑的另一种实现方式,不知你觉得是否会稍简洁一些?”
  • 先肯定,再否定。这个我想很多人一直都在用,先摆事实诚恳地说一些同意和正面的话,然后用 however 一转,说出相反的情况,这样也就在言论中比较了 pros 和 cons,意味着这是经过 trade-off 得出的结论。
  • ……

一些很常见的可以在代码评审中提及的问题

这样的问题其实有不少,多数和实现的技术无关,但是又很容易不小心略过。它们多数时候是问题,当然也有时候不是。

  • 比如说,我最痛恨的之一,职责过于宽泛或者职责不清的类或模块。我无数次见过这样的类:Helper,或者 Utils(注意,它们都没有模型或者模块前缀)。考虑项目的规模,大多数情况下,这样的类很容易变成一个越吹越大的气球,似乎什么东西都可以往里搁,是个十足的违反单一职责原则的糟糕设计。
  • 比如说,在线程使用,容器使用,连接管理……中,缺乏上限控制的设计,在一些情况下导致资源使用过度膨胀。生产者和消费者的队列设计,一旦消费者挂掉或者跟不上,队列里越堆越多,没有拒绝机制。
  • 再比如说,缺少分包、分层,所有的东西都叠在一起,十几个,甚至几十个类文件并列在同一个文件夹下面。
  • 再再比如说,代码缺乏设计,流水账结构,面条型代码,或者简单铺陈几个过程式的方法,这种修改往往代价还不小,自然修改的落实率低,因而令提出问题的人也颇为头疼。
  • ……

避免一次评审提及过多的问题

少数情况下,手里拿到一份实在是问题多到令人发指的代码,这时候需要扼制自己想骂人的冲动。 先抓问题的主干,不要去挑那些细枝末节的毛病,因为很有可能,这些代码会被改得面目全非,或者重写。写一大堆评审意见,反而容易淹没最重要的问题。

说说容易,但是这个度有时候并不好掌握。我的习惯是,在明确代码要解决的问题以后,先快速走读一遍代码,判断我是应该粗略地抓主要矛盾呢,还是应该严格地挑刺呢。我也见过一些别的方式。

不一样的评审要求

我始终觉得, 代码评审的要求不应该完全一致。不要过度追求公平。对于新项目代码,以及新员工写的代码,要适当严格一些。

新项目的代码是形成后续风格和质量的模板。我们也许都听说过“破窗户原理”,在一块干干净净的代码田地里,新耕种的代码才容易保持一致,也可以避免一些“为了和现有代码风格保持一致”而导致质量妥协的情况出现。

新员工代码的高质量要求则更多地在于对他们良好习惯养成的负责。软件工程师良好职业习惯的养成,代码可以说是最重要一环,而前三个月的影响举足轻重。

还不如不做的评审

有些情况下,代码评审是非常耗时费力的工作。特别是对业务背景不熟悉,对实现技术不熟悉,或者干脆是对方提交上来一大堆修改,阅读非常费力。我不知道哪一种是最难的,但是如果因为这些原因很难做到良好的评审,我会在评审中说明,或者放弃一些评审的工作,保证评审的质量。

代码评审是建立在团队中影响的好方式。 一方面是业务逻辑,一方面是代码结构,我反对在两方面都难以给出足够清晰的评审意见的时候,提一堆风格方面的次要问题。否则很容易达成负面影响。

先谈到这里,我想还有不少可以涉及的内容,而它们都难以在课堂和书本上学到。争论才有意思,实践出真知大抵如此吧。你有什么一致的看法,或者反驳,欢迎和我讨论。

文章未经特殊标明皆为本人原创,未经许可不得用于任何商业用途,转载请保持完整性并注明来源链接 《四火的唠叨》

2,911 次阅读

5 thoughts on “评审的艺术——谈谈现实中的代码评审

  1. 坚定自己的意见,但是委婉地表达
    这点感觉是最容易理解,但是最难做好的部分,不过楼主给的例子很棒,平时工作中很多老美也是这样,上来先说自己这个观点可能是偏颇的,然后再说出来自己的想法,这样听得人会觉得这个人很谦虚,就更容易接受一点。

  2. I have noticed you don’t monetize your website, don’t waste your
    traffic, you can earn additional cash every month because you’ve got hi quality content.

    If you want to know how to make extra bucks, search for: Ercannou’s essential adsense alternative

  3. 两点讲的很赞,学到了:

    – 标注 comment 的类型是 picky/nit
    – 避免一次评审提及过多的问题

    这两点本质上都是差异化 Code Review Comment 的等级,
    也从 Committer 的角度去想怎么样是真正对代码负责的。

    科学。

发表评论

电子邮件地址不会被公开。

back to top