• Code Review容易忽视的3件小事
  • 发布于 1个月前
  • 64 热度
    0 评论
Code Review(代码评审)是一种流行的软件开发实践。通过在代码合入主分支前引入人工评审,能有效促进成员间的知识交流,提升软件质量。我以评审者的身份参与过大量代码评审。在评审一份代码时,有些事项长期处在我的关注榜头部,比如设计是否考虑到了边界情况、代码是否有合理的单测覆盖。也有一些事项,因看似无关痛痒一直未引起足够重视,直到最近,我才渐渐发现它们的重要性。

以下是曾被我忽视的 3 件重要的小事。
1. 命名
小女孩千寻误入汤婆婆为神明开设的浴场。为了留在浴场内工作,千寻与汤婆婆签订了一份协议,但协议并非重点,重点是另一件看似无关紧要的小事——汤婆婆给千寻改了个名:从“千寻”改为“千”。一旦失去了原本的名字,人们便失去了逃离浴场所在的异世界的能力,甘心永世被汤婆婆所奴役。——电影《千与千寻》

程序员们对“命名”的关注程度似乎呈一个“倒 U 形”曲线。缺乏经验时,对命名的关注度很低,代码中充斥着各类不准确、不精确的名字,无法有效描述各种抽象概念。

下面这段代码中的命名就存在不少问题:
def get_var(u):
    """获取环境变量列表"""
    data1 = UserVarsManager.get(u)
    data2 = SiteVarsManager.get(u.site)
    return data1 + data2
随着经验逐渐增加,大家对命名的关注度逐步提升。项目中的名字开始变得更具有描述性,含糊不清的名字渐渐绝迹。名字至少不会成为他人理解代码时的屏障。

这个阶段,代码会逐渐演变成像是这样:
def list_environment_vars(user):
    """获取环境变量列表"""
    items_user = UserVarsManager.get(user)
    items_site = SiteVarsManager.get(user.site)
    return items_user + items_site
在绝大多数评审中,这绝对算是一份合格的代码,至少不大可能因为命名应发争议。自此之后,大部分程序员们对命名的关注度进入“倒 U 形”曲线的后半段:不再如从前那般关注命名,名字只要有一定描述性,不造成歧义就足够。我也曾经是其中一员。

但不应在这个阶段停留太久,作为代码评审人,我们应该不断提升自己对于名字的敏感度。比方说,对于前面那份代码,也许应该提出以下评审建议:
def list_environment_vars(user): # 1
    """获取环境变量列表"""
    items_user = UserVarsManager.get(user) # 2
    items_site = SiteVarsManager.get(user.site)
    return items_user + items_site
•评论 1: 项目中对于“环境变量”的统一缩写是 env_variables /env_vars,此处应保持一致,使用 list_env_variables 或 list_env_vars。
•评论 2: UserVarsManager.get 的命名可优化,因为 Manager 是一个“万金油”名词,虽然放在各种场景下都不违和,但也是以损失名字(等同于“职责”)的精确指向性为代价,此处可考虑改用一个更精确的名字,比如:UserEnvVarsRetriever.get(user);SiteVarsManager 同理。

虽然只是两处小改进,但是积少成多。每一次代码评审,必定涉及到许多新名字。但名字并非生来平等,不是所有名字都值得我们花费时间,应当尽量把关注点聚焦在那些最常被使用、最靠近用户的名字上,比如 URL 路径的资源名、数据库模型与字段名、工具函数(类方法)名,等等。

此外,与业务直接相关的领域词汇重要程度极高。评审时,每一个关键的领域词汇都值得仔细斟酌、反复推敲。举个例子,开发一个影评功能,”用户评分“、“媒体评分”、“平均分”分别该用哪些名字表示?你绝不会想要在一个文件里看到 movie_score,在另一个文件里看到 movie_rating。

命名这件小事,虽然看似不起眼,但项目规模越大、所跨越的时间维度越长,在名字质量上的细微差别就越容易累加出不可估量的巨大影响。

2. 指引性注释
夏洛已经在网上织出了光彩照人四个大字,威尔伯站在金色的阳光里,真是光彩照人。自从蜘蛛开始扶助它,它就尽力活得跟它的名声相衬。夏洛的网说它是王牌猪,威尔伯尽力让自己看上去是只王牌猪;夏洛的网说它了不起,威尔伯尽力让自己看上去了不起;现在网上说它光彩照人,它尽力让自己光彩照人。——《夏洛的网》

