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

641次阅读  |  发布于9月以前

阿里妹导读

代码评审于技术团队的工程师文化建设非常有意义，它是形成团队统一代码风格最有效的方式，作者把自己团队在一年的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报错不好解决时就有同学会选择使用它规避问题。

现场:

危害

建议

不是万不得已（通常是维护祖传代码场景），不要禁用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的格式了，比如如何表达新功能、修复、重构等信息，主要问题是描述含糊不清太过简化，无法理解变更的上下文。

现场

危害

建议

2.2 代码风格类

2.2.1 模块引入顺序混乱

翻车指数：★★★★★**

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

现场

危害

建议

最好直接使用一些工具进行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），它能带来以下优点：

2.5 不好的编程习惯

2.5.1 过度使用可选链

翻车指数：★★★★★

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

现场

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

建议

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

2.5.2 React中直接操作state

翻车指数：★★★

现场

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

建议

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

2.5.3 if条件不清晰

翻车指数：★★★★★

案例

危害

建议

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

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

翻车指数：★★★

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

案例

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

2.5.5 组件互斥属性值的使用

翻车指数：★

说明：

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

案例

建议

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

2.6 缺乏服务端思维

2.6.1 接口评审只听不评

翻车指数：★★★★★

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

案例

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

建议

提高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() 接口未择优

翻车指数：★

现场

区别

建议

当你只需要一个立即解决的Promise，并没有复杂的异步逻辑时，应该使用Promise.resolve()。而当你需要处理更复杂的异步逻辑，或者需要在异步过程中根据条件解决或拒绝Promise时，应该使用new Promise()构造函数。

2.7.3 React.createElement与jsx接口未择优

翻车指数：★

现场

区别

JSX更接近HTML语法，更易于编写和理解。相比之下createElement作为函数参数较多时代码可读性较低，需要明确指定每个元素的类型、属性和子元素，而JSX则可以很自然地嵌套并表达这些结构。建议通常推荐使用JSX。

除非一些特定场景下（不使用jsx的环境、动态创建元素）使用createElement

2.7.4 误用stopPropagation

翻车指数：★

现场

区别

stopPropagation是用来阻止整个dom事件传递过程中该节点之后的事件传递，随意使用可能会引发一些意外的bug，比如说我们经常会将点击dom任意节点关闭模态框此类的事件绑定到document，而误用了该方法则会使委托在document上的事件失效，导致点击stopPropagation的元素无法关闭模态框。建议确实需要向父元素隔离事件时才使用。

2.8 重复造轮子

翻车指数：★★★★★

说明：“重复造轮子”通常指的是在已有通用解决方案存在的情况下，开发者还是选择创建自定义的实现，团队目前主要存在以下两种情况会- 对现有轮子的不熟悉

现场

危害

建议

2.9 TS 类型定义问题

2.9.1 AnyScript风

翻车指数：★★★★★

说明：指的是 any 类型在 TypeScript 中过度使用的情况，经常类型问题难以解决了就会使出 any 这个“万金油”。

现场

危害

类型系统弱化，失去了自动补全和智能提示，在没有享受TS带来的优点的同时还徒增了很多代码建议将老项目逐渐升级到TypeScript项目的过程中使用any无可避免（也需有计划地最终去掉any），但增量代码应该尽量减少any的使用。

2.9.2 as everywhere

翻车指数：★★★★

说明：评审中发现 as 同样是很多时候同学们快速绕过类型检查的一种方式。

现场

危害

实际上是将类型错误的问题给隐藏起来了，阻止了类型推断，然后代码风险却一直存在。建议as 关键字用于类型断言，允许你告诉编译器你知道一个变量的类型比它能推断出来的更确切，如果你发现自己过于频繁地使用 as，那么这可能是你的类型设计有问题，或者你需要改进你的类型定义的信号。

2.9.3 可空? everywhere

翻车指数：★★★★

现场

危害

这是大部分过度使用可选链的一个诱因建议随意的可空? 通常代表你没有掌控或足够了解你的代码，再次建议一定要经过思考和了解你定义的字段是否可能为空，否则一定不要使?修饰 。

2.10 异常处理

异常处理时常容易被前端所忽略。

2.10.1 异常被吞没

翻车指数：★★★★

说明：常出现的情况是要么catch了异常什么都不做，或者迫于eslint的检查会写一个console.log(error)，又或者只是看似恢复了（不正确的）用户界面却丢失了案发现场，不再上报异常。

现场

危害

异常通常由我们没有考虑到的意外情况引起，从中能发现业务逻辑中不易发现的问题，一旦我们捕获了这些异常，顶层的错误监控也不能主动捕获到这些问题，程序也许没有崩溃但如果没有用户告知我们，我们就无法发现用户的哪些功能无法正常使用了。

建议写catch代码块时一定要想清楚哪些地方会引发异常，最起码要做的两件事是：

2.10.2 忽略了接口异常时的loading处理

翻车指数：★★★★

现场

危害

接口一旦失败，用户将卡在loading中无法恢复，系统可用性降低。建议自己管理loading状态时，一定要注意在finally代码块中重置loading，或者可以选择useRequest这类高度抽象的hook去管理请求。

2.10.3 使用then catch then

翻车指数：★

现场

建议

Promise链式调用中的catch，建议在错误处理完终止链

三、什么是好的习惯

这里单单还想说下工程师的信仰，当下我们的工程师们更多都在学习着谈产品谈业务，甚至会出现不为工程质量长久计的“快速”实现当下业务目标的想法，而忘了我们还要保证好的代码、好的文档、好的系统设计的时候，那一定是工程师们的失职，是追求卓越信仰的缺失，将软件工程师基本素养丢之于脑后带来的必然是一个个腐化的系统。确实很多时候我们会把忙当成自己的心理安全线去解释我们这么做的原因，可是否也该想想当不忙的时候，我们的代码离卓越还有多远。

四、写在最后

几乎没有哪个实践能有比代码评审之于技术团队的工程师文化建设更有意义，它是形成团队统一代码风格最有效的方式，持续做评审持续去改善是每个技术团队应该执行下去的事情，共勉！

Copyright© 2013-2020

All Rights Reserved 京ICP备2023019179号-8