专栏名称: 阿里开发者

阿里巴巴官方技术号，关于阿里的技术创新均将呈现于此

年度代码翻车现场｜前端代码评审问题总结

阿里开发者 · 公众号 · · 2024-03-01 08:30

正文

阿里妹导读

代码评审于技术团队的工程师文化建设非常有意义，它是形成团队统一代码风格最有效的方式，作者把自己团队在一年的CR中常见的那些小问题做了一些梳理，希望能对大家起到一点小帮助。

一、前言

团队开展线下的周代码评审已有一年有余，作为主要的评委原本以为给出代码建议是需要花很多时间和心力的事儿，总也会想着能给被评审者以巨大的帮助改善代码的设计问题，实际review下来发现大部分同学还是在一些基础的代码质量问题上，改善设计或者寻找业务bug这种目标达成的评审很少出现，因而当下我认为团队对CR也还是要有足够耐心，将基础问题当做一个个bug来解说透理解透，在走向卓越工程的路上积累。

与此同时也能很深刻的感觉到每个同学对待代码的态度是不一样的，这就产生了两个分化有人代码改善很快有人代码也不断的重复的出现评审过的问题，用心的代码是一定能看到设计的痕迹，用心的同学也一定会吸取一次次的小问题当做自己的成长点，借此年终大总结要上分的阶段，我把我们团队在一年的CR中常见的那些小问题做了一些梳理，希望能对大家起到一点小帮助。

二、翻车现场（CR中的常见问题）

2.1 代码规范类

2.1.1 使用魔法值

翻车指数： ★★★★★

说明： 魔法值表示在代码中未声明而被直接使用的值，这在我们的代码评审问题中广泛存在。

危害

代码不易读
代码不被复用
容易造成代码缺陷

建议

使用声明后的常量替代魔法值！

bad case

const now = Date.now();const lastVisitTime = localStorage.getItem('last_visit_time');
if (lastVisitTime && parseInt(lastVisitTime, 10) > 24 * 60 * 60 * 1000) {  console.log('一天前来过');}
localStorage.setItem('last_visit_time', now.toString());

good case

const LAST_VISIT_TIME_CACHE_KEY = 'last_visit_time';const DAY_IN_MS = 24 * 60 * 60 * 1000;
const now = Date.now();const lastVisitTime = localStorage.getItem(LAST_VISIT_TIME_CACHE_KEY);
if (lastVisitTime && parseInt(lastVisitTime, 10) > DAY_IN_MS) {  console.log('一天前来过');}
localStorage.setItem(LAST_VISIT_TIME_CACHE_KEY, now.toString());

2.1.2 滥用eslint-disable

翻车指数： ★★★

说明： eslint-disable 是用于在 ESLint 中禁用特定规则的指令，在遇到lint报错不好解决时就有同学会选择使用它规避问题。

现场

危害

代码质量下降：禁用规则可能会引入不推荐的代码模式，增加潜在的bug和维护难度
教育价值丢失：eslint也是一个学习工具，它可以帮助我们学习更好的编码习惯。禁用规则可能会错失学习和改进代码的机会。

建议

不是万不得已（通常是维护祖传代码场景），不要禁用lint检查。

2.1.3 使用幽灵依赖

翻车指数： ★★★

说明： 幽灵依赖（也称为隐性依赖或隐式依赖）是指在项目的某个模块中使用了未在该模块的package.json文件中直接声明的依赖。

案例

危害

不可靠的构建，可能依赖不会被安装，依赖缺失无法完成打包；
版本问题，幽灵依赖被隐式的升级或更改，项目出现突然中断或错误；
让安全风险和依赖地狱的问题难以被追踪；

建议

使用Lint工具，启用import/no-extraneous-dependencies规则来检测未声明的依赖

2.1.4 未遵循no-else-return原则

翻车指数： ★★

说明： no-else-return是一个代码质量规则，它强调了代码简洁性和可读性，用于指出当一个if块中已经包含了一个return语句后，就没有必要再使用else块。因为一旦执行了if块中的return语句，代码就会退出当前函数，所以else是多余的。

❌ You can skip the else block

function hello(condition: boolean) {  if (condition) {    return '👋';  } else {    return '👻';  }}

✅ Yay, much easier to read

function hello(condition: boolean) {  if (condition) {    return '👋';  }
  return '👻';}

