Google 教我如何進行 Code Review
01 Nov 2020每天我都要做很多次 code review，但到底怎樣 code review 才是好的呢？這篇文章是我閱讀 Google 的「How to do a code review」的筆記還有自己的小感想。
Code Review 的標準
做每件事情都一定是有個終極目標的，那麼 code review 的目標就是能提高程式庫（code base）品質。
基本原則
- 審閱者（reviewer）給的建議必須是開發者（developer）能做改善的，因為那種很難改的建議會讓開發者不想改、改不動。
- 審閱者要確保每個 CL (change list) 品質是好的，而不會搞壞程式庫 - 這是審閱者必須要堅持，因為當團隊有時間壓力下，常常會為了趕專案而不在乎程式碼的品質。
- 審閱者要確保程式庫的程式碼是一致的、好維護、好讀易懂。
- 最高指導原則：只要這次 CL 是能改善程式碼的品質就應該要被 approve，「沒有最好，只有更好」、「人非聖賢」，不必要求完美，只要持續進步即可。為了讓這個 CL 完美而延遲上線時間就是不必要的。
- 如果開發者新增了不必要的功能 (feature)，則審閱者可以拒絕這個 CL。
- 審閱者必須要能在評論（comment）中表示能改善的地方，但若並非必須修改的、nice to have，則要標註「Nit」(nit + pick 或 nitpicking，表示「挑剔」的意思)，開發者就能選擇要不要在這個 CL 做修改，或當成新知即可。
- 為了修復緊急的 bug 的 CL 該怎麼辦呢？審閱者此時要關注的是自身要快快進行 review 和協助開發者注意程式碼的正確性，有修復問題即可 approve，然後之後要再重新 review 來給予原本應該要給的意見。
指導、教學 (Mentoring)
在 code review 時在 comment 分享新知能幫助開發者能寫出更好的程式碼。注意，若只是純分享而非要在本次 CL 做修改，要在 comment 前加上 Nit。
以下列出一些注意事項…
- 有助於改善程式碼品質的技術才是值得寫在評論裡面的，至於個人喜好就不用寫了。很多開發者做久了會養成一些習慣，但這些習慣並不會讓自己寫的 code 變好，那麼這些習慣就不應該被推廣，若是壞習慣更應該要改善。
- 必須要遵守 style guild 所訂下的規則，至於沒有在 style guild 所規定的範圍內，就以開發者的選擇/偏好為主，因為未被規範應屬小事，而這些小事並不會損壞程式碼品質。
- 若開發者的設計可證明是有用的解法，則審閱者必須接受開發者的 CL；否則建議使用通用的設計原則。這是為了鼓勵開發者要多多創新、研究更好的解法來解決問題，畢竟守舊的人不會是好的工程師。
- 如果沒有前例可循，審閱者可衡量狀況，要求開發者與目前程式碼的 codebase 一致，盡量達到程式碼的一致性，這樣就不會把 codebase 弄得更糟。乍看之下與 3 衝突，但其實沒有，原因是解法是可以追求卓越的，但同時也要兼顧一致性，也就是好維護、好讀易懂。
解決衝突
這裡指的衝突是人和人溝通上的衝突，不是 code 要怎麼 merge 或 rebase 的問題。若有衝突的話，審閱者和開發者必須根據 The CL Author’s Guide 和 Reviewer’s Guide 來達成共識。建議面對面或視訊會議來做溝通，而不是只是像網友般用 comment 的文字溝通而已。如果只是像網友般用 comment 的文字溝通，那也要將每次討論的結論紀錄下來也放在本次 CL 的 comment 裡面，給之後的人參考用。
若還是無法達成共識呢？就只能尋求更高位子、能做決策的人的支援了，例如：找 technical leader 一同與團隊討論，或要求程式碼的維護者來做決定；又或要求技術經理（engineering manager）來幫忙做決定。千萬不要就讓這個 CL 躺著躺到天荒地老、無法結案。
Code Review 到底要看什麼？
設計 (Design)
最重要的是「設計」。
這次 CL 的程式碼是有意義的嗎？我看過不少程式碼是在寫廢話的更動的範圍是什麼-是更動到專案的 codebase 還是函式庫 (library) 呢？
與系統整合的狀況好嗎？千萬不要改 A 壞 B 啊 現在加上這個新功能是好的時機點嗎？(這個問題是比較高層次的問題，我個人是認為這應該在 design review 時提出來，而不是 code review 階段，況且也很容易拖延這個 CL 的 code review)
功能 (Functionality)
這次 CL 是否達到開發者想要完成的功能？對使用者來說，這是好用的功能嗎？對未來的開發者來說，這段程式碼好維護、好擴充嗎？我們會預期開發者在提交 CL 做 code review 前已完整測試過自己撰寫的程式碼，而審閱者也必須來幫忙做測試，甚至是幫忙測試極端的狀況 (edge case)，例如：同步的問題、把自己當使用者來用用看、掃除可見的 bug。把自己當使用者來測試這段程式碼是很重要的，因為 UI 的變化不見得能從閱讀程式碼中完全看出來…因此若無法完全掌握細節，也可以請開發者來做個 demo。
另一個很難從 code review 中閱讀程式碼時看出的問題是 deadlock 或 race condition，因此需要開發者與審閱者一同留心這個問題，這也是為何不推薦 concurrency model 的原因，因為這類複雜度較高都情況，code review 是無法檢查出來的。
複雜度 (Complexity)
本次 CL 的程式碼可以更精簡嗎？不管是從 function 或 class 的角度來看，是否太複雜呢？「複雜」通常代表「難以讀懂」，更代表未來的維護者「容易讓它產生 bug」。
另外一個議題是過度設計 (over-engineering) ，「過度設計」常會使得程式碼變得複雜，像是嘗試變得更通用或是新增了目前用不到的功能。審閱者要特別注意過度設計的問題，鼓勵開發者先專注在解決目前的問題上，而不是考慮到超遠以後的事情…因為未來的事情都說不準啊，誰能知道未來會有什麼樣的需求呢？尤其是在尚未找到產品存活的目標、TA 的口味、市場的需求的時候，說不定產品很快就倒了
測試 (Tests)
本次 CL 是否有 unit test、integration test 或 end-to-end test 這些測試程式呢？除非是緊急狀況，每個 CL 應該要有相對應的測試程式。而測試程式不能只是花瓶、好看而已，必須是真的有用的、正確的、合理的。例如：測試碼偵測的錯誤時，可以準確斷定目前程式碼真的有問題；測試的斷言必須簡單有用；使用不同的測試方法來做測試以試圖面面俱到…千萬不要覺得測試程式碼的重要性遠低於開發功能，而不需要付出人力時間來做維護。
命名 (Naming)
工程師十大難題之一就是「命名」，審閱者要好好幫開發者檢查有沒有取個合理的名字。一個好的名字要足以讓大家明白「它是什麼」或是「它要做什麼」，而不是一堆字合起來但完全看不懂是什麼意思。
例如：要做字串處理，比起 doSomething，formatString 一定是好很多的…
- (X) doSomething
- (O) formatString
註解 (Comments)
開發者所寫的註解是否清楚易懂、皆為必要？註解通常要寫的是為何這段程式碼要存在，像是當初的決策等，而不是寫它在做什麼，因為如果還要寫註解才能讓人知道它怎麼運作就是程式碼寫得很複雜、難以讀懂的意思，必須要改善程式碼使之精簡好讀（回到上一小節「[複雜度(#複雜度-complexity)」那部份來做檢閱）。當然有些狀況是不得已必須要解釋程式碼的運作方式的，像是內含困難的 regular expression 或演算法，但多數狀況的註解是可以寫得更好理解的。
對於先前的註解也可以重新做檢視，看看要不要做刪除或修改。
注意，不要把註解當文件來寫，註解應該表達的是為什麼這段程式碼要存在、怎麼用和使用的時機，若需要更多的說明可以考慮撰寫 wiki page 或 readme。
撰寫風格 (Style)
可參考 Google 的 style guide。
請開發者務必遵守 style guild 的規範，若 CL 的撰寫方式在目前的 style guild 沒有提到，而 review 有建議想要提出，可以在 comment 前加上「Nit:」，畢竟是沒先講清楚的，不要造成這個 CL 被卡著而不能進 code。
而針對 style 相關的調整，開發者必須另外開一個 CL 來做變更，這樣分開目的所提出的 CL 是比較容易了解到底改什麼、為什麼原因而改，而且之後若想要 revert / cherry-pick 這個 CL 也是比較容易的。
風格一致 (Consistency)
如果現存在 codebase 裡面的程式碼與 style guild 的規範不同，該怎麼辦呢？除非這樣的修正會讓程式碼造成誤解，否則就把它調整成符合規範的樣子吧！不然規範就形同虛設了。
如果開發者所撰寫的方式在 style guild 沒有規範到，那就跟現存在程式庫的寫法一致即可；不然，開發者可以發起一個 bug or TODO（我認為可以是 issue or 維護的 task）來處理現存在 codebase 的程式碼。
文件 (Documentation)
將本次 CL 的變更，同時更新在文件上；若移除某些程式碼或功能，建議同時移除這份文件；若沒有文件，要想辦法生出來。
讀懂每一行
仔細閱讀本次 CL 的每一行程式碼，並且一定要能讀懂，千萬不要只是掃過一次（魔鬼藏在細節裡）。若很難讀懂、花了太多時間導致延誤 review 的時間，應該要先通知開發者可能要花更多時間來做 code review，並且找開發者來說明到底是在寫什麼，然後再繼續做 code review。在 Google 上班的工程師基本上程度都不會太差，所以寫得讓人看不懂一定是開發者的錯，也就是回到前面所說的，太複雜的程式碼可能會產生更多 bug…就要讓開發者來看看能不能改善了。當然也有可能是審閱者的人程度不夠，那就找一個程度更好的人來看。
檢視關聯部份 (Context)
不要只是看更動的片段，而是要看所有有關聯的程式碼，甚至是整個/多個檔案，來評估這個變更是否合理。例如：看完整個檔案而不是這次變更的幾行，會發現可以重構、切割為更多更小的函式；思考這次變更對 codebase 的健康是否有幫助？如果會讓 codebase 變得更糟，那麼 CL 就不能被接受 —「羅馬不是一天造成的」codebase 會愈來愈糟往往是因為一次又一次小小的糟糕的變更累積而成的。
適度的鼓勵：你做得很好 👍
若在 CL 中看到不錯的地方，千萬不要吝於對開發者表達讚美。code review 不是只針對改進錯誤而已，鼓勵也很重要。在指導開發者上，告訴開發者什麼是對的，比糾正錯誤還來得重要。
總結
針對「Code Review 到底要看什麼」，總結身為 reviewer 必須確保以下事項
- 程式碼必須設計良好。
- 程式碼必須好理解、好維護、好擴充。
- 對於使用者來說，這個功能必須是好用的。
- 程式碼沒有過於複雜。
- 程式碼沒有增加不必要的功能。
- 有搭配相關的測試程式。
- 測試程式必須是有用的。
- 註解是簡單有用的，並且是解釋「為什麼」而不是「做什麼」。
- 有文件。
- 符合 style guide。
- 讀懂每一行、每個細節、所有有關連的部份，確保 codebase 的健康。
- 做到指導者應該做的工作：適度的鼓勵開發者做得好的地方、糾正錯誤、傳遞新知。
如何有效做 code review
目前已知在 code review 時要看哪些重點了，那麼…要怎麼有效的 review 分散在多個檔案的 CL 呢？
總結應該要注意的點…
- 第一步，這個變更合理嗎？
- 第二步，看最重要部份，而這個部份有被良好的設計過嗎？
- 第三步，依照適當的順序看其餘的部份。
第一步：先大致瀏覽整個 CL
大致瀏覽 CL 的描述（通常我都會圖文並茂的敘述這個功能或修改的狀態，讓審閱的人能快速進入狀況）和更改的程式碼，先思考「這個變更是合理的嗎？」假設不合理，一定要馬上跟開發者解釋原因，並且務必在拒絕（reject）CL 前提出更好的建議。例如，你可以這樣說「謝謝你做了這麼多的努力，但我們現在正在刪除你剛剛對於本次 FooWidget 的修改…這裡暫時不打算做任何調整，但你可以重構 BarWidget 嗎？」這樣拒絕開發者所提出的 CL 並提出更好的建議是禮貌的，而這樣的禮貌很重要，因為這表示儘管不認同但依然尊重每個開發者。在我看來，在指出錯誤時必定要再給予方向，才是有建設性的建議。
如果收到很多的 CL 但都不是想要改善的部分，這代表必須重整團隊的開發流程或考慮外部的貢獻者所提出的已知的作業方式，再經過更多的溝通才做修改、提出 CL。因為比起做了很多工作但被拒絕，讓大家知道「不要做什麼」才是比較好的。
第二步：檢視這個 CL 最重要的部份
找出這個 CL 最重要的部份，通常會有一個檔案有最多邏輯上的變更的，那就是這次 CL 最應該要著重的部份，先檢視這部份，這會對其他部份給予上下文的線索並加快 review 速度。但如果這個 CL 真的太大而導致看不出哪塊最重要，可以詢問開發者提供建議應該要看哪部份，或要求開發者把本次 CL 切成多個 CL 方便 review。
若在 CL 中看到一些重大的設計方面的問題，就算還沒有 review 完全部的 CL，都應該要馬上告知開發者在設計不佳的狀況下，產出的東西根本不會符合實際要求，要直接 reject 並且請開發者做調整，不然繼續做 code review 根本就是浪費時間。
為什麼審閱者要馬上通知開發者呢？原因如下
- 開發者可能在提交 review 後就展開新的工作，而這個新工作可能是根據這個 CL 為基礎的。若這個 CL 有設計上的問題，那麼接下來的新工作勢必要在修正這個 CL 後重做。這當然也是為了節省彼此的時間，不要讓錯誤產生更多的錯誤。
- 開發者通常都有 deadline 壓力，若是設計上的錯誤必定需要花較多時間修改，而早早讓開發者針對設計不良的問題趕快修正，除了可以給開發者更多 buffer 來做調整外，也可避免在 deadline 前才草草做修改而導致程式碼品質不佳。
第三步：依照適當的順序看其餘的部份
一但確認沒有設計上的大問題後，接下來就依照邏輯順序檢視個別檔案，並且要確保沒有漏掉任何一個檔案。通常在 review 完最重要的那個部份後，就可以輕鬆地用一些 code review 工具來做檢視。
測試程式通常可以告訴我們大致上是做了什麼更動，因此先閱讀測試程式也有助於了解主要的程式碼。
Code Review 的速度
Google 做了開發團隊的生產速度的優化，而不是優化單一開發人員編寫程式碼的速度。個人的開發速度很重要，但團隊的開發速度更重要。
當 code review 速度慢時，可能是發生了以下的狀況：
- 整個團隊的速度都下降。當 review 延遲時，後續的新功能開發與修 bug 也會被延誤。
- 若審閱者每隔幾天才回覆 code review，但要求開發者做出大幅度的修改，那勢必會讓開發者抗拒 code review 這件事，因為這樣會造成修改的困難和導致心情沮喪。這通常和 review 的嚴格程度有關，若 review 能快速回應，這樣大幅度的修改是有助於 codebase 的健康程度，也有助於減緩開發者的抱怨-大部份的抱怨都可以靠加快速度來解決。
- codebase 的健康程度會受到影響。當審閱者的速度緩慢，就會增加開發者提交 CL 的壓力，導致 code 的品質下降。code review 緩慢也會導致減緩清理 code 的效率、重構和改進目前 CL 的進度。
Code Review 的速度或頻率應該怎樣呢？
假設你不是專心正在做某個特定 task，那麼應該可以在 CL 發出後不久的時間內開始進行 code review。最多在「一個工作天」就要回覆這個 CL，也就是收到 CL 的隔天早上的第一件事就是回覆 CL。如果有需要的話，應該在一天之內有多輪審核。
速度與中斷
讓開發者不停的 context switch 會減損產能，也就是說，打斷正在進行的工作後需要一段時間才能恢復到專注時的產能，因此不建議為了做 code review 而打斷當前的開發工作，這時候是可以請被 review 的開發者稍待 code review 的回覆。為了不打擾開發者的開發工作，開發者可以選擇工作告一段落、午餐後或是會議結束後來做 code review。
快速回應
code review 應在意的是回應的速度，而不是在意這整個 CL 通過 review 的整個時間。當然這整個 CL 通過 review 的時間也應該要是快的，但審閱者能（多輪）快速回應開發者是更重要的事情，而且這樣快速的回應能緩解等待的不悅。也就是說，可以先給綜觀的評論，或是讓可以更快速回覆的人來做 code review，都可以讓開發者得到快速回應。注意，快速回應不代表自己要中斷手上的工作；而且，快速回應仍應花足夠多的時間做 code review 以確保程式碼的品質 (LGTM)。
跨時區
儘量在對方當天上班時間時回覆，或是確保在他（們）隔天上班前完成 review，讓他（們）能在一早上班時收到回覆。
在評論中標註 LGTM
LGTM 表示「Looks Good To Me」，意思是審閱者大致上瞄了一下覺得可以摟-本次 CL 已過關，可以合併進入程式庫中了。
為了加快 code review 的速度，有一些狀況是審閱者就算沒有 review 過所有的程式碼，貨留下一些未解決的評論，而可以直接以 LGTM 同意通過本次 CL。以下是這些狀況…
- 審閱者認為開發者能適當的處理剩下的評論。
- 剩下還沒 review 的程式碼不是很重要，所以就算有評論也不需要急著修改程式碼…也就是前面提到的「Nit」(沒什麼問題，我只是特別龜毛挑些小毛病）。
針對以上兩種狀況，審閱者應該要明確指出是本次 CL 的哪些部份。當審閱者與開發者在不同時區時，特別推薦使用 LGTM，不然開發者可要等一天後才會得到審閱者回覆「LGTM，接受本次 CL」。
一次收到一大包程式碼的 CL
假設收到很大一包的 CL 導致難以確切預估完成 review 的時間，可以要求開發者拆解為多個較小的 CL。但若無法拆解，審閱者至少可以針對整體設計給予一些回饋來讓開發者做改善，因為審閱者必須做到不要阻擋開發者接下來的工作並兼顧程式碼的品質。
好好進行 Code Review 能讓未來的日子更好過
假設確實遵照 guideline 來做 code review，會發現 code review 的速度會愈來愈快。開發者會讓發出的 CL 是有一定水準的、能維持程式庫的健康，而能減少 code review 來回的次數和時間。審閱者也能快速回應而無不必要的延遲。注意，不能在 code review 的標準或品質上做妥協來加快速度，因為以長遠角度來看，降低標準或品質並不會讓 code review 的速度變快-因為增加技術債會讓以後的日子更難過。
緊急狀況
在緊急狀況下所產生的 CL 該怎麼辦呢？像是要上線一個必須立刻修復的 bug…審閱者這時候要關注的是 review 的速度和程式碼的正確性，有修復問題即可 approve，然後之後要再重新 review 來給予原本應該要給的意見。
如何給予 Code Review 的評論
先講結論。
- 態度要和善。
- 為需要修改的部分提供充分解釋理由。
- 在「給予明確的指令」與「指出問題並讓開發者決定該怎麼做」之間保持平衡。
- 鼓勵開發者簡化程式碼與加入註解。
禮貌
審閱者的態度必須是要有禮貌與尊重開發者的，審閱者尤其是要說出一些可能會讓人感到不開心或具有爭議的話，就必須注意是要「對事不對人」，可以針對程式碼留下評論，而非針對 開發者 給予評論。
例如…
- (X) 為什麼你不用 single-thread 的方式實作？這裡看不到使用 concurrency model 後的好處。
- (O) 在這裡使用 concurrency model 增加了系統的複雜度，但卻沒有獲得應有的效能改善。由於沒有提升效能，在這裡使用 single-thread 即可，而不需要用 multi-thread。
解釋「為什麼」
在評論中解釋「為什麼」，有助於開發者了解這麼做的原因以及有什麼好處。
給予指引
修復 CL 是開發者的責任，因此審閱者是不需要來對解法做細部的設計或幫開發者撰寫程式碼。但當審閱者指出問題所在、提供解法的方向來讓開發者做決策時，能讓開發者學習到更多東西，也能讓 code review 的過程更順利。然而，直接的指示、建議或範例程式碼會是更有幫助的。code review 的第一要務是讓 CL 變得更好，第二個目標則是讓開發者學到東西、提升技能，以期未來能花更少 code review 的時間。
如果審閱者看到 CL 中有不錯的地方，也可以在評論中提出。例如：開發者整理了一段很亂的寫法，並且加上測試程式，或是讓審閱的人有學到什麼。這類鼓勵的評論也能讓開發者知道這麼做是好的，值得持續下去。
接受說明
當審閱者要求開發者解釋一段程式碼時，通常會導致開發者重寫來讓這段程式碼更簡單易懂。不過有時候，開發者只會加上註解，只要這段註解不是在解釋過於複雜的程式碼即可，這樣也是可以接受的，不一定要重寫。這樣的解釋通常會希望寫在程式碼裡面，但若只是要讓審閱的人看懂（而其他開發者是可以很容易理解的），是可以只寫在 review 的工具裡面。
拖延處理 code review 的回覆
開發者不同意審閱者的建議或認為審閱者過於嚴格的時候，很可能會拖延處理 code review 的回覆。
誰才是對的？
當開發者不同意審閱者的建議時，審閱者應該要先思考一下開發者的解釋是否有道理（尤其是否可增進程式庫的健康程度），因為開發者通常是最了解目前程式碼狀況的人。如果開發者的看法是有道理的，就採納吧。
但若審閱者的建議才對呢？審閱者應該要做更進一步的解釋，並且針對開發者所提出的疑問提供解答，也要告知這樣的改善好在哪裡。特別是當審閱者認為這樣的建議可以改善程式庫的健康程度，審閱者應該要應該要堅持這樣的主張，儘管可能會導致要多做一些工作，但這樣的工作通常可以很快完成。有時候在修改程式碼前會來來回回好幾趟來解釋審閱者的評論，注意要保持禮貌，並且讓開發者理解到審閱者有聽到開發者的建議，只是不認同。
基本上大家保持開放心胸來討論就應該不成問題摟？
不要擔心會讓開發者不開心
審閱者有時會認為若自己堅持要做修改，則會讓開發者感到不開心，但其實這樣的不開心可能存在，但也只是短暫存在，之後開發者會感激審閱者對於程式碼品質改善的建議。況且，若評論的態度是禮貌的，開發者通常也不會覺得不悅，因此審閱者應該要認清這種不悅的情緒通常和撰寫評論的方式或禮貌有關，而非針對堅持程式碼的品質。
等會再處理
開發者常會因為自身有太多工作而推遲（甚至是不做）某些工作，例如：清除廢棄程式碼。通常開發者希望審閱者能先專注在主要的工作上，也就是允許本次 CL，然後再下一個 CL 再來解決廢棄程式碼的問題，只是開發者可能會因為工作的優先順序而導致這件事情都不會發生。因此，若無其他考量（例如：增加程式碼的複雜度），應堅持在本次 CL 進行，而不是之後再說；若真的會有問題，則也必須開一個 bug 單給自己、並在程式碼中加註 TODO 來做提醒務必要完成這個工作。
對於嚴格審核的抱怨
如果審閱者過去對程式碼審核的標準是比較鬆散的，而現在變得嚴格，開發者可能會不適應這樣的改變而產生抱怨，解法就是審閱者要加快自身審核的速度。抱怨無法馬上消失，但開發者會發現嚴格的審核對程式碼品質的提升而支持這樣的做法。
若審閱者與開發者發生衝突，請再回頭閱讀這裡。
