前端 Code Review 指北
作者：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.ACTIVE2.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 = false3.13 過多的非業務邏輯相關代碼 (如超過 10 行的上報, 參雜在業務邏輯裏)
如果在業務邏輯裏摻雜太多的上報，後續理解業務邏輯時需要看上報邏輯，查上報邏輯的時候也需要理解大量的業務代碼。點擊埋點和曝光埋點都可以以屬性的形式掛在元素上，通過冒泡，統一進行處理。
<button
data-exposure
:data-exposureval="{event:'bottom.btn'} | jsonStringify"
data-event
: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 的細則了。
手都寫麻了。當然，肯定還有很多遺漏的點，歡迎補充。
本文由 Readfog 進行 AMP 轉碼，版權歸原作者所有。
來源：https://mp.weixin.qq.com/s/yyqGipUGMbfcLmXcQ2QBcA