首页 / 文章

预示未来将产生高昂修改成本的七大审查预警信号

学会识别审稿人会提前指出的七种代码问题,从布尔模式标志到被忽略的错误,以及如何判断每种问题是否真正构成隐患。

发布者:REACTAPP.TOP阅读约 13 分钟3031 词

来源: 7 Code Smells Senior Engineers Catch Immediately

大多数有问题的代码看起来并无异常。它们能够成功编译,通过测试,在演示中也能正常运行,但经验丰富的审查者往往能在几分钟内判断出接下来的修改、出现的故障或交接工作将会带来问题。本指南将介绍他们所关注的七种信号,解释每种信号预示什么,并说明如何区分真正的问题与仅看似可疑的模式,以便您在自身的审查工作中运用同样的判断标准。

代码异味实际上能告诉您什么

经验丰富的审查员通常无需通读整个模块就能察觉问题。在函数调用处其用途不明的布尔参数、增加了大量可选功能的工具类、将所有错误都转化为看似正常的同一值的catch语句——这些目前都不会导致任何问题。真正的问题在于它们将来可能带来的影响。

这种直觉源自经验,而非更为严格的风格指南。那些多年从事系统维护的人见过一条被重复应用的规则演变成三个相互矛盾的版本,一个原本实用的抽象层变成了处理各种特殊情况的场所,而默认的备用方案则让生产故障持续数周无法解决。他们学会识别的是那些会积累不确定性的代码部分。

应将气味视为一种提示而非定论。它并不能证明实现方式有误,只是表明该代码需要更仔细地审查:

  • 是谁或哪个模块负责做出这个决策?
  • 当需求发生变化时,这种设计还能成立吗?
  • 出现故障时仍然能够被发现吗?
  • 后续的开发者无需追溯代码的编写者,就能理解其设计原因吗?

如果不在早期提出这些问题,以下七种情况最有可能带来成本问题。

1. 一个函数承担多项无关任务

过长的函数容易受到批评,但长度其实很少是经验丰富的审查者所担心的问题。一个函数可能因为以一系列易于理解的步骤执行某项明确的操作而长达数行;另一个函数虽然只有二十行,却可能因为同时处理输入验证、权限检查、数据保存到数据库、响应格式调整、通知触发以及错误翻译等功能而难以使用。

真正的问题在于,当一个函数承担多种职责时,它就会有许多独立的修改理由。新的访问规则会影响到它,响应格式的变更也会影响它,数据库结构迁移同样会涉及它,新的通知需求更是如此。那些本应分别处理的问题最终都集中在同一个函数中,哪怕是微小的修改也迫使开发者必须了解整个功能流程。

一个有用的判断标准是那些无法准确描述函数功能的名称。比如名为processOrder的函数,实际上可能要执行客户信息核查、价格计算、库存管理、创建支付请求、保存订单记录以及发送确认邮件等操作。这样的名称之所以含糊,是因为一个准确的描述需要用一整段文字才能表达清楚。

为何机械式拆分并非解决方案

将函数拆分成更小的部分并不能自动解决问题。按行数分割代码只会导致同样的混乱分散到更多文件中,而且所有的隐藏依赖关系依然存在。更好的方法是寻找具有实际意义的边界:

  • 业务决策是在哪里做出的?
  • 哪部分是对外部系统的副作用?
  • 哪部分是纯粹的数据转换?
  • 哪部分负责处理事务?

一旦那些接口变得可见,就可以按照实际操作方式来组织代码。根本问题从来不是函数执行的任务量,而是没有人能够在不同时考虑多个无关决策的情况下更改某个单一决策。

2. 用于切换隐藏模式的布尔参数

布尔参数看起来似乎无害,因为它只包含一个比特位。但实际上,它常常会在现有函数名之下暗中引入整套不同的工作流程。请看下面的调用示例,它几乎无法让读者了解将会发生什么:

await updateAccount(accountId, true, false);

要理解它,必须打开实现代码,查看第一个 true 是会强制更新、触发通知、绕过验证、保留历史记录还是激活账户。编写该函数的人可能还记得其用途,但其他人则必须通过参数位置来推断其含义,而且只要字面量顺序调换,就能通过所有类型检查。

