【译文】代码审查中我忽略的 3 件大事

代码审查| 2024-04-22

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 大叔在《代码整洁之道》里的观点：“注释的恰当用法是弥补我们在用代码表达意图时遭遇的失败。” 这就是说，好代码应该总是能清晰说明自身意图，无需注释再来画蛇添足，注释只应该被用来描述那些代码之外的信息，比如解释“为什么”。

正因如此，注释总是应该被谨慎使用。假如一段代码很难理解，第一反应不应该是补注释，而是应该去追求用一种更易理解的方式重写它。

但随着时间的推移，我渐渐意识到，事情不能一概而论。“指引性注释”，或者说常被人们诟病为“近乎复述代码意图”的描述性文字，也有着不可替代的重要作用。

Redis 的作者 antirez 就是“指引性注释”的忠实拥护者，他曾写过一篇文章详细分析过指引性注释在 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);

        return 1;
    }

在这段代码中，两段注释并未提供任何在代码之外的新信息。所以，好处是什么？

最直观的好处，就是这些注释让代码变得更容易理解了，它们极大地降低了人们阅读代码时所需付出的心智成本。同样一份代码，在缺少指引性注释的情况下，完全理解它的行为可能得花费 10 分钟，而有了注释的帮助，时间也许就能缩短到 5 分钟甚至更短。

当新开发者加入项目时，这些指引性注释也能助力他们更快上手。

正因如此，在评审一份代码时，我常常会在一段复杂的代码逻辑上评论：“Nit：考虑增加一小段指引性注释，帮助理清代码行为。”（Nit=nitpick，表示“鸡蛋里挑骨头”式的并不强烈要求修改的意见）。

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

不过，在追求“指引性注释”的路上，也要避免踩入以下几个陷阱：

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

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

3. 沟通方式

“我因为鲁思和萨拉不得不离开我们而痛苦万分。而令我感觉更加痛苦的是我当时以为自己是完全孤立无援的。”

“说真的，肯顿小姐……”我端起那个我用来放使用过的瓷器的托盘。“对那样的解雇我自然是极不赞同的。我还以为那是不言自明的。”

——《长日将尽》

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

在代码评审时，评审者的工作内容似乎一句话就可简单概括：指出他人代码中的不足。 这听起来易如反掌，对不对？我曾经正是这么以为，所谓评审，只要做完下面的“123”即可：

找出所有可优化的点（有事说事！）
等提交者完成改动，或在讨论后确认维持原状（就事论事！）
合并代码（大功告成！）

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

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

让我来举一个例子。团队内来了一位新人，用他不太熟悉的 Python 语言提交了一个 PR。作为 PR 的评审人，你在代码里发现了一段冗长的循环代码，于是写下评论：

代码比较啰嗦，建议改成列表推导式。

虽然你的观点没错，但这种表达方式值得商榷。下面是这条评论的另一种写法：

这里的循环体较简单，只有过滤和转换逻辑，很适合改成列表推导式，代码更精简。举个例子：

items = [to_item(obj) for obj in objs if obj.is_valid()]

参考： https://docs.python.org/3/tutorial/datastructures.html#list-comprehensions

比起第一条评论，后面这条显然更不容易引起新人的抵触情绪，更可能被采纳。这就像那句老话所说：“你表达的方式跟所要表达的内容同样重要，如果不是更重要的话。”

除了保持谦逊和尊重，还有一些其他值得采用的评审沟通小技巧：

一例胜千言：有时用文字洋洋洒洒写一大堆，不如直接写几行代码，举一个实际的代码样例
见什么人说什么话：对于加入团队一个月和一年的开发者，你在评审时可以（也应该）区别对待；对缺乏经验的新人，组织语言时要谨慎，尽量避免让对方感觉不被尊重，产生太多的挫败感；对熟稔的老人，语言风格就可以相对随意，言简意赅即可，不必过于啰嗦

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