Google 如何進行 Code Review - 2
一窺 Google 內部程式審核的指導原則 - What to look for in a code review
文章目錄
- 在考慮以下要點時，請務必連同 The Standard of Code Review 一起考慮。
程式審核裡最重要的一環是 CL 的整體設計
CL 內多個程式片段的改動之間的交互作用是否合理? 改動是屬於程式庫 (codebase) 還是函式庫 (library)? 此次改動是否能和系統其他部分很好地整合? 現在是加入該功能的適當時機嗎?
CL 是否真的有符合開發者意圖? 開發者的意圖對於使用者們是好的嗎? 這裡使用者通常係指終端使用者 (受到改動而影響)與開發者(未來會用到該程式的人)雙方。
在大多數情況，我們會期望開發者徹底測試他們的 CL，並期望在接受審核時它能夠正常運作。但身為審核者你仍必須去思考可能的「極端案例 (edge cases)」、找出「併發問題 (concurrency problem)」、 嘗試「作為一個使用者去思考」，並且確保「在單純閱讀程式碼時沒有明顯的程式缺陷 (bug)」。
願意的話你也可以去親自去驗證 CL - 如果當改動會帶來直面使用者的影響 (user-facing impact) 時，比方說 UI 改動，驗證 CL 的變化便會是審核者在整個過程中最重要的事情。 單純閱讀程式碼有時很難體會它會如何影響使用者，面對如此的改動如果覺得不方便自己測試的話，可以要求開發者展示 (demo) 此次改動的功能性。
另外，審核功能性時格外重要的一點是，要注意 CL 中是否有「平行程式設計 (parallel programming)」的存在，並是否在理論上會造成 deadlocks 或 race conditions。 這類的問題較難透過執行程式來查覺，通常會需要有人 (開發者跟審核者) 一起仔細思考，確保該問題不會被引入程式庫內。
Note: 這也是不使用可能會造成 deadlocks 或 race conditions 的併發模型 (concurrency model) 很好的理由 - 它可能會讓程式審核或理解上造成困難。
一個 CL 是否複雜到超過原本所必須的? 針對任何層級的 CL 請務必確認這幾點 - 單行程式是否過於複雜? 函式是否過於複雜? Class 是否過於複雜?
「複雜」通常代表著該程式很難快速被閱讀程式的人所理解
也意味著很有可能其他開發者在呼叫或修改這段程式時引入缺陷
其中就是一種複雜性便是「過度設計 (Over-engineering)」，開發者會讓程式過度通用化 (more generic) 超過它原本所需的，或是新增現在系統不需要用到的功能 [1]。
審核者對於必須非常警惕過度設計的發生。
鼓勵開發者解決他們
現在需要解決的問題，而非那些猜測未來可能會需要被解決的問題
可以在真正面臨到那些未來的問題時才解決它們，因屆時你才能更清晰看到問題的樣貌 (actual shape) 與需求。
- [1] 個人認為和 Donald Knuth 說得「過早最佳化是萬惡的根源 (Premature optimization is the root of all evil)」有類似意思。 自己在開發過程中也常常落入相同窠臼，認為未來可能會需要所以現在先寫起來放。但更常見的狀況是，當實際面臨問題時，狀況可能和原先大相徑庭導致需要重寫。
請將要求單元、整合、端到端測試視為要求 CL 所做的適當變更。一般 CL 內除 production code 以外，測試也應該要被加入其中。 除非該 CL 是為了處理某個緊急事件而存在。
另外，也要確保測試是「正確 (correct)」、「合理 (sensible)」、「有用的 (useful)」。 測試並非來測試他們自己本身，一般也極少為了測試寫測試 (write tests for tests)，因此我們必須確保測試是有效的。
當程式真的出問題時，測試是否真的會失敗? 如果被測試的程式發生改動時，測試是否會產生誤報 (false positives)? 每個測試是否有做出簡單而有用的斷言 (assertions)? 不同測試方法之間的測試是否適當分開?
請記住，測試也是必須維護的程式。不要因為它們不包含在 main binary 內，
而接受測試程式中包含不必要的複雜性。
開發者是否為每個東西挑選了恰當的名字? [1]
一個好的命名意謂著
長到足以完整表達該東西的作用或做什麼
同時不要長的難以閱讀。
開發者是否用可理解的英文留下清晰的註釋? 這些註釋是否真的必要?
通常註釋在解釋為什麼 (explain why) 這段程式存在 (code exists) 時相當有用，而不應該去解釋某段程式正在做什麼 (not be explaining what some code is doing)。
如果程式碼本身不能清楚解釋自己的話，意味著它需要更加簡化 (made simpler)。當然也有例外狀況，比如解釋正規表達式 [1] 或複雜的演算法正在做什麼的註釋往往相當有用，
但對於大部分註釋來說是用來說明那些不包含在程式本身的資訊，比方說為什麼決定這樣做背後的理由 [2]。
查看該 CL 之前的註釋也很有幫助。或許有個 TODO 項目現在可以移除、一個評論建議為什麼不要進行這種改變等等。
要注意的是，註釋與 class、模組 (module)、函式 (function) 的文件不同。後三者要能夠表達一段程式的目的、如何使用它、以及它在使用時的行為。
- [1] 請見先前關於 Cloudflare 全球大斷線的文章
- [2] 常見的狀況是基於
商業上決定，而非程式面因素
Google 對於主要語言皆有提供風格指南，甚至包括大多數的冷門語言，因此請確保 CL 遵循適當的指南上的說明。
如果你想改進 CL 中某些不包含在風格指南中的風格要點時，請在評論前加上 Nit: 讓開發人員知道這是你認為可以改善程式的小問題且並非強制性的。
但記住，不要僅根據個人風格偏好阻止提交 CL。
開發者不應在 CL 內同時包含主要風格的改動與其他程式的更改 [1]，這樣會導致難以看出 CL 到底做出什麼改動。同時也會讓合併 (merge) 和回滾 (rollback) 更為複雜，並產生其他問題。
例如，如果作者想要重新格式化 (reformat) 整個檔案的話，讓他們將重新格式拆成一個 CL，然後再發送另一個包含功能修改的 CL [2]。
- [1] 比方說 CL 內包含上百個更改縮排的文件，中間額外穿插程式的異動
- [2] 通常 revert 時會希望 commit 越小越好，如此能更易於回滾，請參考 Atomic Commits with Git
如果 CL 更改用戶如何「構建 (build)」、「測試」、「交互 (interact)」、「發布」程式的方法時，請檢查是否也同時更新相關文檔， 包括 READMEs，g3doc 頁面和其他生成 (generated) 的參考文件。如果 CL 刪除或棄用 (deprecate) 程式，請考慮是否應該刪除對應文檔，如果缺少文檔時請詢問。
仔細查看你被分配審核的 CL 的每一行程式
有些東西，比如資料文件 (data files)、生成的程式 (generated code)、大型資料結構 (large data structures)，你可以稍微掃過。
千萬不要在掃過開發者寫的 class、函式、程式區塊 (block of code) 時，並假設其內部的內容是沒問題的。
很顯然的某些程式需要比其他程式更仔細的審查 (scrutiny) - 這是必須由你做出的判斷 (judgment call)。但至少你應該確定你理解所有程式在做些什麼。
如果閱讀程式過於複雜並且減慢審核速度時，那麼你再繼續審核前要讓開發者知道這件事，並等待他們為程式做出解釋、澄清 (clarify)。 在 Google 我們聘請許多優秀的軟體工程師，而你也是其中的一員。如果連你也無法理解程式的話，很可能其他人也不會。 因此，你要求開發者去澄清此程式時，同時也在幫助未來的開發人員理解這些它們。
如果你能夠理解程式但覺得沒有資格進行某部份的審核，請確保 CL 審核者中有一個適合 (合格) 的審核人來審核該部分。尤其是針對「安全性 (security)」、 「並發性 (concurrency)」、「可訪問性 (accessibility)」、「國際化 (internationalization)」等複雜問題時。
在充足的前後文 (broad context) 下查看 CL 通常很有幫助。
一般來說，程式碼審查工具只會顯示修改部分周圍的幾行程式而已。但有時你必須查看整個文件以確保改動是否合理。 比方說，你可能只看到添加 4 行新程式碼，但實際上查看整個文件時會發現這 4 行是被加在有 50 行的函式裡，此時便需要將它拆解為更小的函式。
以整個系統的角度出發來思考 CL 也是很有用的。該 CL 是否改善整體系統的程式品質，亦或是會讓整個系統更加複雜? 是否缺少測試?
千萬不要接受會降低整體系統程式品質運行狀況的 CL。因為大多數系統是由於許多小改動的積累而變得複雜，因此阻止新的改動引入複雜性 (儘管很小) 也非常重要。
當你在 CL 裡看到優點時記得告訴開發者，尤其是當他們用很棒的方式來解決你的評論時。
程式審查通常只關注在錯誤，但它們也應該同時應該為良好實踐 (good practices) 提供鼓勵與欣賞。
這點在指導 (mentoring) 開發者時尤其重要: 比告訴告訴他們做錯了什麼，更好的是告訴他們做對什麼 [1]。
- [1] 個人認為並非不要指出錯誤，而是「多以鼓勵來代替指出錯誤」，讓其他開發者更有動力想將事情做好。 其實透過簡單的一句話讓對方知道哪裡做得很好，未來他們會將繼續保持下去，並為其他開發者帶來的正面影響
對方註明每個 commit 修改什麼，可以幫助審核者快速進入狀況 此時就不要吝於給出你的稱讚吧!
在進行程式審核時，請務必確保
- 程式經過完善的設計
- 功能性對於使用者們是好的
- 對於任何 UI 改動要合理且合眼 (look good)
- 任何平行程式設計 (parallel programming) 的實現是安全
- 程式不應該複雜超過原本所必須的
- 開發者不該實現一個現在不用而未來可能 (might) 需要的功能
- 程式有適當的單元測試
- 測試經過完善的設計
- 開發者對於每樣東西有使用清晰、明瞭的命名
- 註釋要清楚且有用，並只用來解釋「為什麼存在 (why)」而非「做什麼 (what)」
- 程式有適當的寫下文件 (一般在 g3doc)
- 程式符合風格指南
確保你查看被要求審核的每一行程式碼、確認上下文 (context)、確保你正在改善程式品質，並讚揚開發人員所做的好事與優點吧!
相關文章
- Google 如何進行 Code Review - 6
- Google 如何進行 Code Review - 5
- Google 如何進行 Code Review - 4
- Google 如何進行 Code Review - 3
- Google 如何進行 Code Review - 1
文章內容的轉載、重製、發佈，請註明出處: https://tachingchen.com/tw/
Twitter
Google+
Facebook
Reddit
LinkedIn
StumbleUpon
Pinterest
Email