如何進行 Code Review?

Engine Bai

10 min readMay 17, 2022

在多人軟體開發過程中，Code Review 就是在我們完成一個功能或問題修正後，會請其他開發者來幫忙檢查、給建議，這過程扮演了一個十分重要的角色，讓我們來看看該如何進行。

今天會講三個面向：

Why? 為何我們要做 Code Review?
What? 我們要看什麼重點？
How? 大致的流程為何？

不論是你的程式被別人審核、或者你審核別人的程式碼，如果還沒有相關經驗，那建議找一間有這樣機制的公司加入，可以讓技能磨練成長的更快。
如果你們有這樣的機制，那麼你們的流程怎麼跑？你自己會看什麼重點？可以思考一下再來看這篇文章。

第一、Why? 為何需要這個流程？

最主要分成兩個面向：

以「程式面」來說，我們會有一個比較健康的程式碼。
以「團隊面」來說，每個成員都能彼此熟悉做的東西以及互相學習的機會。

以「程式面」來說，我們可以透過這過程幫忙找到一些潛在的錯誤或問題，讓我們對交付的產品能更有信心一點，如果是剛加入團隊的新人或者比較資淺的開發者，也可以透過這個過程來預防一些重工、不符合專案的實作，讓程式碼的一致性更高，同時藉此更加了解專案。

以「團隊面」來說，當你在看別人程式碼的時候，你熟悉對方實作的內容，不用擔心他請假或離職的時候無法接手對方的東西，同時這也是一個很好學習成長的機會。另外，Code Review 過程中一定會有討論，這樣也創造成員之間的互動和交流。

第二、What? 我們要看什麼重點？

Understand 看懂

第一個最基本也是最重要的，你要看「懂」。

看懂整個 PR 的實作，要可以知道對方是怎麼實作這功能的，包含所用的套件或者語法。
如果是 Bugfix，則要確認這個問題的根本原因為何以及是否真正修正。
如果有新的東西，這是一個很好的學習機會。像是出現了一個你沒看過的類別，你就可以藉此機會去了解（自己去查或者直接跟同事請教）

如果是功能開發，個人習慣依照流程下去追程式碼，而不是 Diff 所列出的檔案先後去看，在看完某個檔案後，會去 GitHub 檔案上面可以打勾。PR 右上方會有進度條，依照流程看完之後可以回頭看看還有哪些變更還沒看到。

Functionality 功能

功能的話我們會看實作有沒有「正確完成功能規格」，假設我們現在這 PR 是在做股價顏色顯示的需求：

1. 上漲：如果現在價格 > 開盤價，則顯示綠色。
2. 下跌：如果現在價格 < 開盤價，則顯示紅色
3. 平盤：如果現在價格 = 開盤價，則顯示白色。

下面提交的程式碼是否有錯誤？

Answer: 上面上漲下跌的顏色顯示邏輯寫反了。

Logic 邏輯

邏輯的話就是要看是否實作邏輯、流程正確。

以下真人真事，我接手某新創公司前人寫的程式，在那前人的時期沒有 Coder Review，某天使用者回報無法重設密碼，然後我去追了一下程式碼，發現了下面程式，請問哪裡有問題？

Answer: 這個 API 方法（上圖）預期依序傳入新密碼、舊密碼，但是實際呼叫（下圖）時卻是先傳入舊、新密碼。

另外我們會看看 Edge cases / Invalid input 是否有正確被處理到。假設我們宣告了一個資料結構來儲存「版本」，然後提供一個字串轉版本的方法，請問這個方法實作當中有什麼問題你應該要抓出來？

Answers:
1.字串拆分 split 陣列沒有檢查長度就直接取 [0], [1], [2]，如果輸入的是 “2.3” 就會爆掉。
2. 另外這個實作也假設輸入的字串都是數字，如果輸入 “a.b.c” 在 toInt() 就會爆掉。

Readability 可讀性

可讀性：我們會看整個實作是否容易理解和追朔流程？整體實作邏輯、架構和流程應該是很貼近功能需求，如果有看不懂的地方理當請對方解釋（寫註解或者口頭）或者請對方調整作法。另外可讀性還牽扯下列項目：