关于注释,我向来信奉 Bob 大叔在《代码整洁之道》[1]里的观点:“注释的恰当用法是弥补我们在用代码表达意图时遭遇的失败。” 这就是说,好代码应该总是能清晰说明自身意图,无需注释再来画蛇添足,注释只应该被用来描述那些代码之外的信息,比如解释“为什么”。

正因如此,注释总是应该被谨慎使用。假如一段代码很难理解,第一反应不应该是补注释,而是应该去追求用一种更易理解的方式重写它。但随着时间的推移,我渐渐意识到,事情不能一概而论。“指引性注释”,或者说常被人们诟病为“近乎复述代码意图”的描述性文字,也有着不可替代的重要作用。

Redis 的作者 antirez 就是“指引性注释”的忠实拥护者,他曾写过一篇文章[2]详细分析过指引性注释在 Redis 项目中的应用。下面这段代码摘自 Redis 源码,里面就有不少“指引性注释”:
    /* Call the node callback if any, and replace the node pointer
     * if the callback returns true. */
    if (it->node_cb && it->node_cb(&it->node))
        memcpy(cp,&it->node,sizeof(it->node));

    /* For "next" step, stop every time we find a key along the
     * way, since the key is lexicographically smaller compared to
     * what follows in the sub-children. */
    if (it->node->iskey) {
        it->data = raxGetData(it->node);
       // 堆代码 duidaima.com
        return 1;
    }
在这段代码中,两段注释并未提供任何在代码之外的新信息。所以,好处是什么?最直观的好处,就是这些注释让代码变得更容易理解了,它们极大地降低了人们阅读代码时所需付出的心智成本。同样一份代码,在缺少指引性注释的情况下,完全理解它的行为可能得花费 10 分钟,而有了注释的帮助,时间也许就能缩短到 5 分钟甚至更短。

当新开发者加入项目时,这些指引性注释也能助力他们更快上手。正因如此,在评审一份代码时,我常常会在一段复杂的代码逻辑上评论:“Nit:考虑增加一小段指引性注释,帮助理清代码行为。”(Nit=nitpick,表示“鸡蛋里挑骨头”式的并不强烈要求修改的意见)。

此外,如果一段代码曾在评审过程中引发过一些深度讨论,那么那些讨论内容,也许很适合被二次加工后,作为指引性注释加入代码中。对于理解代码来说,它们有时有奇效。

不过,在追求“指引性注释”的路上,也要避免踩入以下几个陷阱:
•简单复述代码:指引性注释虽然是一种帮助理解代码的辅助性文字,但绝不能只是复述代码而已,简单来说,你可以这么理解两者在传递信息方面的风格差异:代码是一本厚厚的权威科学教材,指引性注释则是一小册面向中学生的科学启蒙读物
•追求“注释率”:不要在“代码注释率”指标上设置硬性要求,指引性文字也需要讲究质量,盲目追求数量只会适得其反
•不注重时效性:过时的注释比代码危害更大,要及时修改或删除已经过期的指引性注释

总而言之,你可以把指引性注释当成有针对性的代码“教学文本”。审阅代码时,如果你发现一段逻辑理解起来很吃力,而代码本身也没有太多优化空间,请不要迟疑,勇敢表达出你对于“教学文本”的需求吧!

3. 沟通方式
“我因为鲁思和萨拉不得不离开我们而痛苦万分。而令我感觉更加痛苦的是我当时以为自己是完全孤立无援的。”
“说真的,肯顿小姐……”我端起那个我用来放使用过的瓷器的托盘。“对那样的解雇我自然是极不赞同的。我还以为那是不言自明的。”——《长日将尽》

时至今日,仍有许多人认为软件开发是一种单打独斗的工作。一位程序员捡起一块键盘,就能源源不断地产出代码,根本不需要其他人。但事实是,程序员单打独斗的黄金时代早已过去,现代软件开发已演变成一种多人参与的协作事务。正因如此,程序员的日常充斥着各类沟通工作,参与代码评审正是其中之一。

