前端 Code Review 指北

Posted 2021-09-08 腾讯技术工程

作者：magentaqin，腾讯 CSIG 前端开发工程师

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

PCG 代码委员会曾推出过通道晋级代码检查报告。笔者打算在这些报告基础上，从代码格式、代码错误、代码习惯、代码优化四个角度，并结合腾讯医典前端 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 = 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 < 1000; i++) {
  outer();
}

解决方案：在 outer 执行完毕时手动释放 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();
    }
  }

  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('resize', this.func);
}

5）第三方库的销毁函数，在页面卸载时也需要调用，比如 EventBus：

destroyed () {
  this.eventBus.off()
}

6）v-if 指令导致的内存泄露

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

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

<template>
  <div id="app">
    <button v-if="showChoices" @click="hide">Hide</button>
    <button v-if="!showChoices" @click="show">Show</button>
    <div v-if="showChoices">
      <select id="choices-single-default"></select>
    </div>
  </div>
</template>

<script>
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: "Item " + 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
    }
  }
})
</script>

解决办法是在 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。

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

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

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

//  else 代码块里只写了console.log
if (a) {
} else {
  console.log('something')
}

// 条件判断过于复杂，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 case
try {
  doSomething();
} catch (e) {
}

// bad case
try {
  doSomething();
} catch (e) {
  console.log(e);
}

// bad case
somePromise().then(() => {
  doSomething()
}).catch(() => {
})

somePromise().then(() => {
  doSomething()
}).catch((e) => {
  console.log(e)
})

为了解决这个问题，医典这边做了一个 eslint 插件@tencent/eslint-plugin-medical，能够检查 try catch 里的 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 硬编码1001
const isActive = this.$route.query.id === '1001'

// good case 写到配置信息中。这样，id和状态的对应关系一目了然，便于管理和维护。
const idConfig = {
  1001: STATUS.ACTIVE
}
const isActive = idConfig[this.$route.query.id] === STATUS.ACTIVE

2.7 格式校验

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

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

3. 代码习惯

3.1 if-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.2 Don't repeat yourself

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

// 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 Promise((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++里定义宏, 而且还是 #define private public 这种。从可靠性来说，多人协作很容易出现冲突。从兼容性来说，你不能保证后续推出的原生方法实现和你现有的一致，也不能保证多个库之间对该方法的实现一致。比较有名的故事是 prototype 库的 getElementsByClassName。在还没有这个原生方法之前，prototype 这个库实现的是返回 Array，并且加了“each”方法：document.getElementsByClassName('myclass').each（doSomething）；但原生方法出来后，返回的是 NodeList，并没有 each 方法；所以就悲剧了。

详细说明可以看：Nicholas C. Zakas 的 Maintainable JavaScript: Don’t modify objects you don’t own

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

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

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 < 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）类的注释

1. 类的属性注释

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。

之前遇到过某文件全局禁用"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”；而一个 Vue computed 属性，又命名为 getUserInfo；表示关闭状态，命名为“close”；真是让人非常头疼。

// bad case 1
function doctor () {}

// bad case 2
computed: {
  getUserInfo() {}
}

// bad case 3
close = false;

同学，就不能好好写代码吗？

// good case 1
function getDoctorInfo() {}

// good case 2
computed: {
  userInfo() {}
}

// good case 3
closed = false

3.13 过多的非业务逻辑相关代码(如超过 10 行的上报, 参杂在业务逻辑里)

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

<button
	data-exposurename="test-exposure"
	:data-exposureval="{event:'bottom.btn'} | jsonStringify"
  data-eventname="button.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 的细则了。

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

腾讯程序员视频号最新视频