你怎麼做 Code Review 呢？以前端開發為例
怎麼做 code review？相信是工程師們，以及每個多人合作的研發團隊都會面臨到的問題，就算是同一間公司，因為每個團隊關注的點都不一樣，大家執行的方式可能也都不盡相同。
這篇想針對工作上的經驗，整理一下團隊內怎麼來做 code review，單純分享我們團隊怎麼做，並不一定適用於所有的團隊，每個團隊還是必須在進行 code review 之前，先共同有一個共識。
為什麼要 Code Review?
身為一個開發者，日常工作在多人協作的情況下，最可能遇到的問題就是，不同的人寫的程式碼都不盡相同，可能存在許多有問題的程式碼在裡面，而 code review 是一個維護程式碼品質且降低錯誤的一個普遍方法。
Code review 這件事，我相信除了學習怎麼做之外，需要一定程度的經驗累積，網路上有許許多多的文章分享要怎麼做 code review，推薦參考 Google Engineering Practices doc 裡面關於他們怎麼做 code review:
不過就像前面提到的，自己覺得 code review 可以參考準則，但很難有一個所謂的 best practice，因為不同團隊、公司、人，等等各種因素都讓所謂的 “rule” 或是 “practice” 要因地制宜的調整。
我自己認為，以下幾點會是做 code review 最重要的原因：
- 維護程式碼品質且避免有問題的程式碼進入專案
- 讓程式碼撰寫方式與團隊規範一致
- 去除作者的盲點
- 互相學習與知識共享
Code Review 要看什麼呢？
那 code review 實際我們會看什麼呢？可以先想想在你們的團隊合作中 :
- 什麼對於專案的程式碼才是最重要的呢？
- 什麼樣的程式碼會造成大家很大的困擾？
一般來說正確性是一定要的，但我們團隊認為可讀性與可維護性也是同等重要的。一般來說，大方向的規範可能包含以下幾個面向：
是否符合團隊規範？
每個團隊或專案通常都有自己的程式碼規範，例如命名慣例、縮排規則和語法風格，或是一些必要需求，reviewer 要確保程式碼符合這些規範，以提高程式碼的一致性和可讀性。
例如，最常討論的兩行的縮排 vs 四行的縮排？
P.S. 通常這個可以透過 linter 與自動的 formatter，搭配 git hooks 在 push code 之前就處理掉。
延伸閱讀：
舉幾個簡單的例子，以前端來說，在我們團隊以下幾個是不能接受的：
- 開發的時候沒有使用特定的元件庫，自己重新造輪子
- 直接寫 HTML 的 tag 在 react 的專案中
- 使用
!important - 文字沒有使用 i18n
- 一次改動的量是不是太多
是否易於理解(可讀性)？
Reviewer 需要確保程式碼易於理解，即使是對該程式碼不熟悉的人也能理解。通常我們會希望變數名稱、函式名稱不要太過於精簡，而是要能夠直接表達出這個變數或函式的功能。
你可能有聽說過，有人為了簡潔(或是炫技？)喜歡把多行程式碼不段簡化到極致，或是追求都寫在一行，有的時候這種方式在團隊合作上來說，可讀性與可維護性並不高，且未來要修改這段邏輯反而會需要花更多時間。
舉個例子，寫一個 function 來把 data 當中，只回傳 price > 100 的物件，且內容只需要 id 與 name 。
你可以這樣寫：
const f = data.filter(i => i.price > 100).map(i => ({ id: i.id, name: i.name }));也可以這樣寫：
const filteredData = data
.filter(item => item.active && item.price > 100)
.map(item => ({ id: item.id, name: item.name }));這個例子並不是太複雜的功能，但這裡想表達的是，不論是排版、變數命名都會對程式碼的可讀性造成很大的影響。
變數命名要更語意化是非常重要的，不要出現無意義的變數命名。舉例來說，看看下面這個例子，你知道 a 是什麼，以及 getData() 是拿了什麼的 data 嗎？
const a = getData();想想看，這樣寫是不是比較好懂一些？
const userProfile = getUserProfileData();是否可維護？
Reviewer 需要確保程式碼易於維護，即在需要修改程式碼時，可以輕鬆地進行修改。舉例來說，每個函式必須維持單一責任原則，一次只做一件事，且不能會造成 side effect。
舉個例子，有一個 getUserProfileData 可以根據傳入 userType 來拿回使用者的資料，但是裡面卻又同時去更新使用者的 usage：
const getUserProfileData = async (userType) => {
const userData = await getUserDataRequest(userType);
if (userType === 'vip') {
return filterVIPUser = userData.filter(user => user.usage > 10000);
}
const userUsage = userData.map(user => { id: user.id, usage: user.usage };
updateUserData(userUsage);
return userData;
}
const normalUser = getUserProfileData('normal');
const vipUser = getUserProfileData('vip');如果這時候需求改了，只有某些特定 user 才需要更新 usage，是不是就必須在這個 function 去新增一個 if-else，並且要根據特定條件，或是要多傳入一個參數來判斷？在這個簡單的例子裡，改動並不會太難，但如果這個 function 內容很多，牽扯許多不同其他的 function，你能保證你的改動不會影響到其他使用這個 function 的其他地方嗎？
如果我們一開始就把 updateUserData 這個功能拆出來，是不是下一個人改起來就有信心許多呢？
const getUserProfileData = async (userType) => {
const userData = await getUserDataRequest(userType);
if (userType === 'vip') {
return filterVIPUser = userData.filter(user => user.usage > 10000);
}
return userData;
}
const updateUserUsageData = (userUsage) => {
updateUserData(userUsage);
}
const normalUser = getUserProfileData('normal');
const vipUser = getUserProfileData('vip');
const vipUserUsage = vipUser.map(user => { id: user.id, usage: user.usage };
updateUserUsageData(vipUserUsage);是否正確且可靠？
Reviewer 需要確保程式碼可靠，確保功能是符合預期的，我們可以透過要求提供測試結果，或是實際運行看看來驗證程式碼是不是正確的。
另外，我們也可以透過要求每一個 PR 都要有相對應改動的測試、測試結果，來達到要求最基本的品質。我們會在 PR 內如裡面要求附上 unit test 跑完的成功結果，以及這個功能改動的 UI 畫面截圖或是錄影。除了人工把關之外，我們也會在 pipeline 綁上使用前端的自動化測試 (我們使用 Cypress) 來做改動的把關驗證。
確認程式碼可靠，可以有著許多不同的方式可以達成，主要看團隊的共識來決定你們團隊想要採用的做法。
如何進行 code review?
還記得多年前在一場面試中詢問面試者，你會如何做 code review 的時候，只得到一句 “就看 code 啊“，真的是十分的印象深刻，雖然大方向來說沒有錯，但怎麼看才是問題的重點。
以前端開發為例子，通常我們進行 code review 會經過以下幾個流程：
了解這個改動的背景
由於我們的 PR 描述中，會要求大家附上這個改動的 tickct、當初的 design document 與 Figma 連結，並且會簡單描述一下主要改了什麼，我們會從這些資訊先初步了解這個改動的背景。
延伸閱讀：如何寫一份前端的開發設計文件？
實際抓下來執行
通常一個比較大一點的改動，我們會建議 Reviewer 可以到這個 branch 裡面，直接抓下來實際執行看看，看看改動的功能是不是真的有如預期，畫面呈現是否與 Figma 上面一樣，並且實際操作看看是不是有什麼問題。
除了實際抓下來執行，也可以有其他方式達成，例如我們團隊就會利用自動化搭配 mock data，直接在 AWS build 一個 for PR 的 preview，直接就會跟著你發的 PR 產生出來，reviewer 就可以直接點連結進去看直接 PR 的畫面改動了。
檢查程式碼寫法
有沒有不好的寫法？
舉個例子，在 React 開發中最常遇到不好的寫法，就是會不小心去改到了原本的物件，例如在更新 object type 的 state 的時候，直接改動到了 state 本身的物件：
❌ 直接操作 updateUser
const [userProfile, setUserProfile] = useState({
name: 'Sean',
address: 'Taipei'
});
const updateUser = userProfile.address = 'Taiwan';
setUserProfile(updateUser);🟢 使用複製的新物件
const [userProfile, setUserProfile] = useState({
name: 'Sean',
address: 'Taipei'
});
const updateUser = {
...userProfile,
address: 'Taiwan'
}
setUserProfile(updateUser);有沒有符合團隊規範？
以我們團隊為例，我們也會訂定一些希望大家遵守的規範，舉例來說，我們會要求一次 PR 的 code change 不能太大，否則這樣 reviewer 根本很難做 code review，且很容易遺漏一些問題。當看到超過團隊限制的改變數量 (可能可以是行數，或是一次改動的 ticket 數量，取決於大家的共識)，直接就可以 Request Change 不需要先看了。
而我們也會要求每次的改動，都必須要有相對應的 Unit test，確保程式碼的品質。
留下 Review 結果與 Comment
在 review 完成後，通常會根據是不是有著一定要修改的部分，來決定是要送出 Request Change 還是直接留下 Comment 而已，然而我覺得很重要的是，哪些是一定要修改，哪些又是建議而已，取決於團隊的共識。
如果團隊間沒有共識，除了明顯的錯誤之外，有時候會遇到我覺得很重要，但別人覺得不重要的這種尷尬情況，這時候通常建議可以開一場會議大家坐下來好好討論，但是如果雙方僵持不下，code review 過程就會變成一種口水戰，反而來來回回浪費大家時間。
因此我自己會認為，就算團隊只有兩人或三人在一起開發，從一開始就訂定好一些規範與共識，根據開發的經驗累積，團隊內也定期 review 這些規範，常常針對規範修正，可能會是比較好一點的做法。
Code Review 幾個注意事項
理性討論
Code review 是一個互相學習的過程，一定要記得理性討論，對事不對人，我們是針對程式碼討論，而不是針對某個人討論，要避免在 Code review 使用情緒性用語。
Code review 一定會遇到大家意見不合、僵持不下的時候，這時候請大家坐下來，好好開一場會議，以爲了產品更好的方向來理性討論。
給予 Junior 更多範例
對於比較 Junior 的開發者，在 review 的時候不要只丟出幾句話，或是只留言說這樣寫不行，可以多提供範例、或是直接建議他寫法，讓他們能夠更快、更直接在被 review 的時候學習更多。
虛心接納建議
就算你是在產品中最資深的開發者，也要虛心接受團隊成員在 Code review 給予的建議，常常你習慣或是知道得寫法，不一定是最好的，團隊的新成員有時候反而可以帶給你不一樣的想法。請秉持著有著良好的討論，大家才會一起更進步的想法。
這篇主要紀錄工作到現在對於 Code review 的看法與經驗，不一定完全正確，或是適用於不同的團隊、公司，或許幾年後自己回頭來看看也會有不一樣的看法。還是回到一開始提到的，每個團隊與組織，成員間要有個對 Code review 的共識才是最重要的第一步。
如果你覺得這篇文章對你有幫助，歡迎買杯咖啡贊助 ☕️ 謝謝