无人设计过的组合

随着标志位的不断增加,问题会愈发复杂。三个布尔值会产生八种可能的组合,其中很多都是没人考虑过的。如果一个函数同时接受 forceUpdate、skipValidation 和 sendNotification 参数,那么在强制更新时绕过了验证但仍向某人发送通知的情况下,应该发生什么?尽管业务逻辑上不应如此,类型系统却依然允许这样的调用。

审核人员会标记此问题,因为一组标志通常意味着一个函数正在替代多个操作。这些操作虽然可能共享内部实现,但目的并不相同。编辑联系信息、验证账户以及应用管理级覆盖都可能向同一行数据写入内容,但调用者无需记住哪种标志模式对应哪种工作流程。

选择合适的重构方式

在某些情况下,只需改用一个具有命名属性的单一参数对象即可,因为这样在调用处至少能够明白 updateAccount(id, { notify: true }) 的作用。而在其他情况下,使用诸如 verifyAccount 和 overrideAccount 这样的独立函数,能够明确表达业务意图,并彻底避免出现不合理的组合。选择哪种方式取决于这些行为是同一操作的真正变体,还是恰好共享代码的独立操作。

布尔值本身并非不好。像 includeArchived 这样命名清晰的选项是完全合理的。真正需要警惕的是那些会改变函数含义的标志,而非仅仅调整某个已明确理解的细节。

3. 以不同形式表达的同一业务规则

复制粘贴的文本很容易被识别。而重复的知识则更为危险,因为每个副本看起来可能完全不同。

以权限规则为例。它可能在用户界面中表现为一个隐藏按钮,或在控制器中进行角色验证,又或是在服务中作为状态条件出现,还可能在查询语句中以WHERE子句的形式存在。每一层似乎都在保护系统,但每层对谁有权限执行操作的理解可能略有差异。

在策略发生变化之前,一切都能正常运转。某个接口采用新规则,另一个接口仍使用旧逻辑,还有定时任务直接调用内部服务从而绕过这两者。此时界面提供的操作会被服务器拒绝。应用程序的某个部分因用户的角色而认为其已获得授权,而另一个部分则坚持要求用户必须拥有相关记录的所有权。

经验丰富的审核人员会注意到某些状态、角色、标记和日期的组合反复出现,因为这类情况往往代表着相同的业务含义被多次表述。他们会询问权威决策所在何处,以及其他层级是执行该决策还是暗中重新定义它。

具有不同用途的层级并无问题

这并不意味着某条规则只能出现在整个系统中的一行代码里。用户界面可以通过展示授权信息来提升可用性,而后端则必须通过强制措施保障安全。数据库约束可以在存储层面保护相同的不变性。这些层级承担着不同的职责。当每个层级都用自己的方式来回答业务问题时,就会出现问题。

良好的设计会为规则指定唯一的明确负责人,让其他部分负责使用、执行或展示该规则的结果。一旦责任主体不明确,后续的每一次政策变更都会变成一场寻找工作:必须找出规则被隐藏编码的所有位置,并确保没有遗漏。凡是经历过政策演变的人都知道,重复编写相关知识通常比重复编写代码成本更高。

4. 带有不断扩展的选项对象的共享辅助函数

许多棘手的抽象概念最初都是作为实用的辅助函数诞生的。当两个地方出现相似的逻辑时,就可以提取出公共部分,这样得到的代码会更简洁、目标更明确,也更容易理解。

随后,两个调用方开始提出不同的需求。一个需要重试机制;另一个则绝不能重复发送请求。一个能从缓存中获益;另一个则绝不能看到过时的数据。一个会返回完整的负载数据;另一个则是以流的形式传输数据。而非将这些工作流程分开处理,辅助工具会为每一处差异新增一个选项。

不久之后,选项对象就涵盖了错误转换、结果格式调整、缓存设置、重试次数、身份验证处理、钩子函数以及几组布尔开关。大多数调用方只会使用其中两三个选项,但任何改动都可能影响到所有选项。