在代码评审时,评审者的工作内容似乎一句话就可简单概括:指出他人代码中的不足。 这听起来易如反掌,对不对?我曾经正是这么以为,所谓评审,只要做完下面的“123”即可:
1.找出所有可优化的点(有事说事!)
2.等提交者完成改动,或在讨论后确认维持原状(就事论事!)
3.合并代码(大功告成!)

而现实总是和理想相去甚远,代码评审很少会像上面这样顺利。因为一旦涉及到人与人之间的沟通,尤其其中一方还在给另外一方“挑毛病”,事情又能简单到哪儿去呢?

人类是一种神奇的智慧生物,阅读一段文字,不仅能从中获取到信息,更能从字里行间感受到情绪,有时,这份情绪甚至会盖过信息,影响他们做出判断。因此,当你在参与评审时,请谨记这一点:保持谦逊、尊重他人,无论对方的经验或背景如何。优秀的表达,能做到内容即使在批评,也能让对方感受到自己仍是被尊重的。

让我来举一个例子。团队内来了一位新人,用他不太熟悉的 Python 语言提交了一个 PR。作为 PR 的评审人,你在代码里发现了一段冗长的循环代码,于是写下评论:
代码比较啰嗦,建议改成列表推导式。
虽然你的观点没错,但这种表达方式值得商榷。下面是这条评论的另一种写法:

这里的循环体较简单,只有过滤和转换逻辑,很适合改成列表推导式,代码更精简。举个例子:
items = [to_item(obj) for obj in objs if obj.is_valid()]
比起第一条评论,后面这条显然更不容易引起新人的抵触情绪,更可能被采纳。这就像那句老话所说:“你表达的方式跟所要表达的内容同样重要,如果不是更重要的话。”

除了保持谦逊和尊重,还有一些其他值得采用的评审沟通小技巧:
•一例胜千言:有时用文字洋洋洒洒写一大堆,不如直接写几行代码,举一个实际的代码样例
•见什么人说什么话:对于加入团队一个月和一年的开发者,你在评审时可以(也应该)区别对待;对缺乏经验的新人,组织语言时要谨慎,尽量避免让对方感觉不被尊重,产生太多的挫败感;对熟稔的老人,语言风格就可以相对随意,言简意赅即可,不必过于啰嗦

我相信也许大部分人在心底都认同:代码评审是一个“对事不对人”的过程,不应该把对代码的批评当成对人的否定。但这和提倡“好好说话”并不冲突。一次让双方满意的沟通,几乎等同于一次更高效的沟通。所以,改善沟通方式就能提升工作效率,你又何乐而不为呢?

总结
代码评审作为保障软件质量的重要手段,是大型软件开发中不可或缺的重要一环。本文总结了我作为评审的参与者,在命名、指引性注释和沟通方式三个方面的一些思考,要点如下:
•程序员们应当不断提高在代码评审时对于命名的敏感度
•检查命名的两个技巧:同类名词保持一致、用更精确的词代替那些“万金油”名字
•对待名字不要一视同仁,多多关注那些最重要的名字
•对任何一个项目,领域(业务)相关的名字最重要,值得仔细斟酌、反复推敲
•指引性注释虽不提供太多代码之外的信息,但也有着不可替代的作用
•指引性注释能大大降低人们为理解代码所付出的心智成本
•留意指引性注释的几个陷阱:简单复述代码、追求“注释率”、不注重时效性
•评审时,要勇于对复杂的代码逻辑提出补充指引性注释的请求
•软件开发中包含许多与沟通有关的事项,代码评审正是其中之一
•理想的评审是“有事说事,就事论事”的,但正因涉及人际沟通,导致现实往往偏离理想
•文字不光能传达信息,更是情绪的载体,而情绪往往会影响沟通的效果
•在评审中,永远保持谦逊、尊重他人,无论对方的经验或背景如何
•对于有着不同经验的待评审者,应当采取不同的沟通风格

代码评审是一项涉及多人协作的复杂事务,里面藏着许许多多的学问。质量高的评审,对于提升质量和塑造团队氛围有着不可替代的作用。质量低下的评审,则可能沦落为形式主义,甚至让团队内部滋生矛盾和不满。

而影响评审质量的因素,往往藏在那些不起眼的小细节、小事情中。以上这些关于“小事情”的经验之谈,希望能对你的工作有所启发。
用户评论