前端代码评审通常注意些什么

代码评审又称为（Code Review，简称CR），关于CR，开发同学其实都不陌生，现在大部分公司的项目开发流程中，它都是必不可少的一个环节，CR的好处也都耳熟能详。不同的公司对于CR的方式、质量要求标准都不一样。这篇文章主要讲的是在代码评审的过程中，会对哪些代码和方向进行评审，给大家在接下来的CR中作为参考。

一、评审节点

很多项目一般把CR的时间节点放在上线前，如果项目越大，CR的节点越靠前越好，越靠后临近上线前，开发同学越不愿意去修改，因为测试过的代码只要出现改动，就有发生问题的风险，测试也需要重新回归，有可能会产生新的缺陷，或者将问题带到生产。下面是我们项目流程中CR的节点，一般会在联调前、提测后、上线前各进行一次代码评审。

二、评审内容

1、设计方案
设计方案一般是开发前期定好的方案，在拥有一份完美的设计方案的理想状态下，在CR阶段只需在阅读代码时判断，是否遵循设计方案进行开发即可。
一份相对完整设计方案通常会包括：

但是现实情况是，现在很多公司没有设计评审环节，项目开发前并没有进行方案设计。可能有以下几个原因：
第一：项目太小，认为没有必要。
第二：没有意识到设计方案的好处，觉得浪费时间，在方案设计的过程中，需要调研、设计、画图、出方案，少则一两天，多则三四天。在紧张的开发周期内，项目都希望越早开发完成越早上线。
第三：即使开发同学做了设计方案，也不会进行设计方案的评审。因为每次项目都需要将相关人员和评审负责人拉到一起，进行半个小时或者一个小时的方案评估。所以因为各种原因，最终设计方案都成为了需求文档的变种。

一般注释是什么时候写？
经常听到这样的回答：注释应该写于代码之前，提前捋清楚逻辑和思路，将注释写好，这样也无需反复修改，代码能更加整洁清晰。其实设计方案也像是一个超级大的注释，提前整理好开发的思路，这样也能够事半功倍。但是提前做好技术设计方案不仅仅是为了在开发的过程中更加顺利，同时也是代码评审过程中的一大助攻。

很多时候我们拿到需要评审的代码都是通过文件的顺序从上而下进行逐行阅读，这种方式往往每个文件的上下文都无法衔接，理解了第一个文件但是切换到第二个文件依然不知所云，往往需要借助注释、口述或自行查阅上下文查看代码等方式进行理解。但是通过一份完整的设计方案我们便可以了解大致的需求、开发者的设计思路，并且跟随者设计方案的结构和思路对代码的主线进行阅读。

在写了设计方案并设计方案已通过评审的项目中，在代码设计方面更多的侧重主要在于代码是否遵循设计方案进行开发，设计方案的合理性、可行性、可扩展性等在设计方案评审阶段已经通过了讨论有了结论。而对于没有设计方案或没有进行设计方案评审的项目，在理解代码的同时，也需要对整体方案的设计进行评估，而，即使方案设计并不合理，一旦经过测试面临即将上线，也没有时间和条件去进行调整。

在设计上，一般会关注下面几个方面：

• 复用：功能、组件是否可以提取公共方法？是否重复造轮子？同一代码不应该重复两次以上。
• 可扩展：当需求发生改动，是否能够使用少量的改动达到我们的目标，适应未来的变化？
• 过度设计：很多时候由于早期为了能够拥有更好的扩展性，进行了很多抽象和封装，使其复杂化，造成了过度设计。
• 依赖倒置：模块之间的依赖是否合理？一旦模块发生修改，影响面是否能得到有效的控制？

2、可维护性
可维护性这个词其实意味着很多，比如，符合团队规范，复杂度善可、可读性较高、可扩展性也还不错、结构合理，前面做的很多优化设计其实都在为之后的可维护做铺垫。
每个团队都有自己的一套编码规范，在评审的过程中需要注意是否符合团队的编码规范。但是大部分的编码规范都可以用工具进行约束，能够使用工具约束的内容，尽可能不必在评审时花时间去关注。可以将关注点放在工具约束之外的规范上，比如：

• 命名：首先格式（中划线、小驼峰、大驼峰、下划线）需要符合规范，不管是文件命名或者变量命名，尽可能符合其功能特性，能够通过命名知道它的含义，无须增加注释去特意说明。
• 注释：是否在关键代码内增加注释说明？是否符合正确的规范？
• 复杂度：复杂度是否在合理范围阈值，推荐文章前端代码质量-圈复杂度原理和实践
• 不合理的代码：每个项目都有一些难以维护的旧代码，在这个基础上继续添加代码，也许可以很快的解决当下问题，但对于日后来说，只会让它更加难以维护。及时对不合理的旧代码进行重构和优化就显得尤为重要。