出现这种状况是因为抽象层不再消除复杂性,反而开始将其集中起来。调用接口看起来很简单,但要理解整个系统就需要知道每个选项是如何与其他所有选项相互作用的。调用次数众多并不能说明抽象层是合理的;有时这只是表明该辅助功能已经变得无法避免。

相似的语法与共享的行为

关键问题在于这些调用方是具有稳定的行为,还是仅仅拥有外观相似的代码。两个操作可能都会发送HTTP请求,但对失败和重试的处理方式却截然不同。一对表单可能有相同的字段,但在验证、提交以及草稿处理方面存在差异。将它们合并虽然消除了表面上的重复,却使得所有实际的区别更难表达出来。

通常更有效的做法是稍后引入更具体的抽象层。不必共享整个工作流程,只需共享某个稳定的部分,比如某一次转换、某项策略或某个低级调用,同时明确区分不同的工作流程。重用依然很有价值;值得关注的是那种新需求不断增加现有调用者既不需要也不理解的选项的模式。如需从前端角度了解同一问题,可参阅为什么前端抽象会悄悄变成技术债务。

5. 将失败伪装成成功的错误处理方式

catch块能让应用程序看起来很可靠,但实际上却在悄悄削弱人们对它的信任。

想象这样一个数据访问层:当遇到数据库错误时,它会返回 []。又或者某个服务在发起 HTTP 请求失败后直接返回 null。前端会将所有被拒绝的请求映射为空状态。虽然不会发生崩溃,但系统再也无法区分是真正没有数据还是处理失败了。

审查人员能很快发现这个问题,因为他们知道在故障发生时会出现什么情况:数据库宕机时仪表板会显示零条记录;客户会因为在会话无声过期后看到“未找到订单”;由于依赖项返回了无用数据,后台任务会将某个项目标记为已完成。界面依然保持平静,因为它用看似合理的答案替代了真实情况。

针对每个异常处理块需要提出的问题

在审查错误处理机制时,需检查以下三点:

  • 原始原因是否被保留下来,例如作为新错误的cause字段,或至少记录在日志中?
  • 调用方能否区分预期的业务结果与技术故障?
  • 捕获错误的那一层是否真正拥有后续决策权?

如果该层的代码无法恢复、重新尝试、撤销部分操作或提供合理的替代方案,那么捕获错误除了破坏证据外可能毫无作用。

以上内容并不意味着原始异常就应该直接呈现给用户。展示给用户的消息应当安全且易于理解,而内部错误往往需要转化为稳定的领域类别。其要求是这种转换必须保留足够的信息,以便应用程序和开发人员能够做出正确响应。

空结果应表示操作成功但未找到任何内容。失败的操作应当保持为失败状态,除非系统已明确定义了安全的备用方案。为方便起见将两者混为一谈会降低调试效率,并向用户展示错误信息。良好的错误处理机制会暴露故障,而糟糕的错误处理机制则会隐藏它。

6. 要求简单但涉及大量无关文件

经验丰富的审查者通常会在阅读具体代码行之前先查看拉取请求的整体结构。当一个简单的需求分散在大量无关的文件中时,这往往表明存在隐藏的耦合关系,或者该任务已经偏离了最初的目标。

添加一个字段就可能会涉及到持久化存储、服务层、API响应以及用户界面;这是一种贯穿各层的连贯变更。而当同一个拉取请求还同时修改共享工具函数、重命名无关类型、更改错误处理方式、移动模块位置以及更新大型快照文件时,问题就出现了。

这些修改中的每一项都可能带来改进,但合在一起却将多种变更原因捆绑在同一个请求中。审查者必须同时评估新功能、代码重构、接口变化以及清理工作。如果出现了回归问题,要找出具体的责任修改就会更加困难。而一旦需要撤销新功能,那些无关的改进也会一同被取消。

为何人工智能辅助编码会让这种情况更常见

代码生成工具使得人们更容易陷入这种模式,因为完全重写代码几乎不耗费任何成本。只要请求修复一个漏洞,就可能得到一个经过精心设计的替代函数,它拥有新的名称、不同的控制流程、全新的抽象结构以及改变后的错误处理方式。虽然创建这样的函数几乎不花钱,但团队仍需像以往一样投入相同的精力来验证它。