命名：是否夠明確，是否符合我們團隊的規範。
符合 Coding Style：基本上我們不太會在 Code Review 做格式的檢查，這應該給自動化處理而且這應該是團隊當初就應該先統一一套團隊共用的規範，避免不同開發者使用不同的格式。
註解：我們不會寫很多註解，因為程式碼本身就是很好的註解，只有在一些比較容易看不懂、或者特別邏輯的地方我們會寫適當的註解，而且我們註解只會描述為何要有這段程式。如果你看到一段蠻 “奇特” 寫法的地方卻沒有附上註解，就應該請對方補。

Modularity 模組化

模組切分的是否恰當，考量單一職責原則，一個元件的職責或目的應該很明確，像是 View 我就是放單純 UI 的東西，Model 就負責處理資料。

Function 的參數是否過多、過長？如果是過長那很可能是職責太多太雜，我們應該把這個方法拆分。

如果真的需要保留這個多參數，可以考慮另外用一個資料結構封裝所有的參數，好讓此方法只傳入此資料結構。或者另一個作法是，保留這個很多參數的方法，但是是私有的，開不同的公用方法，但是參數不會那麼多，讓外部知道一些特定使用情境。

Reusability 複用性

UI 元件、資料結構、方法或者 API 在之後是否好複用？需求稍做調整的時候，這裡的實作是否要作很大的調整？舉例我們實作一個客製化圓角框線按鈕元件，但是我們只提供文字可以設定而已：

這很明顯一定不好複用，例如想要調整顏色呢？文字大小？框線粗細？或者其他屬性呢？

另外是否有重複的程式碼？假設專案之前已經一些 Untilities 的方法，對方沒使用（可能是沒發現）也做了一樣的邏輯，這就重工應該請對方改用專案內寫好的方法。

Error Handling 錯誤處理

一個流程可能會出錯，像是最常見的例子：API 錯誤（例如斷線），看看是否有 try-catch 捕捉到，以提正確的處理邏輯：處理邏輯可以是 log 紀錄、顯示錯誤訊息在畫面上、提供預設流程之類的。

Leak

是否有在正確時間點確實釋放資源？（像是畫面關閉時）如果沒有可能會造成洩漏，程式跑久了就容易耗費更多系統資源，甚至造成 Out of Memory (OOM) 當機。

Complexity

看有沒有更短、更快、簡單、清楚的實作方式。舉例來說，下列我們要實作「計算本公司主管的平均年齡」，Kotlin 就有針對 Collection 提供很方便的方法來做這件事情，我們就應該建議用比較簡潔的作法。

// Suggested way for this
getAllEmployees().filter { user.title.contain(‘lead’) }.map { user.age }.average()

Thread Safe

針對有平行處理、非同步的需求，實作是否有滿足 Thread Safe。舉最簡單的例子，一個變數在不同執行續做加總，這個問題很容易出現 Race Condition，要看看是否有好好處理。

Race Condition: 不同程式（執行續）在同一時間存取和操作同一資料，結果會依照這些程式執行的順序而不同，造成結果不正確和不一致。Thread Safe: 為了解決上述競爭問題所提供的一個機制，讓不同程式存取同一資料的結果可以正確一致。

Completeness

再來幫忙檢查看看有沒有測試程式或者假資料忘了移除，有沒有 // FIXME 忘了修之類的。

Testing

最後看看對應的實作或改動有沒有撰寫單元測試涵蓋到，如果公司沒有硬性規定，至少看看是否既有單元測試是否會通過（如果有既有測試的話），可能你實作改壞了某個東西沒發現到。

有沒有要求 PR 也要寫單元測試就要看公司政策了，有些公司為了趕時間上線就不會要求要寫單元測試。

第三、How? 我們的流程為何？

這邊我們分成兩個角色來看：「提交者」和「審查者」。

提交者

在實作之前：這邊強調在「實作」之前！先和團隊討論架構、設計和實作方向，確定整體規劃都沒問題後再下去實作，一般不會在 Code Review 的時候才過（想想如果到後面才發現整個架構設計有問題，那前面時間不都浪費了）。再來，不論是資深還是資淺的工程師，所有提交的東西都應該要被審查（人嘛！難免會犯錯）。