万字长文教你如何做前端Code Review

万字长文教你如何做前端Code Review

2021-08-12 18:00·

作者:magentaqin,腾讯前端开发工程师

说到 Code Review,经常有同学会问,究竟从哪些方面下手?除了一些抽象的 Review 原则,有没有更细化的实施准则来指导实践?

PCG 代码委员会曾推出过通道晋级代码检查报告(www.qwww.com.cn)。笔者打算在这些报告基础上,从代码格式、代码错误、代码习惯、代码优化四个角度,并结合腾讯医典前端 Code Review 过程中遇到的一些 bad case,逐一列出更细化的实施准则。希望对各位有一定的参考价值。

1. 代码格式

代码格式问题完全可以通过自动化工具来解决。 标准的 eslint 规则( 如Airbnb或公司统一推出的eslint规则) + husky( 本地pre-commit校验 ) + 远端 CI 流水线 eslint 校验(开启cache,增量校验)就可以解决。

2. 代码错误

2.1 是否存在会导致内存泄露的代码

对于 SPA 应用,用户无需刷新浏览器,所以要想确保垃圾回收生效,我们需要在组件对应生命周期里做主动销毁。

1)存在不必要的全局变量且未及时解除引用

全局变量,除非你关闭窗口或者刷新页面,才会被释放,如果缓存大量数据,很可能导致内存泄露。 比如,我们之前就遇到过把 IM SDK放在全局window上,但在页面卸载时却没有解除引用。

mounted { window.im = TWebLive.createIM({ SDKAppID })}

解决方案:在页面卸载时解除该全局引用。

destroyed { window.im = null}

其实该 im 实例也不需要挂在window上,直接绑定在vue实例上即可,组件销毁时该实例也会销毁;但没有绑定在vue实例上的一定要主动销毁。

2)闭包内部变量未被销毁

来看一个容易忽视的闭包引发内存泄漏的例子。 outer函数内部定义了两个函数: unused 和 foo。虽然inner函数中并没有使用outer函数中的变量,但是由于unsed函数使用了outer函数的bar变量,bar也不会被释放,所以foo相当于隐式持有了bar。每次执行outer,bar都会指向上一次的foo;而foo也会隐式持有bar,这样的引用关系导致bar和foo都无法释放。

let foo = nullfunction outer { let bar = foo // 该函数历史原因,调用方被注释掉。并无调用 function unused { doSomething console.log(`unused ${bar}`) } // foo赋值 foo = { bigData: new Array(10000), inner: function { doSomething } }}for (let i = 0 i < 1000 i++) { outer}

解决方案:在 outer 执行完毕时手动释放bar。这样,隐式持有 bar 的 foo 也没有其他变量引用,也会被回收了。

let foo = nullfunction outer { let bar = foo // 该函数历史原因,调用方被注释掉。并无调用 function unused { doSomething console.log(`unused ${bar}`) } // foo赋值 foo = { bigData: new Array(10000), inner: function { doSomething } } bar = null // 手动释放bar}for (let i = 0 i < 1000 i++) { outer} 3)定时器是否及时清理

常见的情况是mounted设置了定时任务,但却没有及时清理。

mounted { this.timer = setTimeout( => { doSomething }, 300)}

参考写法,页面销毁时需清理定时器:

destroyed { if (this.timer) { clearTimeout(this.timer) }} 4)监听事件是否有解绑

window/body 等事件需要解绑:

mounted { window.addEventListener(‘resize’, this.func)}beforeDestroy { window.removeEventListener(&aposresize&apos, this.func)} 5)第三方库的销毁函数,在页面卸载时也需要调用,比如EventBus: destroyed { this.eventBus.off} 6)v-if 指令导致的内存泄露

拿 vue 官网避免内存泄漏 的例子来看下。

v-if 指令只是控制虚拟DOM的添加和移除,但是由 Choices.js 添加的 DOM 片段并没有被移除。