3、健壮性
代码是否具备安全性和健壮性，对任何一个团队来说，无疑都是非常重要的。

• XSS：XSS攻击详细内容，推荐文章前端安全系列（一）：如何防止XSS攻击？
• CSRF：CSRF攻击详细内容，推荐文章前端安全系列（二）：如何防止CSRF攻击？
• 逻辑边界处理：是否有考虑到代码的边界逻辑？交互逻辑是否全面？
• 异常错误处理：一旦抛出异常或者错误，页面或者运行的代码是否会崩溃？
• 资源释放：定时器是否及时清空？内存及时清理？
• 兼容性：是否有浏览器版本兼容？手机机型兼容？历史数据兼容？接口兼容？小程序版本兼容？
• 数据展示：对于资产、购物车金额等关键数据的展示，尽可能直接展示后台返回数据，前端不做计算。
• 数据校验：对传输/接收的数据都进行校验、认证，确保数据的来源和正确。校验有效位、计算精度、完整性、一致性、时效性（获取时机是否正确、缓存是否更新）
• 数据转换：数据转换处理一定要经过充分的测试验证，并且尽量选取源数据进行传输，而并非转换后的数据。

4、功能范围
很多人认为功能特性的范围是测试应该去保障的，代码评审时不需要去关注开发了什么功能。但其实现状是，我们上线的代码往往有很多属于夹带私货，比如，上个迭代有一个影响不大的小Bug，趁着还没被发现，偷偷将它带上线，或者，发现上一次写的代码太蠢了，还有更好的解决思路，于是洁癖发作，默默地改了，还觉得自己棒呆了。但是测试只知道本次迭代的功能特性，除了回归主功能之外，并不知道还有其他需要重新测试的地方。如果开发同学刚好对自己的代码非常自信，觉得一定没问题，没有通知到测试回归。根据墨菲定律，这种往往觉得没有问题的代码，最后...都能够引发线上故障。

• 影响范围：底层架构、组件或者方法的修改，是否确认影响范围，每个受影响的依赖都能正常使用。
• 修改范围：是否属于本次迭代正常上线的功能范围，有没有对本次范围进行变更，是否通知到测试同学。

5、监控/埋点覆盖
添加监控的前提是公司有一套监控系统，除了定义好的异常监控场景以外。通常新增的一些关键功能、页面等也需要加入监控，提前加好监控代码，无需等到要查问题时，才记起来忘记加监控了。

• 监控：监控一般用于监控数据的异常的情况，页面的渲染异常、数据的一致性、正确性。比如：在一些关键数据的逻辑上，如果接口返回的数据与原有约定不一致，添加了监控之后，我们就能快速的响应、解决问题。不至于等到引发更多的错误之后，才能看到问题。
• 埋点：埋点一般用于统计用户操作行为的数据，大部分场景下需要埋点的数据产品经理都会提供。

6、合规
合规这个词在金融行业非常普遍，但也因为随着人们越来越注重隐私和安全，法律法规日渐完善，对合规这个词也不再陌生。如果应用不合规，就将面临被下线的风险，而有一部分不合规的内容，可能无法通过测试测出。所以在CR的过程中对合规性问题的审查，就尤为重要。任何使得用户隐私泄露的操作都需要禁止。

• 敏感信息展示：用户关键敏感信息是否直接展示。
• 敏感信息明文上报：用户关键敏感信息是否直接明文传给后台，没有做加密处理等操作。
• 敏感信息存储：保证用户信息的安全，对用户的敏感信息不存储在不安全的地方，比如web storage等。

7、性能
C端应用对前端的性能要求会比较高，代码评审时也可以关注一下比较常见的问题。前端性能优化 24 条建议（2020）

• 图片大小：图片是否有进行过压缩处理，非页面级的图片一般不要超过200kb。
• http请求：页面初始化请求过多？白屏时间过长？初始化加载数据是否在合理范围内？
• 懒加载：是否可以通过懒加载或者按需加载进行优化？
• 缓存数据：需要重复加载数据时，可以通过缓存数据减少请求。

8、tips

