前端CodeReview指北

优家礼品

作者：magentaqin，腾讯CSIG前端开发工程师说到CodeReview，经常有同学会问，究竟从哪些方面下手？除了一些抽象的Review原则，有没有更细化的实施准则来指导实践？PCG代码委员会曾推出过通道晋级代码检查报告。笔者打算在这些报告基础上，从代码格式、代码错误、代码习惯、代码优化四个角度，并结合腾讯医典前端CodeReview过程中遇到的一些badcase，逐一列出更细化的实施准则。希望对各位有一定的参考价值。1.代码格式代码格式问题完全可以通过自动化工具来解决。标准的eslint规则(如Airbnb或公司统一推出的eslint规则)+husky(本地pre-commit校验)+远端CI流水线eslint校验（开启cache，增量校验）就可以解决。2.代码错误2.1是否存在会导致内存泄露的代码对于SPA应用，用户无需刷新浏览器，所以要想确保垃圾回收生效，我们需要在组件对应生命周期里做主动销毁。1）存在不必要的全局变量且未及时解除引用全局变量，除非你关闭窗口或者刷新页面，才会被释放，如果缓存大量数据，很可能导致内存泄露。比如，我们之前就遇到过把IMSDK放在全局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 = null;function 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  {    doSomething();  }, 300)}参考写法，页面销毁时需清理定时器：destroyed () {  if (this.timer) {    clearTimeout(this.timer)  }}4）监听事件是否有解绑window/body等事件需要解绑：mounted() {  window.addEventListener(‘resize’, this.func)}beforeDestroy () {  window.removeEventListener('resize', this.func);}5）第三方库的销毁函数，在页面卸载时也需要调用，比如EventBus：destroyed () {  this.eventBus.off()}6）v-if指令导致的内存泄露拿vue官网避免内存泄漏的例子来看下。v-if指令只是控制虚拟DOM的添加和移除，但是由Choices.js添加的DOM片段并没有被移除。HideShow解决办法是在hide方法里调用Choices.js的API来清理DOM片段：hide: function() {  this.choicesSelect.destroy();}以下是优化前、后的JSHeap对比图：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) {    // 接口请求出错 + 接口响应成功后业务逻辑处理出错都会进入catchblock。需要进一步区分错误类型    if (error.bizcode !== 0 || error.retcode !== 0) {      reportApiError(error)    } else {      reportBusinessError(error)    }  } }2.3取值时是否进行了空判断、调用函数时是否进行了类型判断拿医典3月中下旬的错误日志来说，这类错误在错误日志中占了1/3。如果项目里已经全量使用了Typescript，这类错误应该都可以避免。但如果项目里还存在js代码，可以使用lodash.get来做空判断，在调用函数之前要对函数做类型判断。2.4存在无意义的ifelse代码块或考虑漏的条件无意义的ifelse代码块，指的不仅是空的ifelse代码块，还有只写了console.log的情况。另外，也存在条件判断过于复杂，else情况考虑不全，导致逻辑没有正常处理的情况。//  else 代码块里只写了console.logif (a) {} else {  console.log('something')}// 条件判断过于复杂，else情况考虑不全，导致逻辑不能正常处理if ((a & (b || c)) || d || (e & (f || g))) {} else {  doSomething()}解决办法：开启eslintno-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，能够检查trycatch里的catch代码块、promise的catch代码块，是否为空，是否只有console调用。当然，有些时候，并不需要对异常逻辑进行额外业务逻辑处理，catch里可以加一个上报。2.6是否含有安全风险的代码这一步可以在流水线里接入安全风险检测插件进行处理。以下是医典接入后的一个示例分析。确实存在一些误判，比如会把mixin关键字当做泄露人名来进行告警，开发人员可以对照分析是否需要处理。前端常见的硬编码场景有：请求参数和返回对象比如之前就遇到过，开发同学把mock的请求参数id，放到了线上的情况。（因为该接口是根据医生id，拉取评论，评论也是后端灌的假数据，所以在测试阶段都没发现）// 请求参数硬编码getCommentsApi({ doctorId: 1 }).then((res) => {  doSomething()}).catch(() => {  handleError();})也出现过ABtest接口，开发同学本地模拟返回对象，忘记删除硬编码的情况fetchABTestApi().then((res) => {  // res.data  const obj = {    imgSrc: 'xxx',    name: 'qinmu'  }})接口mock的硬编码，完全可以通过使用mock平台来解决。推荐使用专业的接口管理平台来进行接口管理、mock等，这里我们使用的是腾讯内部接口管理平台tolstoy。该产品还未正式开源，欢迎提前关注。路由参数// bad case 硬编码1001const isActive = this.$route.query.id === '1001'// good case 写到配置信息中。这样，id和状态的对应关系一目了然，便于管理和维护。const idConfig = {  1001: STATUS.ACTIVE}const isActive = idConfig[this.$route.query.id] === STATUS.ACTIVE2.7格式校验输入框的校验规则除了满足产品需求，比如多少字符以内、允许哪些字符，还有一个点：前后端需要校验规则保持一致。最好用统一的正则表达式，不然容易造成前端校验通过、后端校验不通过的情况。上传文件，前后端需求校验文件格式、文件大小。尤其是后端，需要对content-type为text/html的加以限制，防止出现安全问题。我们已经有过此类安全问题的工单了。3.代码习惯3.1if-else嵌套不能超过4层拒绝面条代码，减少代码中各种结构的嵌套，例如if-else、try-catch、循环等。尽量控制在三层以内，增加可读性、降低圈层复杂度。你肯定不愿意维护这样辣眼睛的代码：async getConfig(id, type = '', 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.2Don'trepeatyourself逻辑相同或相似的代码，应封装为函数进行调用。// bad case 都有展示modal的逻辑，开发同学直接复制粘贴function handleA(msg) {  wx.showModal({    title: '提示',    content: msg,    showCancel: false,    confirmText: '确定',    confirmColor: '#02BACC',    success: (res) => {      if (res.confirm) {        doA();       }     },  });}function handleB(msg) {  wx.showModal({    title: '提示',    content: msg,    showCancel: false,    confirmText: '确定',    confirmColor: '#02BACC',    success: (res) => {      if (res.confirm) {        doB();       }     },  });}function handleC(msg) {  wx.showModal({    title: '提示',    content: msg,    showCancel: false,    confirmText: '确定',    confirmColor: '#02BACC',    success: (res) => {      if (res.confirm) {        doC();       }     },  });}解决方案，封装showModal函数。function showModal (msg) {  return new romise((resolve, reject) => {    wx.showModal({      title: '提示',      content: msg,      showCancel: false,      confirmText: '确定',      confirmColor: '#02BACC',      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++里定义宏,而且还是#defineprivatepublic这种。从可靠性来说，多人协作很容易出现冲突。从兼容性来说，你不能保证后续推出的原生方法实现和你现有的一致，也不能保证多个库之间对该方法的实现一致。比较有名的故事是prototype库的getElementsByClassName。在还没有这个原生方法之前，prototype这个库实现的是返回Array，并且加了“each”方法：document.getElementsByClassName('myclass').each（doSomething）；但原生方法出来后，返回的是NodeList，并没有each方法；所以就悲剧了。详细说明可以看：NicholasC.Zakas的MaintainableJavaScripton’tmodifyobjectsyoudon’town3.4回调嵌套不建议超过3层回调嵌套减少回调的嵌套，避免产生callbackhell:fs.readdir(source, function (err, files) {  if (err) {    console.log('Error finding files: ' + err)  } else {    files.forEach(function (filename, fileIndex) {      console.log(filename)      gm(source + filename).size(function (err, values) {        if (err) {          console.log('Error identifying file size: ' + err)        } else {          console.log(filename + ' : ' + values)          aspect = (values.width / values.height)          widths.forEach(function (width, widthIndex) {            height = Math.round(width / aspect)            console.log('resizing ' + filename + 'to ' + height + 'x' + height)            this.resize(width, height).write(dest + 'w' + width + '_' + filename, function(err) {              if (err) console.log('Error writing file: ' + 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.createElement('canvas');        const tctx = tCanvas.getContext('2d');        tCanvas.width = nw;        tCanvas.height = nh;        for (let i = 0; i  {    // ...  }).catch(() => {    // ...  })}3.8避免存在大量注释掉的无用代码git的版本管理能帮我们回溯之前的代码。如果项目里存在大量注释掉的代码，会降低可读性。3.9避免遗留大量多余的console.log调试日志虽然console.log调试日志在生产环境构建时不会输出，但就本地开发环境来说，代码里惨杂过多console.log调试日志，控制台满屏的调试日志，对于每个接手的开发都是噩梦。另外，就像上面说的一样，catch处理或else分支里存在只打console.log而不做任何处理的情况。尽量避免少使用console.log，也可以减少这类意外的发生。所以，日常开发调试建议使用浏览器sourcestab的断点调试；另外，就算要输出调试日志，也不止有console.log可以使用，参考这篇文章。你可以使用console.table等来格式化输出3.10存在很多eslint-disable注释我能想到的允许eslint-disable的场景只有一种，那就是解构后端返回对象。后端返回对象属性名是下划线，这个时候可能需要//eslint-disable-next-linecamelcase。其他情况我都不建议使用eslint-disable，尤其是整个文件全局eslint-disable。之前遇到过某文件全局禁用"no-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”；而一个Vuecomputed属性，又命名为getUserInfo；表示关闭状态，命名为“close”；真是让人非常头疼。// bad case 1function doctor () {}// bad case 2computed: {  getUserInfo() {}}// bad case 3close = false;同学，就不能好好写代码吗？// good case 1function getDoctorInfo() {}// good case 2computed: {  userInfo() {}}// good case 3closed = false3.13过多的非业务逻辑相关代码(如超过10行的上报,参杂在业务逻辑里)如果在业务逻辑里掺杂太多的上报，后续理解业务逻辑时需要看上报逻辑，查上报逻辑的时候也需要理解大量的业务代码。点击埋点和曝光埋点都可以以属性的形式挂在元素上，通过冒泡，统一进行处理。如果你上报的参数需要根据不同渠道来配置，建议封装出来，不要和业务逻辑耦合了。3.14没有README文档、或者README太简单、太作用有限除了项目的READMD，每个模块都应该有各自的README，说明这个模块的功能点、技术实现方案等。看个人习惯，你也可以写在iwiki里，在README放一个iwiki的链接。3.15尽量使用export而不是exportdefault来导出exportdefault有两个问题：1）不利于treeshaking2）如果使用了一个导出对象上不存在的属性，要运行时才能发现。4.代码优化【持续更新中】4.1避免大量直接操作dom节点直接操作DOM的性能损耗至少有两个地方：进行DOM操作的时候上下文切换+DOM操作引起的页面重绘4.2避免使用deletedelete操作符并不会释放内存，而且会使得附加到对象上的hiddenclass失效，让对象变成slowobject。（hiddenclass是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的细则了。手都写麻了。当然，肯定还有很多遗漏的点，欢迎补充。腾讯程序员视频号最新视频