2.1.5 随意的commit message

翻车指数： ★★★★

说明： 大部分时候大家我们已经会去注意commit message的格式了，比如如何表达新功能、修复、重构等信息，主要问题是描述含糊不清太过简化，无法理解变更的上下文。

现场

危害

困难的代码评审： reviewer难以理解每次提交的目的，影响评审效率
低效的问题追踪：难以追踪bug来源，影响修复进度
项目历史混乱
自动化流程受阻：工具可能无法正确解析不清晰的commit message，比如生成changelog

建议

规范团队的commit message格式规约并用lint工具做一定的保证，可以页内著名的Angular团队的“Conventional Commits”
保证信息主题的清晰、简洁，能让其他成员快速理解提交的意图和内容。

2.2 代码风格类

2.2.1 模块引入顺序混乱

翻车指数： ★★★★★

说明： 在JavaScript模块中，import 顺序随心所欲没有遵循一些最佳实践原则也在我们早期的代码评审中经常出现。

现场

危害

代码不整洁可读性差，可能导致重复导入同一个模块
css文件放在上方在想覆盖外部模块样式时会不生效

建议

第三方库优先
项目内部模块其次，同时先绝对路径再相对路径
样式文件最后

最好直接使用一些工具进行import排序，比如 prettier-plugin-sort-imports

import 案例

// 第三方库import React from 'react';import PropTypes from 'prop-types';import { connect } from 'react-redux';
// 项目内的模块import { myUtilityFunction } from '@/utils/myUtilityFunction';import { MY_CONSTANT } from '@/constants/index';import Dashboard from '@/components/Dashboard';
// 相对路径import Header from '/components/Header';
// 样式import




    
 './style.less';

2.2.2 未使用解构（destructuring）

翻车指数： ★★★

案例

建议

解构允许你用一行代码从对象或数组中提取多个属性或元素，这比逐一赋值更简洁，可以配置lint规则prefer-destructuring鼓励使用数组和对象的解构而不是通过成员表达式访问数据。

2.3 命名问题

程序员世纪大难题之一，随意命名变量、函数、类和其他代码实体会带来一系列问题，对开发效率、代码质量、团队合作和软件维护性产生负面影响。

结合线下评审发现这方面重视的同学无比重视挖空心思想取些很专业很上道的名称，也有同学认为在命名问题上纠结大可不必，毕竟一个名称不会影响代码跑出来的结果。然后混乱错误的命名必定将你的代码带向深渊，因为在代码的每一个角落不好的命名可能都会误导着下一个维护者，让维护者在不断的爆粗当中堆积屎山。大部分时候我们很难正确地命名，缺乏意愿、缺乏思考、缺乏技巧都是罪魁祸首。

以下列举我们常见的命名问题：

2.3.1 表意错误或者不明确

翻车指数： ★★★★★

说明： 这是评审时出现频次最高的问题，以至于后面几个坚持要严谨对待命名问题的同学评论的都多少有些乏力。

案例

建议

如果命名不能表达出变量、函数或类的含义，简短的命名将没有任何意义

2.3.2 大量的拼写错误

翻车指数： ★★★★

说明： 这是最不能容忍的，命个好名还讲究方法和积累的话，命名的拼写错误则是比较容易达成的事情。

案例

建议

无他，vscode插件（ Code Spell Checker ）可以告诉你哪里错了，重点是你得去改！

2.3.3 命名不符合规范

翻车指数： ★★★

说明： 命名规范是我们在代码评审的过程中慢慢完善的，这比较有效地统一了大家的命名风格。

现场

建议

有规范本身比规范是不是最佳要重要得多，统一大家的命名风格是成熟前端团队值得去做的事

2.4 函数使用

2.4.1 缺少抽象小函数（或小组件）的习惯

翻车指数： ★★★★★

说明：评审中经常会有觉得函数/组件小不愿意去抽象、或者说还没有被复用就不去抽象的情况。

案例

危害

大函数/组件容易导致代码分层不明确，不利于团队合作分工，导致更复杂的状态管理，导致代码复用变得困难。

建议

函数/组件抽象不仅可以是为了复用，也可以是为了使我们的主干逻辑或主干架构足够清晰，让代码易读易维护！

2.4.2 创建不必要的包裹函数

翻车指数： ★★★★

说明： 通常是出现创建了不必要的包裹函数的现象，导致更深的函数嵌套。