• checklist：在review的过程中，可以发现很多TODO List，比如增加了配置，在上线前需要先发布配置平台，比如增加片，要记得发布CDN，比如测试环境添加了测试代码，需要生产重新测试等等，所以在每次CR时，可以生成一份上线前的checklist，每次上线前查看并执行，这样能够确保不会遗漏。
• 少吃多餐：经过很多次的CR能够感觉到，每次评审的代码越多，质量就会降低，评审时间过长，都会产生疲劳感，并且一些小细节都会更容易忽略。所以每次评审的时间最好控制在30min~60min左右为佳，可发起多次评审，少吃多餐~
• 好的代码：每次CR都在像是找茬，找到不合理的地方。但其实，找到优秀合理的代码也可以促进大家的进步。大多数项目CR并不是全员参加，所以将好的代码整理出来，生成一份最佳实践，可以供大家学习参考，扩展一些新思路。

三、常见问题

前段时间听了我司一位资深CR大佬的讲座，列了很多平时在CR过程中发生的一些常见问题，我在此基础上进行了一些新增和扩展，供大家参考。

第三方库
第三方库的新增、删除、版本号的修改（包括新增小箭头），都要确认好修改范围，确保了解库升级所带来的影响。不得随意修改版本号，第三方库哪怕是小版本的升级也不能保证对当前项目或者当前依赖包没有影响，为了避免造成线上问题，最好锁死版本号。

// package.json

// before
{
     "dependencies": {
        "eslint": "7.27.0",
        "eslint-plugin-vue": "7.15.1",
    },
}
// after
{
     "dependencies": {
        "eslint-plugin-vue": "^7.15.1",
        "typescript": "^4.3.2"
    },
}

命名
命名不统一，导致阅读的成本过高，同表示数量，但是在a文件命名quantity，b文件命名amount，c文件命名count。

// a.js
const quantity = 1;

// b.js
const amount = 1;

// c.js
const count = 1;

循环引用
JavaScript 模块的循环加载
文件的相互引用，可能会导致引用报错undefined。尽量避免文件之间的相互依赖，可以使用eslint或者webpack进行约束。

// a.js
import b from b.js'

// b.js
import a from 'a.js'

复杂的判断条件
一般逻辑判断非常复杂一般前期没有想得很清楚，或者后期的维护不断的迭代，持续往上叠加，在这种情况下，逻辑会变得越来越复杂，在开发或CR时可以考虑重新梳理清楚，重点查看，进行优化。

if (!somethingA || somethingB && (!somethingC || somethingD)) {
    ...
}
or
if (...) {
    if (...) {
        ...
    } else if (...) {
        ...
    } else {
        ...
    }
} else {
    ...
}

逻辑范围变化
此问题一般出现在增量代码上，因为前面的条件判断的范围变小了，导致后面的逻辑处理的范围变大了。此时要注意这种范围的变化，是否真的符合预期，还是只想修改一处逻辑，却不小心影响到了后面的逻辑。

// before
if (type === 1) {} else {}
// after
if (type === 1 && isShow) {} else {}

异常处理
确保所有的边界逻辑都已经处理或者无需处理。

// else为空，确认无需处理
if (conditionA) {}

// 报错catch
UserService.getList().then()

// try...catch，未处理catch
try {...} catch() {}

⼤概率正确 !== 正确
前面说过的墨菲定律（墨菲定律真好用）：凡事只要有可能出错，那就一定会出错。下面setTimeout的取值方式相信很多人都见过，使用500毫秒的延迟，使偶现的问题，“不再出现”。但其实只是将出现的概率变小了，该出现的问题还是会出现。治标不治本。一定要找到出现问题的原因，真正解决它。

setTimeout(() => {
    ...延时拉取接口 or do something
}, 500)
=> 正确 !== 小概率出错

object链式取值
监控平台总是会出现很多这样的报错xxx is undefined，大部分的原因主要是因为我们在对象取值时，喜欢直接点点点。建议使用lodash的get或者?.??。

const a = productObj.something.a;
=> productObj.something是否一定有值？

// lodash
const a = get(productObj, 'something.a')
// 或者双问号操作符
?.??

隐式类型转换
隐式类型转换容易出现很多问题，==可以使用eslint避免。

if ( amount  == '22' ) {}

+new Date()

css重复样式
很多时候CR时都会忽略css，觉得它翻不出什么浪花，但是在像小程序这种对代码包的体积有严格要求的项目中，CSS的精简就显得非常重要了。很多时候在写样式时，都会根据视觉稿一股脑全部写完，但其实很多可以继承的属性，是不需要重复书写的。

.page {
    font-size: 18px;
    font-family: sans-serif;
    line-height: 18px;
    color: #000;
    ...

    .box {
        font-size: 18px;
        font-family: sans-serif;
        line-height: 18px;
        color: #000;
        ...
    }
}

每个团队对CR的要求和规范都不一样，大家可以选择适合自己的。如果还有更多建议欢迎留言补充~