Google 教我如何面對 Code Review
10 Dec 2020每天我都要被 code review 很多次，而如何準備 code review 時需要的內容與心態呢？這篇文章是我閱讀 Google 的「The CL author’s guide to getting through code review」的筆記還有自己的小感想。
詳細撰寫 CL 的內容
CL (change list) 的內容主要是要表達 (1) 改了什麼 和 (2) 為什麼要做這樣的修改。當我們進 code 並經由 pull request 填寫 CL 的內容，再之後 merge 進入 master 便會成為 commit 的歷史紀錄。這樣的紀錄能提供後續查閱程式碼的人一個概略的介紹與理解，而不需要直接深入每一行程式碼，追程式碼、看註解（如果有寫註解的話）來慢慢的抽絲剝繭、去理解所有的內容。
第一行要寫什麼？
CL 的描述的第一行應該要
- 簡短描述這個 CL 的目的，但也要提供足夠的資訊以供後續查閱。
- 以祈使句的方式撰寫。在英文文法裡面，祈使句表示命令、建議與要求；而若是動名詞在句首，則表示這件事情而已，沒有強制意味；不定詞則暗示不一定要做。例如：
- (O) Delete the FizzBuzz RPC and replace it with the new system.
- (X) Deleting the FizzBuzz RPC and replacing it with the new system.
- (X) To Delete the FizzBuzz RPC and replace it with the new system.
- 最後要留下一個空白行，以作為與接下來的內容作區隔。
內文必須要有用且足夠的資訊
CL 的內文（body）必須要有用且足夠的資訊，因此 CL 的內文必須要有
- 簡單描述要解決的問題，與解決的方法（為何選用此解法？有無其他顧慮？是否有其他缺點或已知的問題？）
- 其他相關資訊，例如：task 編號、評估後的結果、設計文件的連結等。注意，若連接到外部文件，必須考慮是否會因權限問題而導致未來無法瀏覽，所以盡量在此 CL 提供足夠的資訊。
- 就算是很小的 CL，其內文也應該要仔細撰寫。
錯誤的 CL 描述
填寫 CL 描述時必須要能提供有用且足夠的資訊，以下是錯誤的範例：
- 修復錯誤（fix bug）- 必須要說明是怎樣的 bug、如何修復。
- 修復打包版本（fix build）
- 新增補丁（add patch）
- 將程式碼從 A 移到 B（moving code from A to B）
- 第一階段（phase 1）
- 加上工具程式（add convenience functions）
- 刪除怪異的網址（kill weird URLs）
好的 CL 描述
以下是好的 CL 描述範例。
功能變更（Functionality Change）
RPC：移除 RPC 伺服器的訊息的內存分配器的空間限制。 —–(1)
如同 FizzBuzz 這樣的伺服器擁有很大的訊息量，若能重用則可得到很大的效能改善。將內存分配器的空間加大，並加上輕量的執行緒管理器來緩慢的釋放用不到的空間，這樣閒置的伺服器最後就能釋放所有的可利用空間。 —–(2)
說明
- (1) CL 的第一行用來說明這個 CL 做了什麼事情，也就是前面提到的 (1) 改了什麼 和 (2) 為什麼要做這樣的修改。
- (2) CL 的內文仔細說明解決什麼問題、如何解決、為何是好的解法與實作方式。
重構（Refactoring）
利用 TimeKeeper 建立一個 Task 來使用其 TimeStr 與 Now 方法。—–(1)
在 Task 中加入一個 Now 方法來取代 borglet() getter 方法（目前只有用在 OOMCandidate 用以呼叫 borglet 的 Now 方法）。之後會由 TimeKeeper 來使用 Now 方法。為了移除 Borglet 的相依性，目前是利用 Task 來使用 Now 方法，但最終的目的是要直接使用 TimeKeeper，目前會利用持續重構 Borglet 的結構的方式一步步達成目標。—–(2)
說明
- (1) CL 的第一行用來說明這個 CL 做了什麼事情，包含過去可能產生的技術債，也就是前面提到的 (1) 改了什麼 和 (2) 為什麼要做這樣的修改。
- (2) CL 的內文仔細說明解決什麼問題、如何解決、為何是好的解法與實作方式，包含目前解法可能不是最理想的，還有未來持續改善的目標。
就算是很小的 CL，其內文也應該要仔細撰寫
為 status.py 這個檔案建立 Python3 打包版本的規則。—–(1)
允許已使用這個打包版本到 Python3 的客戶來根據原本已建立的規則來建立規則，而不是重新建立新的規則。這樣能鼓勵客戶使用 Python3 而非 Python2，並且大大簡化了當前正在使用的某些自動構建文件重構工具。—–(2)
同樣的，說明
- (1) CL 的第一行用來說明這個 CL 做了什麼事情。
- (2) CL 的內文仔細說明進行修改的原因與背景資訊。
由工具自動產生的 CL
有些 CL 是由工具自動產生的，而這些 CL 應該也要遵守以上規則。
在提交前要仔細檢查 CL 的描述
在提交前要仔細檢查 CL 的描述，確保這些描述內容能適當表達 CL 的變更。
同場加映：如何撰寫 Pull Request 的內容
看完以上的建議，可能會感到對於到底該怎麼撰寫 CL 的內容依舊感到模糊。幸好已有許多現成的樣板可供使用，那就來用吧。
下圖是目前我所在的團隊所使用的樣板的部份內容，是要在提交 pull request 時仔細撰寫相關資訊的。
squash & merge 進入 master 後，點進去這個 commit log 便能看到之前填寫的詳細內容。
那麼，要怎麼在自己的 repo 設定樣板呢？
- 第一步：在資料夾
.github與docs加上檔案pull_request_template.md，並在裡面放置樣板內容即可，可參考這裡。 - 第二步：填寫樣版內容。樣板內容可包含 ticket 編號與其連結、更改的類型（例如：功能、修正 bug、文件、測試、效能、其他）、描述或根本原因（root cause）、解法或更新、checklist、參考文件等，推薦Issue and Pull Request Template Generator來選擇適合的樣板，可參考其中一個範例。
完工後，便可在之後提交 pull request 時，依照樣板填寫該有的資訊，就不用煩惱到底該提供什麼樣的內容給審閱者了。
小的 CL
為什麼要提交小的 CL？
小的、簡單的 CL 意味著
- 審閱起來更未快速，因為比起要審閱者撥出一個完整的 30 分鐘來審閱一個大的 CL，多次花個五分鐘審閱一個小的、簡單的 CL 是比較容易的。
- 提升審閱的品質、審閱者能更徹底、仔細閱讀這些程式碼來給予評論。大的 CL 往往讓審閱者和開發者迷失在評論海中而感到沮喪，或是因為要看的東西、要改的東西太多了，而導致遺漏了某些關鍵問題。（審閱者：一次看這麼多程式碼，就是會給這麼多建議啊！花很多時間很累捏。開發者：我的程式碼有寫得很爛嗎？一次要改這麼多東西壓力很大、時程很趕啊。諸如此類的內心戲。）
- 比較不容易產生 bug。由於更動範圍較小，較容易讓審閱者與開發者評估 CL 的影響範圍與衡量是否產生 bug。
- 如果 CL 被拒絕（reject）也沒有花費太多的 effort。假設拒絕一個花費太多時間精力而產生的大型的 CL，那就做了很大的白工了。
- 容易合併至主線。大型的 CL 通常因為花費很多時間，導致與主線的程式碼有很大的不同，在合併前可能要處理很多檔案的衝突（conflict），因此建議用小的 CL 加速合併的速度來減少這種差異。個人建議另一個解法就是常常與主線同步，這樣也能避免與主線差異過大。
- 容易被良好設計。小的變更較容易被良好的設計，畢竟大的變更有太多細節需要被處理而很可能被遺漏。
- 避免因為等待審閱而延誤後續的工作。盡量將每個 CL 切分成沒有關聯性的變更，這樣就不用因為在等待審閱一個 CL 而導致另一個工作被延誤。
- 容易回復（roll back）。大型的 CL 往往因為變更範圍廣、更改的檔案較多，導致若要 roll back 時會牽涉到的其他的 CL 也較多，很有可能連這些相關的 CL 也要一同 roll back，這樣情況就會比較複雜難解了。
注意，審閱者可能會因為 CL 過大而拒絕這個 CL，進而要求 (1) 將這個大型的 CL 做拆分，而拆分的工作是很麻煩的；或 (2) 需要開發者來說服審閱者為何要同意如此大的 CL。因此在一開始就提交小的 CL 會讓這整個審閱過程更加容易。
什麼是小的 CL？
通常，CL 適當的大小是一個「獨立的變更」，意思是
- CL 只為了一個目的（或是只做一件事情）所做的最小變更，因此可能只是一個功能的其中一部份，而不是一整個功能。這個範圍或大小可與審閱者討論，而不至於過大或過小。
- CL 必須包含相關的測試程式碼。
- CL 必須包含所有審閱者需要知道的訊息，包含這個 CL 的描述、目前已知的 codebase 和已經審閱過的 CL。
- 必須保證 CL 進入 codebase（程式庫）後，系統仍能正常運作。
- CL 不能過小導致難以理解它到底要做什麼。假設要加入新的 API，開發者應該要在 CL 中加入這個 API 的用法，這樣審閱者才能理解這個 API 要怎麼使用，這同時也可避免加入用不到的 API。
如何界定 CL 是大還是小？
沒有硬性規定多大是太大，而通常一個 CL 在 100 行以內的程式碼是可接受的範圍，若是 1000 行就真的太大了，但通常要由審閱者來決定。變更所散布的檔案數也會影響所謂的大小，一個檔案若有 200 行程式碼的變更也許是可以的，但散佈在 50 個檔案就是太大。
開發者畢竟是最熟悉程式碼的人，因此對於開發者來說的「很小」也許對於審閱者來說是「很大」的。推薦的做法是開發者提交比自己認知還要小的 CL，況且審閱者很少會抱怨 CL 太小 ( ￣ 3￣)y▂ξ
怎樣的狀況下允許大的 CL？
以下是允許大的 CL 的情況
- 將刪除一個檔案當成變更一行程式碼，因為這不會耗費審閱者太多時間來審核。
- 有些 CL 是由工具自動產生的，而審閱者的工作只有確認這個變更，同樣也不會耗費審閱者太多時間來審核。
依照檔案來做切分
大的 CL 可依照檔案來做拆分，這些檔案的分類方式是依據要給不同的審閱者來分組，但依舊是獨立的變更，範例如下：
- 範例 1：一個關於 protocol 的大的 CL 可拆分為 (1) 這個 protocol buffer 的修改 與 (2) 使用這個 protocol 的修改。這樣兩個 CL 就能同時被審閱，而也務必告知審閱者這兩個 CL 的關聯性，讓他們能理解來龍去脈。
- 範例 2：將實作功能的 CL 拆分為 (1) 關於這個功能的變更 與 (2) 這個功能的設定或實驗，而設定或實驗的部份可能會比較早上線，而未來若有需要也較容易 roll back。
獨立重構的 CL
重構（refactor）的 CL 最好要與功能開發或修復 bug 的 CL 分開提交，例如：移動或重新命名 class，這樣審閱者會比較容易理解每個 CL 的脈絡與目的。
小的重構（例如：調整變數名稱）可以包含在功能開發或修復 bug 的 CL，但功能開發或修復 bug 的 CL 若要包含重構，這個重構到底可以調整到什麼程度仍須由開發者和審閱者來決定，畢竟包含太大的重構可能會造成審閱上的困難。
CL 必須包含相關的測試程式
CL 必須包含相關的測試程式。「小」的 CL 只是一個概念，用來表示 CL 必須專注在特定的功能或目的，而不是指程式碼的行數很少。
當 CL 內涵邏輯新增或變更，就需要包含相關邏輯新增或變更的測試程式，就算是重構而沒有變更任何邏輯仍需要有測試程式。理想上，重構的部份應該已經有現成的測試程式了，如果沒有就請補上。
然而，測試程式是可以獨立被提交單獨的 CL 的，就如同前面提到的拆分大的 CL 的概念，包含以下的狀況都適用：
- 使用新的測試程式驗證已存在的程式碼。
- 確保重要的邏輯有被測試到。
- 重構後為了增強程式碼的可靠度，可先撰寫測試程式並提交 CL。例如：想要重構一段程式碼但目前沒有測試程式，則先撰寫測試程式並提交 CL，然後再進行重構，這樣就能確保重構後並沒有改壞原先的邏輯。
- 重構測試程式，例如：導入 helper function。
- 導入更大範圍的測試框架的程式碼，例如：integration test。
不要搞壞系統
若提交的多個 CL 而彼此間有關聯，則必須確保每個 CL 分別上線時不會搞壞系統。
CL 真的很大怎麼辦？
解法如下
- 盡量將 CL 維持小、少量、多次審閱的狀態，多練習這樣的拆分方式。
- 若真的無法避免大型的 CL，先重構並提交重構的 CL 來解決不能拆分的問題。
- 事先與審閱者溝通這樣大型 CL 的狀況，而開發者也要有心理準備-即將面臨冗長的審閱過程、可能會有很多 bug、趕快多補一些測試來確保程式碼的品質。
如何面對審閱者的評論
當開發者提交 CL 來做審閱時，審閱者應該會回覆一些評論，以下提供一些有用的建議來陪伴開發者度過這個過程。
他不是針對你啦！別放在心上
審閱程式碼的目的是為了維持程式庫與產品的品質。當審閱者提出評論時，當成這是在幫助開發者、程式庫與產品，而非攻擊開發者與開發者的能力，回顧前面提到的，審閱者也會盡量讓自己保持禮貌、快速回應、解釋評論的原因和提出有用的建議與接受討論。
有時審閱者會在評論中表達自己的沮喪，這樣做是不妥的，但開發者應該要堅強一點，問你自己「審閱者想要跟我說什麼呢？我希望可以得到一些有建設性的事情。」然後就再溝通吧，絕對不要意氣用事，像是憤怒的回覆審閱者的評論，這樣很不專業的…由於這些紀錄會永久保存在版本控管的系統中，建議想辦法讓自己平靜之後再來面對。
通常來說，當審閱者沒有提供禮貌且有建設性的回應，建議面對面或是經由視訊來針對這個 CL 來做討論。若無法面對面或是經由視訊來討論，私下寫信也是可以的-禮貌的跟他們解釋你喜歡與不喜歡怎麼做。若還是無法理性溝通，建議請主管來處理。
解釋與註解
假設審閱者提出他們無法理解的部份，開發者應該要先解釋這一部份。若無法解釋，則要加入註解說明為什麼這段程式碼要在那裡。
又，假設審閱者提出他們無法理解某段程式碼，那麼之後接手的開發者可能也會有相同的疑惑，這時候回覆在 CL 的評論就顯得沒有幫助，建議解釋程式碼後，要在程式碼中加上註解。
為你自己想想
提交 CL 的準備工作真的不少，當提交的那一刻應該是準備充足了吧！？因此當審閱者給予評論而需要改進時，開發者很容易認為評論是錯的，或是審閱者阻擋了這個 CL，應該要直接 approve 才對吧。當開發者有這樣的想法時，首先應該先捫心自問「審閱者的建議是有用的嗎？審閱者是對的嗎？」若仍有疑惑，就要請審閱者來說明這些評論，必說出自己為何認為這樣是好的解法與提供審閱者更多相關資訊。不要把審閱者當成敵人，試著溝通和思考審閱者的建議，最後通常雙方是可以達成共識的。
解決衝突
若發生衝突，試著與審閱者達成共識，若無法達成共識，請參考這裡的建議。