案例
危害	增加不必要的代码，提高维护和检索代码的成本如果函数发生改变，其包裹函数也要改变
建议	遵循函数为一等公民原则，包括作为参数传递给另外一个函数，从另外一个函数返回，赋值给变量，或者添加自身的属性方法... 可以像对待JS中的其他任何数据类型一样对待函数。

bad：getNotification被另外一个匿名函数包裹起来了

const { data } = useRequest(() => {  return services.UserController.getNotification();});

good：将getNotification直接传递给useRequest

// 函数被赋值给services.UserController.getNotification后，它并不只可以被执行，也可以被当做参数传递给另外一个方案const { data } = useRequest(services.UserController.getNotification);

2.4.3 把控不好的函数层级

翻车指数： ★★★★

说明： 通常是在主函数中定义很多的子函数，但大部分子函数又是不依赖主函数上下文的

现场
危害	主函数逻辑不够清晰，阅读者没法由浅入深地去了解你的程序，最终导致代码难以阅读和理解。
建议	去思考函数层级是否合理，看到长函数要有优化重构的意识，避免嵌套过深的函数。

2.4.5 函数存在副作用

翻车指数：★★★

现场

危害

以上案例将user信息挂载到了全局window对象，容易导致命名冲突变量相互覆盖等问题，同时让调用方“倍感意外”。副作用本身虽不是毒药，但不要滥用副作用。

建议

好的习惯是尽可能设计纯函数，掌握好它对于理解和使用很多框架非常有帮助（比如react），它能带来以下优点：

可测试
可缓存，提升性能，相同的输入总是对应相同的输出。比如在react中使用memo；
易组合，纯函数的返回结果仅依赖入参，这意味着函数间无耦合，方便互相组合出功能更强大以及适用更多场景的纯函数；
可并行处理

2.5 不好的编程习惯

2.5.1 过度使用可选链

翻车指数： ★★★★★

说明： 很多同学会认为使用可选链会让代码更加健壮，但大部分时候这是你对自己代码不够了解的体现。

现场

危害

过度使用?会使程序更加难以理解和调试，不知道程序内对象的一些真实是否可空的情况导致维护者写后续代码非常艰难，同时会把一些bug在开发阶段掩盖。

建议

使用可选链之前一定要经过思考和了解你取值的对象是否可能为空，否则一定不要使用可选链。

2.5.2 React中直接操作state

翻车指数： ★★★

现场

危害

破坏了React中状态的不可变性，使得状态的改变可能无法正确被追踪，状态的变化变得不可预测，甚至导致组件无法被正确渲染。

建议

始终将状态（state）视为不可变对象，所有的状态更新都应该通过setState进行（创建新的状态对象），永远不要修改现有状态对象的属性。

2.5.3 if条件不清晰

翻车指数：★★★★★

案例

危害

可合并的if分支（案例1），通常多个条件检查命中后要执行相同的逻辑块，可以合并为一个单一的表达式，提高代码可读性；
空的if代码块（案例2），空代码块通常可以被视为“死代码”，即不会被执行的代码，这增加了程序的复杂度且没有任何收益，并可能导致混淆和误解，谨记No Dead Code。

建议

尽管大部分时候我们的if条件检查在逻辑上是正确的（测试能够通过），但在写完代码后还是应该先做一遍自查，看看有没有可优化的空间，以提高代码的可读性和可维护性。

2.5.4 在末端关注不必要的上层逻辑分支

翻车指数： ★★★

说明： 通常是上层函数该处理好的逻辑分支流转到了函数/组件末端，导致逻辑分支需要成倍数地去增加。

案例

建议

可以参考初始化时分支模式的思想，一旦上层的逻辑分支没有规划好会导致越往末端的代码要做的重复判断越多，app入口、容器代码当成我们的初始化代码，做好上层的逻辑分支判断。

2.5.5 组件互斥属性值的使用

翻车指数： ★

说明： 对不熟悉的组件有时候会凭经验去试各种api，最终遗留了互斥的属性值。

案例

建议

使用公共组件还是要避免反复去“试api”的方式完成功能，没有把握的api最好是去翻找官方文档或者看组件的类型定义。

2.6 缺乏服务端思维

2.6.1 接口评审只听不评

翻车指数：★★★★★

