前端 Code Review 指北

发表于 3年以前 | 总阅读数：349 次

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

类的属性注释

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。