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

说到 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 片段并没有被移除。

解决办法是在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调用。cuow当然，有些时候，并不需要对异常逻辑进行额外业务逻辑处理，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.8 格式校验

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

上传文件，前后端需求校验文件格式、文件大小。尤其是后端，需要对 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）类的注释

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。

之前遇到过某文件全局禁用"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: {
  getUserInfo() {}   
}

// good case 3
closed = false

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

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

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

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的细则了。

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

思凡资源网

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

1. 代码格式

2. 代码错误

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

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

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

3）定时器是否及时清理

4）监听事件是否有解绑

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

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

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

2.3 取值时是否进行了空判断、调用函数时是否进行了类型判断**

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

2.5 存在无意义的 catch 代码块

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

2.8 格式校验

3. 代码习惯

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

3.2 Don't repeat yourself

3.3 不建议直接修改Object原型(或者Function, Array原型等)

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

3.5 函数不超过80行

3.6 缺少注释及注释规范化

3.7 注释与实现功能不符

3.8 避免存在大量注释掉的无用代码

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

3.10 存在很多eslint-disable注释

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

3.12 命名规范

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

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

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

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

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

4.2 避免使用delete

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

4.4 尽量使用CDN地址的图片

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

1. 代码格式

2. 代码错误

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

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

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

3）定时器是否及时清理

4）监听事件是否有解绑

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

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

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

2.3 取值时是否进行了空判断、调用函数时是否进行了类型判断**

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

2.5 存在无意义的 catch 代码块

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

2.8 格式校验

3. 代码习惯

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

3.2 Don't repeat yourself

3.3 不建议直接修改Object原型(或者Function, Array原型等)

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

3.5 函数不超过80行

3.6 缺少注释及注释规范化

3.7 注释与实现功能不符

3.8 避免存在大量注释掉的无用代码

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

3.10 存在很多eslint-disable注释

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

3.12 命名规范

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

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

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

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

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

4.2 避免使用delete

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

4.4 尽量使用CDN地址的图片

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