说明：团队系分环节中的接口评审一直存在，但经常会流于形式，前端同学们容易听确不评，接口初版即终版，里面问题情况较多，下面列举几个常见的。

案例

危害

传递冗余字段（案例1），通常后端为了少几个通过唯一键去取数据的逻辑，甚至会认为这是出于接口性能考虑，会在一些写接口中设计冗余的字段，千万别掉入这个陷阱，它危害巨大：

数据一致性风险：冗余数据会带来数据同步问题，前端如果传递了不一致数据会导致后端数据错误。
耦合性增加：冗余字段增加了前后端之间的耦合性，违反了服务的独立性和单一责任原则。
增加前端复杂性：你需要组装额外的数据、管理额外状态，这增加了前端代码的复杂性。

重复定义单一数据源（案例2），通常这会导致数据一致性问题，当数据源更新需要多端发布，前后端也当共享单一的数据源，通过接口获取数据。
千奇百怪的bool值（案例3），依赖存在一致性问题，有使用"yes"、"no"的就有使用字符串"true"、"false"的，或者使用"y"、"n"的，bool类型是多数编程语言中的基本数据类型，具有明确的意义，而字符串则可以包含任意值导致类型不明确，带来额外性能开销的同时更加容易出错。

建议

提高api设计、系分参与度，带着更适合用户界面的前端设计在系分评审中多学、多听、多问，去了解服务端工作原理的同时寻找最适合前端的接口模型，提升前端在团队中解决问题的能力。

2.6.2 用展示字段去做业务逻辑的条件表达式

翻车指数： ★★★

案例

危害

带来极大的稳定性风险，展示字段有其不稳定性容易变更，比如案例1如果业务提示消息改变这个逻辑就走不通了，从而引发系统问题。

建议

业务逻辑应该基于正确的数据模型，而不是用户界面的表示，切勿将数据和表示逻辑混淆

2.7 api 使用

2.7.1 误用路由跳转

翻车指数： ★★★★

说明： 使用a标签或者window.location.href去进行路由跳转的情况。

现场
危害	导致页面重新加载，丢失应用状态，也无法做动画和过渡破坏了历史记录管理降低了代码的可维护性，一旦想调整应用的路由模式会需要修改很多分散的跳转代码
建议	建议一直使用history api或者Link组件保持SPA的良好体验。

2.7.2 new Promise()与Promise.resolve() 接口未择优

翻车指数： ★

现场

区别

return Promise.resolve() ;是 Promise API提供的一种快捷方式，用于创建一个立即解决的 Promise 。通常用于以下场景：

当你需要将一个非异步操作转换成异步操作以保持API的一致性时。
当你需要在异步函数中早期返回一个立即解决的值时。

return new Promise((resolve) => { resolve(); }); 是手动创建一个新的Promise对象，并在 Promise 构造函数中调用 resolve 方法。通常用于以下场景：

当你需要将现有的非Promise风格的异步操作（比如使用回调的旧式异步API）封装成Promise风格时。
当你的异步逻辑比较复杂，不仅仅是立即解决，而是需要执行一些操作后再解决。
当你需要根据某些条件或逻辑来决定是解决还是拒绝（reject）Promise时。

建议

当你只需要一个立即解决的

年度代码翻车现场 ｜前端代码评审问题总结

正文

2.1.1 使用魔法值

2.1.2 滥用eslint-disable

2.1.3 使用幽灵依赖

2.1.4 未遵循no-else-return原则

2.1.5 随意的commit message

2.2.1 模块引入顺序混乱

2.2.2 未使用解构（destructuring）

2.3.1 表意错误或者不明确

2.3.2 大量的拼写错误

2.3.3 命名不符合规范

2.4.1 缺少抽象小函数（或小组件）的习惯

2.4.2 创建不必要的包裹函数

2.4.3 把控不好的函数层级

2.4.5 函数存在副作用

2.5.1 过度使用可选链

2.5.2 React中直接操作state

2.5.3 if条件不清晰

2.5.4 在末端关注不必要的上层逻辑分支

2.5.5 组件互斥属性值的使用

2.6.1 接口评审只听不评

2.6.2 用展示字段去做业务逻辑的条件表达式

2.7.1 误用路由跳转

2.7.2 new Promise()与Promise.resolve() 接口未择优

请到「今天看啥」查看全文

年度代码翻车现场｜前端代码评审问题总结