&lttemplate> &ltdiv id=&quotapp"> &ltbutton v-if=&quotshowChoices" @click=&quothide"&gtHide</button> &ltbutton v-if="!showChoices" @click=&quotshow"&gtShow</button> &ltdiv v-if=&quotshowChoices"> &ltselect id=&quotchoices-single-default"></select> </div> </div></template>&lt>new Vue({ el: "#app", data: function { return { showChoices: true } }, mounted: function { this.initializeChoices }, methods: { initializeChoices: function { let list = [] for (let i = 0 i < 1000 i++) { list.push({ label: &quotItem " + i, value: i }) } new Choices("#choices-single-default", { searchEnabled: true, removeItemButton: true, choices: list }) }, show: function { this.showChoices = true this.$nextTick( => { this.initializeChoices }) }, hide: function { this.showChoices = false } }})</>

解决办法是在hide方法里调用 Choices.js 的API来清理DOM片段:

hide: function { this.choicesSelect.destroy}

以下是优化前、后的 JS Heap 对比图:

2.2 异步操作是否有异常处理

异步操作拿接口请求来说,大家都知道的是,使用 promise 时要有.catch处理。但使用 async/await 时,有 .catch处理的,也有try...catch处理的使用方法。这里推荐使用 .catch。原因在于:

  • 可以控制接口请求出错后,是否要阻塞后续业务逻辑执行
  • .catch里的 error 能明确知道是接口请求导致的错误,而 不需要再对 error 进行分类判断,是接口200返回后的业务逻辑处理报错还是接口报错。
// CASE 1: 接口报错,阻塞业务逻辑执行async fetchList { const res = await someApi.catch(error => { // error处理逻辑 }) if (res) { doA } } // CASE 2: 接口报错,不阻塞业务逻辑执行 async fetchList { const res = await someApi.catch(error => { // error处理逻辑 }) doA } // CASE 3:使用try...catch的情况 async fetchList { try { const res = await someApi doA } catch (error) { // 接口请求出错 + 接口响应成功后业务逻辑处理出错都会进入catch block。需要进一步区分错误类型 if (error.bizcode !== 0 || error.retcode !== 0) { reportApiError(error) } else { reportBusinessError(error) } } } 2.3 取值时是否进行了空判断、调用函数时是否进行了类型判断**

拿医典3月中下旬的错误日志来说,这类错误在错误日志中占了1/3。

如果项目里已经全量使用了Type,这类错误应该都可以避免。 但如果项目里还存在 js 代码,可以使用lodash.get来做空判断,在调用函数之前要对函数做类型判断。

2.4 存在无意义的 if else代码块或考虑漏的条件

无意义的if else代码块,指的不仅是空的if else 代码块,还有只写了console.log 的情况。 另外,也存在条件判断过于复杂,else情况考虑不全,导致逻辑没有正常处理的情况。

// else 代码块里只写了console.log if (a) {} else { console.log(&apossomething&apos)}// 条件判断过于复杂,else情况考虑不全,导致逻辑不能正常处理if ((a && (b || c)) || d || (e && (f || g))) {} else { doSomething}

解决办法: 开启eslint no-empty 规则,不允许有空的block。但这个插件的问题在于,如果是无效的代码块,比如在else代码块只做了console.log操作,并不会检测出来。 另外,较为复杂的条件判断尽量拆成单独的变量,并分别配上注释说明,这样可以防止逻辑处理漏。

2.5 存在无意义的 catch 代码块

和无意义的 else 代码块一样,也存在空catch代码块、只有console.log的catch代码块的情况。

// bad casetry { doSomething} catch (e) {}// bad casetry { doSomething} catch (e) { console.log(e)}// bad casesomePromise.then( => { doSomething}).catch( => {})somePromise.then( => { doSomething}).catch((e) => { console.log(e)})

为了解决这个问题,医典这边做了一个eslint插件@tencent/eslint-plugin-medical,能够检查try catch里的catch代码块、promise 的catch代码块,是否为空,是否只有console调用。cuow当然,有些时候,并不需要对异常逻辑进行额外业务逻辑处理,catch里可以加一个上报。

2.6 是否含有安全风险的代码

这一步可以在流水线里接入安全风险检测插件进行处理。以下是医典接入后的一个示例分析

确实存在一些误判,比如会把mixin关键字当做泄露人名来进行告警,开发人员可以对照分析是否需要处理。

An

前端常见的硬编码场景有:

  • 请求参数和返回对象 比如之前就遇到过,开发同学把mock的请求参数id,放到了线上的情况。(因为该接口是根据医生id,拉取评论,评论也是后端灌的假数据,所以在测试阶段都没发现)
// 请求参数硬编码getCommentsApi({ doctorId: 1 }).then((res) => { doSomething}).catch( => { handleError})

也出现过ABtest接口,开发同学本地模拟返回对象,忘记删除硬编码的情况

fetchABTestApi.then((res) => { // res.data const obj = { imgSrc: &aposxxx&apos, name: &aposqinmu&apos }})

接口mock的硬编码,完全可以通过使用mock平台来解决。推荐使用专业的接口管理平台来进行接口管理、mock等,这里我们使用的是腾讯内部接口管理平台tolstoy。该产品还未正式开源,欢迎提前关注。

  • 路由参数
// bad case 硬编码1001const isActive = this.$route.query.id === &apos1001&apos// good case 写到配置信息中。这样,id和状态的对应关系一目了然,便于管理和维护。const idConfig = { 1001: STATUS.ACTIVE}const isActive = idConfig[this.$route.query.id] === STATUS.ACTIVE 2.8 格式校验

输入框的校验规则除了满足产品需求,比如多少字符以内、允许哪些字符,还有一个点:前后端需要校验规则保持一致。最好用统一的正则表达式,不然容易造成前端校验通过、后端校验不通过的情况。

上传文件,前后端需求校验文件格式、文件大小。尤其是后端,需要对 content-type 为 text/html 的加以限制,防止出现安全问题。我们已经有过此类安全问题的工单了。

3. 代码习惯

3.1 if-else嵌套不能超过4层

拒绝面条代码,减少代码中各种结构的嵌套,例如 if-else、try-catch、循环等。尽量控制在三层以内,增加可读性、降低圈层复杂度。 你肯定不愿意维护这样辣眼睛的代码:

async getConfig(id, type = &apos&apos, isReset = false) { try { if (liveid) { const res = await someApi(id) if (res && res.info) { const { status } = res.info status === 2 && doA({id, type}) if (status === 1 || status === 2 || status === 4) { this.setData({ info: res.info, status, }) if (isReset) { this.setData({ current: 0, index: 0 }) status === 2 && doB } return { code: 0} } return doC } else if (isReset) { resetSomething } else if (isReset && type === someType) { handleType } return wx.showModal({ //... }) } } catch (error) { if (error.code === 1001) { reportA } else { reportB doD } }} 3.2 Don&apost repeat yourself

逻辑相同或相似的代码,应封装为函数进行调用。

// bad case 都有展示modal的逻辑,开发同学直接复制粘贴function handleA(msg) { wx.showModal({ title: &apos提示&apos, content: msg, showCancel: false, confirmText: &apos确定&apos, confirmColor: &apos#02BACC&apos, success: (res) => { if (res.confirm) { doA } }, })}function handleB(msg) { wx.showModal({ title: &apos提示&apos, content: msg, showCancel: false, confirmText: &apos确定&apos, confirmColor: &apos#02BACC&apos, success: (res) => { if (res.confirm) { doB } }, })}function handleC(msg) { wx.showModal({ title: &apos提示&apos, content: msg, showCancel: false, confirmText: &apos确定&apos, confirmColor: &apos#02BACC&apos, success: (res) => { if (res.confirm) { doC } }, })}

解决方案,封装showModal函数。

function showModal (msg) { return new Promise((resolve, reject) => { wx.showModal({ title: &apos提示&apos, content: msg, showCancel: false, confirmText: &apos确定&apos, confirmColor: &apos#02BACC&apos, success: (res) => { if (res.confirm) resolve }, fail: (err) => { reject(err) } }) })}funtion handleA(msg) { showModal(msg).then( doA ).catch( => { catchHandler})}funtion handleB(msg) { showModal(msg).then( doB ).catch( => { catchHandler})}funtion handleC(msg) { showModal(msg).then( doC ).catch( => { catchHandler})} 3.3 不建议直接修改Object原型(或者Function, Array原型等)

在Object.prototype上定义方法就相当于C++里定义宏, 而且还是 #define private public 这种。 从可靠性来说,多人协作很容易出现冲突。 从兼容性来说,你不能保证后续推出的原生方法实现和你现有的一致,也不能保证多个库之间对该方法的实现一致。比较有名的故事是 prototype库的getElementsByClassName。在还没有这个原生方法之前,prototype这个库实现的是返回Array,并且加了“each”方法:document.getElementsByClassName(&aposmyclass&apos).each(doSomething);但原生方法出来后,返回的是NodeList,并没有each方法;所以就悲剧了。 详细说明可以看:Nicholas C. Zakas 的 Maintainable Java: Don’t modify objects you don’t own

3.4 回调嵌套不建议超过3层回调嵌套

减少回调的嵌套,避免产生callback hell:

fs.readdir(source, function (err, files) { if (err) { console.log(&aposError finding files: &apos + err) } else { files.forEach(function (filename, fileIndex) { console.log(filename) gm(source + filename).size(function (err, values) { if (err) { console.log(&aposError identifying file size: &apos + err) } else { console.log(filename + &apos : &apos + values) aspect = (values.width / values.height) widths.forEach(function (width, widthIndex) { height = Math.round(width / aspect) console.log(&aposresizing &apos + filename + &aposto &apos + height + &aposx&apos + height) this.resize(width, height).write(dest + &aposw&apos + width + &apos_&apos + filename, function(err) { if (err) console.log(&aposError writing file: &apos + err) }) }.bind(this)) } }) }) }})

建议使用promise、async/await 的方式让代码更为清晰可读;也可以将callback要做的事拆成独立的function,并分别对err进行处理。

3.5 函数不超过80行

函数尽量精简在80行以内,并且以小function进行组织,方便维护、复用。

3.6 缺少注释及注释规范化

除了知道下面的逻辑是在绘制canvas,其他逻辑你能看懂吗?

function doSomething { let count if ((count = width * height / 1000000) > 1) { count = ~~(Math.sqrt(count) + 1) const nw = ~~(width / count) const nh = ~~(height / count) const tCanvas = document.(&aposcanvas&apos) const tctx = tCanvas.getContext(&apos2d&apos) tCanvas.width = nw tCanvas.height = nh for (let i = 0 i < count i++) { for (let j = 0 j < count j++) { tctx.drawImage(img, i * nw * ratio, j * nh * ratio, nw * ratio, nh * ratio, 0, 0, nw, nh) ctx.drawImage(tCanvas, i * nw, j * nh, nw, nh) } } } else { ctx.drawImage(img, 0, 0, width, height) }}

常用的注释分类有这些,建议参考 JSDoc:

1)文件注释

2)变量注释

3)常量注释

4)函数注释

5)枚举注释

6)类的注释

7)类的属性注释

3.7 注释与实现功能不符

这一条在团队里是出现过现网bug的。

故事背景是开发M在重构代码时,设置底部栏状态这一逻辑已经封装出来,所以根据注释,下面几行代码做的事情也是设置底部栏状态,开发M就把这几行代码都删掉了。但是注释下面的代码,除了做设置底部栏状态的事情,还有一个setBanner 函数,是为了设置banner位的,也被连同删掉,进而导致了bug。

追溯原因,设置底部栏状态是A同学做的,设置 banner 位是B同学做的,B同学在没有看注释的情况下,直接把setBanner放在了错误的位置上。

function fetchData { // ... doManythings // 设置底部栏状态 setBanner fetchStatusApi.then(res => { // ... }).catch( => { // ... })} 3.8 避免存在大量注释掉的无用代码

git的版本管理能帮我们回溯之前的代码。如果项目里存在大量注释掉的代码,会降低可读性。

3.9 避免遗留大量多余的console.log调试日志

虽然console.log调试日志在生产环境构建时不会输出,但就本地开发环境来说,代码里惨杂过多console.log调试日志,控制台满屏的调试日志,对于每个接手的开发都是噩梦。另外,就像上面说的一样,catch处理或else分支里存在只打console.log而不做任何处理的情况。尽量避免少使用console.log,也可以减少这类意外的发生。

所以,日常开发调试建议使用浏览器sources tab的断点调试;另外,就算要输出调试日志,也不止有console.log可以使用,参考这篇文章。你可以使用console.table等来格式化输出

3.10 存在很多eslint-disable注释

我能想到的允许 eslint-disable的场景只有一种,那就是解构后端返回对象。后端返回对象属性名是下划线,这个时候可能需要 // eslint-disable-next-line camelcase。其他情况我都不建议使用 eslint-disable,尤其是整个文件全局eslint-disable。

之前遇到过某文件全局禁用&quotno-undef"规则,结果代码里使用了未定义的变量,导致现网bug。如果你有全局定义的变量,建议写在eslintrc.js的globals字段里。当然,就正如上文代码错误-内存泄露提到的一样,非必要情况,不建议使用全局变量。

3.11 没有使用空行对代码分组

为了增强可读性,建议使用空行对代码分组。

3.12 命名规范

常见的不规范命名有这些,会让之后维护的同学很懵逼:

  • 单词拼写错误,比如submitForm,写成submitFrom。
  • 中英文混用。比如gotoZaihai。你能知道这是什么意思吗?其实是跳到灾害专区活动页。goToDisasterZone是不是要好一点,同学?
  • 以1-9、a-z命名 在项目里我曾经见过不知道怎么命名,就type1、type2、type3直接上了,也不写注释。非常让人抓狂。
  • 混用命名格式 就评论列表,代码里有comments、commentList、也有commentData、commentsData。???能规范统一一下吗。
  • 单复数不分 明明是一个列表数据,非要用单数表示,比如 disease。建议区分下单复数,如果是数组就用List来表示。
  • 动词、名词、形容词不分 比如,一个函数名,命名为名词“doctor”;而一个Vue computed属性,又命名为getUserInfo;表示关闭状态,命名为“close”;真是让人非常头疼。
// bad case 1function doctor {} // bad case 2computed: { getUserInfo {} }// bad case 3close = false

同学,就不能好好写代码吗?

// good case 1function getDoctorInfo {}// good case 2computed: { getUserInfo {} }// good case 3closed = false 3.13 过多的非业务逻辑相关代码(如超过10行的上报, 参杂在业务逻辑里)

如果在业务逻辑里掺杂太多的上报,后续理解业务逻辑时需要看上报逻辑,查上报逻辑的时候也需要理解大量的业务代码。 点击埋点和曝光埋点都可以以属性的形式挂在元素上,通过冒泡,统一进行处理。

&ltbutton data-exposurename=&quottest-exposure" :data-exposureval="{event:&aposbottom.btn&apos} | jsonStringify" data-eventname=&quotbutton.click" :data-eventval="{id: buttonId} | jsonStringify"/>

如果你上报的参数需要根据不同渠道来配置,建议封装出来,不要和业务逻辑耦合了。

3.14 没有README文档、或者README太简单、太作用有限

除了项目的READMD,每个模块都应该有各自的README,说明这个模块的功能点、技术实现方案等。看个人习惯,你也可以写在iwiki里,在README放一个iwiki的链接。

3.15 尽量使用export 而 不是 export default来导出

export default 有两个问题: 1)不利于tree shaking 2)如果使用了一个导出对象上不存在的属性,要运行时才能发现。

4.代码优化【持续更新中】

4.1 避免大量直接操作dom节点

直接操作DOM的性能损耗至少有两个地方:进行DOM操作的时候上下文切换 + DOM操作引起的页面重绘

4.2 避免使用delete

delete 操作符并不会释放内存,而且会使得附加到对象上的hidden class失效,让对象变成slow object。 (hidden class是V8为了优化属性访问时间而创建的隐藏类) 来看一下执行速度对比:undefined> delete> omit

4.3 是否引用了不必要的npm包

比如做一个简单图表的需求,不选轻量的库,非要整一个echarts;或者实现一个简单的代码编辑器,monaco-editor有min版本不使用,非要引用一整个monaco-editor。还有,lodash没法做tree-shaking,要么引用一个具体的子包lodash.get,要么引用 lodash-es,非要引用一整个lodash。

4.4 尽量使用CDN地址的图片

如果代码里引用的是本地图片,构建打包会有耗时。可以在引用之前就把图片传到cdn上,代码里直接使用cdn地址。

以上就是CR的细则了。

手都写麻了。当然,肯定还有很多遗漏的点,欢迎补充。

公司名称:泊头市兴和机械有限公司
主营产品:复合机,型钢机,方管机,承重板机