一个好的原则是,让修改的规模与其可解释的清晰度保持比例。针对具体的问题通常应采用相应的解决方案,而更复杂的优化则可以放在单独的拉取请求中,以便对其本身进行评估和测试。这并非要求所有修改都必须通过微小的拉取请求来实现;某些架构层面的工作本身就需要较大的改动范围。关键在于每项有意义的修改是否都能带来一致的结果。当代码差异超过问题本身的规模时,往往意味着多个问题被混在一起处理,从而导致因果关系难以追溯。

7. 能运行但难以验证的代码

有些实现之所以能快速编写,恰恰是因为其行为难以观察或控制。这样的函数会直接读取系统时钟、自行构建依赖关系、修改全局状态、发起多次外部调用,同时吞掉自身产生的错误且几乎不返回任何信息。在正常情况下它或许还能正常工作,但要证明其失败情形却十分困难。

可测试性往往反映了边界划分的质量。如果某条业务规则必须启动整个应用程序、初始化数据库记录、调用真实服务并等待实际计时器,那么这条规则很可能与过多的副作用纠缠在一起。

异步工作流暴露了这一缺陷

在多步骤异步流程中,这个问题最为突出。想象这样一个函数:它保存更改、发出域事件、通知用户,并添加一条审计日志记录。如果第三步失败,测试能否判断哪些步骤已经完成?能否强制让某个依赖项超时?能否模拟该函数运行两次?能否在不依赖运气因素的情况下重现过时的更新结果?

那些编写容易但验证困难的代码会让人们过度依赖手动测试。有人只需沿着成功的路径操作,看到正确的结果就认为功能已经完成。而实际生产环境中则会出现并发问题、重试操作、异常数据以及那些在本地测试时从未出现过的未完成操作。

在风险存在的层面进行验证

处处追求纯粹以及对一切事物进行Mock测试也都不是解决办法。主要依赖Mock测试的方案往往会使内部细节固定不变,而忽视了实际数据库、网络或事务的真实运行情况。更好的方法是确定哪些保障措施至关重要,然后在这些措施存在风险的地方进行逐一验证:

  • 纯粹的转换逻辑可以单独进行单元测试。
  • 数据库的不变性通常需要通过真实数据库来进行集成测试。
  • 服务边界可能需要通过契约测试来验证。
  • 分布式系统的行为可能依赖于结构化的日志和关联ID,因为并非所有的生产环境条件都能被精确复现。

通过注入时钟或依赖关系而非使用全局变量,往往是让关键决策变得可控的最小改动。这种问题并非单纯源于代码覆盖率低,而是指在正常执行路径不再有效时,那些关键决策无法被控制、观察或解释的代码。

核心要点

这些信号并非自动的判定依据。一个较长的函数可能代表单一明确的操作,一个布尔选项可能显而易见,一个共享的辅助函数可能确实能体现稳定的行为,而某些大规模改动也可能不可避免。应将这份清单视为指导深入分析方向的工具,而非需要机械照做的检查表。

在审查代码时,应思考以下问题:

  • 职责是否混杂,导致一次改动会牵连到无关的决策?
  • 调用者无需解读位置标志就能理解该函数的功能吗?
  • 每条业务规则是否有明确的负责人?
  • 那些共享的抽象层是否掩盖了选项背后的实际差异?
  • 错误处理能否准确反映出究竟是什么出了问题?
  • 差异程度是否与其所满足的需求相称?
  • 当出现问题时,能否验证那些关键的行为是否正常?
  • 经验丰富的审查人员真正察觉到的是潜在的不确定性:下一个需求可能会迫使某人去理解与之无关的逻辑,下一次故障可能留下过少的证据供调查,下一位开发者可能无法判断某种行为是刻意为之,或者下一个调用者可能会在已经负担过重的抽象层上再添加新的功能。他们能迅速识别出这些问题,因为他们早已为处理这些问题付出过代价——比如梳理过度复杂的辅助函数、追踪被吞没的异常、协调相互矛盾的规则,以及审查那些却影响了整个代码库一半内容的“小”功能。其目的并非消除外观上杂乱的代码,而是找出那些会让未来工作变得不必要的复杂的代码。