如何進行 Code Review?

  1. What? 我們要看什麼重點?
  2. How? 大致的流程為何?

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

最主要分成兩個面向:

  1. 以「團隊面」來說,每個成員都能彼此熟悉做的東西以及互相學習的機會。

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

Understand 看懂

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

  • 如果是 Bugfix,則要確認這個問題的根本原因為何以及是否真正修正。
  • 如果有新的東西,這是一個很好的學習機會。像是出現了一個你沒看過的類別,你就可以藉此機會去了解(自己去查或者直接跟同事請教)

Functionality 功能

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

1. 上漲:如果現在價格 > 開盤價,則顯示綠色。
2. 下跌:如果現在價格 < 開盤價,則顯示紅色
3. 平盤:如果現在價格 = 開盤價,則顯示白色。
Answer: 上面上漲下跌的顏色顯示邏輯寫反了。

Logic 邏輯

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

邏輯有一個明顯的錯誤,需要幫忙糾正。
宣告了傳入新舊密碼的方法
保護當事人,已經碼掉 Git Blame,如果他在應該被幹到飛。
Answer: 這個 API 方法(上圖)預期依序傳入新密碼、舊密碼,但是實際呼叫(下圖)時卻是先傳入舊、新密碼。
Answers:
1.字串拆分 split 陣列沒有檢查長度就直接取 [0], [1], [2],如果輸入的是 “2.3” 就會爆掉。
2. 另外這個實作也假設輸入的字串都是數字,如果輸入 “a.b.c” 在 toInt() 就會爆掉。

Readability 可讀性

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

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

Modularity 模組化

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

Reusability 複用性

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

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

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

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

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

提交者

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

審查者

  • 如果是比較具規模的 PR 都蠻建議直接把程式碼拉下來用 IDE / Editor 看,這樣可以看的比較透徹,例如能夠看到新實作的方法是從哪裡被呼叫以及如何呼叫的,如果只看 Diff,很難追蹤整體實作架構或追流程。
IDE + Diff 搭配一起看最有效率
必要修改,對方一定要調整他的實作。
建議修改,通常是一些更優化或新的寫法,不改也不影響。
如果是給 Junior 開發者,給的建議作法可以直接且明確,減少他摸索的時間。
如果是 Senior 則給幾個改進的方向即可,通常 Senior 會自己斟酌挑出適合的作法。

關於其他

  1. 如果雙方意見不同:請找對方好好討論,交流一下想法,找出適合的改動。
  2. 關於 PR 的大小,我個人是提倡小改動就該開一個 PR,小 PR 數個好處:可以看比較快、東西少所以可以看的比較全面、合併也比較簡單。
一個大功能拆分成數個小功能的 PR

--

--

白昌永 (大白) Developer, Lead, Athlete, Learner, focused on Android | Kotlin | Flutter | Python.

Love podcasts or audiobooks? Learn on the go with our new app.

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store
Engine Bai

Engine Bai

白昌永 (大白) Developer, Lead, Athlete, Learner, focused on Android | Kotlin | Flutter | Python.