跟 Google 學如何 Code Review — (2) Code Review 時要看什麼
在看完《基本原則》後，系列文的第二篇要紀錄的是我們針對「What to look for in a code review」（Code Review 時要看什麼，中文有人翻譯了）的內容進行相關討論。而這也可以說是系列文中數一數二重要的部分。期望在自我成長的同時也能激起讀者們更多想法。
What to look for in a code review
設計 Design
檢查 CL（Change List，程式碼變更，GitHub 中即為 Pull Request）中的整體設計是 Code Review （程式碼審查，以下簡稱審查，負責審查的人稱作審查者）中最重要的一件事。CL 裡各個部分的程式碼運行方式合理嗎？這個改動應放在你的 codebase 還是函式庫（library）裡？它跟系統的其他部分有沒有整合好？現在加入這些 CL 上的功能是好時機嗎？
功能 Functionality
這份 CL 有達到開發者的預期目的嗎？開發者有試圖讓這段程式碼為使用它的人帶來好處嗎？「使用它的人」通常包含最終的使用者（會因改動而受到影響）以及開發人員（未來會引用、使用這段程式碼的人）。
通常，我們希望開發人員能夠充分地測試 CL，以確保它們在審查時能夠正常運作。然而身為審查者你仍舊需要考量到極端案例，尋找並發性問題，試著像用戶一樣思考，並且在閱讀程式碼時確保沒有錯誤。
你可以根據需要驗證 CL — — 對審查者而言，最重要的是檢查 CL 的行為，尤其是使用者感受得到的面向，例如 UI 的更改。有時候光閱讀程式碼很難知道某些改動是如何影響使用者的。對於這類的改動，如果在 CL 中指正修正和自己嘗試都不太方便的話，你可以請開發者演示功能的運作。
另一個在審查時要特別考慮的是，有沒有形成平行計算（parallel computing）進而可能造成 deadlock 或是 race conditions 的情況。這類問題很難僅靠執行功能就發現，通常需要有人（開發者和審查者）仔細思考並確保問題不會發生。（請注意到，這同時也是一個好理由，讓我們不去使用可能會造成 deadlock 或是 race conditions 的並行模型（concurrency models） — — 而且這可能會使審查或是理解程式碼變得非常複雜）。不要輕易地逼死同事。
[討論]
Race condition 也會發生在非同步的事件上，比如一個 React 元件同時發出三個非同步的 API 請求，則 API 回傳順序的不同可能會造成 race condition。這樣的情況我們可以把非同步任務的回傳值的相依性畫出來，以搞清楚先後順序（注意，是回傳的先後順序，請求的先後順序通常是可控的），再利用非同步機制 Promise/async/await 等把這樣的先後順序包裝處理。假設請求為 A, B, C ，而相依性為 C 需要 A 與 B 的回傳值，則可以以 `Promise.all(A, B).then(C)` 之類的非同步處理方式做包裝，確定依照順序執行，減少 race condition。
複雜度 Complexity
CL 有比它應有的樣子來得更複雜嗎？在每個 CL 的層級都應該要檢查 — — 每一行（individual lines）是否太過複雜？函式（functions）會不會太複雜？類別（class）會不會太複雜？「太複雜」通常表示「閱讀程式碼的人很難立即理解」。這也表示「開發人員在引用或修改此代碼時可能會引入錯誤」。
有一種特別的複雜度情況是過度設計，即開發者將程式碼變得比其需要的更通用，或者加了系統目前不需要的功能。審查者應該要特別留意過度設計。鼓勵開發者解決他們現在需要解決的已知問題，而不是解決開發者認為未來可能需要解決的問題。未來的問題應該要在未來來臨時再被解決，而且屆時你可以知道它真正應該被解決的樣貌。
測試 Tests
依據改動適當地要求單元測試、整合測試或端到端測試。一般而言，除非 CL 是處理緊急情況，否則應該在相同的 CL 中加上測試並視其為產品的一部分。
確保在 CL 中的測試是正確、合理和有用的。測試不會測試它們自己，我們很少為測試編寫測試 — — 生而為人必須確保測試是有效的。
當程式碼毀損時測試會失敗嗎？如果程式碼在測試下面更改，它們會開始產生誤報嗎？每個測試都使用簡單且有效的斷言嗎？測試是否有適當地在不同的測試方法之間分開？
記得測試也是必須維護的程式碼。不要僅因它們不是主要功能而輕輕放下那些在測試裡的多餘程式碼。
命名 Naming
開發者有為每個東西取個好名字嗎？一個好名字可以明確地表達它是什麼或是它做什麼用，而且不會長到難以閱讀。
[討論] 命名有多難？
軟體內的概念部分是虛擬的、不存在的（尤其是在抽象層或是底層機制），很多時候需要找到一個對應的、實體的真實對照者，ex: hooks, callbacks. 這考驗著命名者的『英文詞彙』能力，以及對於被命名事物的本質上的理解度，還有他們的聯想力。註解 Comments
開發者有用留下可以理解的註解嗎？所有的註解都是必須的嗎？通常註解在解釋為什麼某些程式碼存在時是有用的，而且不應該用來解釋某些程式碼在做什麼。如果程式碼不能清楚地解釋自己，那麼就應該要再簡化一點。的確會有一些例外（例如正則表達式和複雜的演算法通常能因註解解釋他們在做什麼而受益）但大多數註解是針對程式碼本身可能無法包含的訊息，像是決策背後的原因。
查看此 CL 之前的留言可能也會有所幫助。也許有些待辦事項現在可以刪除、早就有針對此改動提出的意見等等。
請注意，註解與類別、模組或函數的文件不同，註解應該是表達一段程式碼的目的、如何使用該代碼以及該程式碼在使用時的行為。
[討論1] 什麼樣子的情況會留下註解？什麼樣的情況不會？
- 產 API 用的註解
- 髒髒邏輯不太容易懂時會
- 技術債有關的放在註解或是直接開 issue / story
- 非技術性的比如商業邏輯有關的，應該放在 PRD／產品規格中，不會寫在程式碼內[討論2] 註解使用什麼語言？
註解的目的是溝通當下區塊範圍在做什麼。因此有兩個要件：作者能夠完整的表達意思，以及讀者能夠清楚地理解作者的意思。以此前提下最好是以作者最熟悉且讀者們容易理解的語言為主。以我們團隊的話就是中文。>>> 如果註解的重點是溝通意圖，那麼就應該用大家都看得懂、也不會理解錯的語言，降低溝通的成本
2020/03/14 補充：分享網路上的 funny comments 供大家輕鬆一下。
樣式 Style
確保 CL 遵循適當的樣式指南，如果沒有樣式指南時，可參考 Google 的。
如果你想改善樣式指南中沒有的一些樣式風格，請在註釋前面加上「Nit：」（Nitpick）／『非必要』之類文字（看團隊要統一用哪種），讓開發者知道這是你認為可以改善程式碼但不是強制性的選擇。不要僅因為個人風格的偏好而卡住審查的進行。
開發者不應該將主要樣式的更改與其他更改結合在一起。這會使改動變得很難看到，使合併和回溯變得更加複雜，並導致其他問題。例如，如果開發者想要重新格式化整個檔案，請他們將重新格式化的檔案作為一個 CL 發送給審查者，其後再發送另一個關於原本要做的功能改動。
[討論]
大部分語言都有類似 formatter 或是 lint 的工具，通常也都會整合進 IDE，可以做到及時提醒甚至是自動排版的功能。再強制一點則可以做成 git pre-commit hook 強迫一定要過樣式檢查才能發 CL。文件 Documentation
如果 CL 更改了用戶建構、測試、與代碼作用或發布程式碼的方式，請檢查其是否還更新了相關文件，包括 README 或任何生成的參考文檔（甚至是與商業邏輯有關的規格說明書）。如果 CL 刪除或棄用程式碼，請考慮該相關文件是否應一併刪除。如果文件不見了，請記得詢問。
每一行 Every Line
看著每一行你應該要審查的程式碼。有時你可以快速掃過資料文件、自動生成的程式碼或大型資料結構之類的東西，但人工撰寫的類別、函數或程式碼則需要認真檢視。很顯然地，某些程式碼比其他程式碼更需要仔細檢查 — — 這是你必須做出的判斷 — — 但至少你應確保你了解所有程式碼在做什麼。
如果你覺得閱讀程式碼太困難，而且閱讀程式碼本身會使得檢視的速度變慢，那麼你應該讓開發者知道這一點，並等他們把 CL 變得更清晰易懂後再嘗試審查。在看這篇文章的你是個了不起的工程師跟審查者，所以當你請開發者把 CL 變得更清晰易懂的同時，你也在幫助未來的開發人員了解這段程式碼。
如果你了解程式碼，但你覺得不夠資格進行某些部分的審查，請確保在 CL 上有一位夠格的審查者，特別是在安全性（security），並發性（concurrency），可訪問性（accessibility），國際化（internationalization）等複雜問題上。
觀看角度 Context
用不同的觀看角度下查看 CL 通常會很有幫助。通常程式碼檢查工具只會向你顯示有改動的幾行程式碼。有時候你必須觀看整份檔案才能確保改動是有意義的。例如，你可能會看到只加了四行，但是當你看整份檔案時，你會發現這四行在一個有 50 行的方法中，現在的確需要拆分為較小的方法。
從整個系統的角度來觀看 CL 也很有用。該 CL 是在改善系統的程式碼健康度，還是使整個系統更加複雜、測試更少等等？不要接受會降低系統代碼健康度的 CLs。大多數系統都會經由許多小的改動而愈發複雜，因此，在新改動中即使是很小的複雜性都應該要避免。
好的部分 Good Things
如果你在 CL 中看到不錯的東西，請告訴開發者，尤其是當他們以出色的方式回答了你的評論時。審查不只關注錯誤，也應該要鼓勵和讚賞良好實作。在指導時，有時告訴開發者他做對什麼比告訴他做錯什麼更有價值。
摘要 Summary
在做審查時，你應該要確認以下幾點：
- 程式碼是經過精心設計的。
- 該功能對使用程式碼的人很有用。
- 任何介面的改動都是明智的，而且看起來不錯。
- 任何平行計算都安全地完成。
- 程式碼沒有比它應有的還過於複雜。
- 開發者並沒有實作他們將來可能需要，但不知道他們現在需不需要的東西。
- 有適當的單元測試。
- 測試有良好的設計。
- 開發者有為所有東西取個合適的名字。
- 註解是明確、有用的，且通常在解釋為什麼（why）而非做什麼（what）。
- 程式碼有適當地文件化。
- 程式碼有符合樣式指南。
- 檢視每一行你被要求要檢視的程式碼，用不同觀看角度減少盲點，確認你在改善程式碼健康度
- 稱讚開發者他做好的部分。
[討論]
審查的要點很多與 Clean Code 內的概念相呼應，可以看看這本書，更深入的了解為什麼要這麼做。坐而言不如起而行，下一篇《Code Review 練習》我們將用範例來練習當一個審查者。
