[譯] 代碼審查之最佳實踐

原文：https://medium.com/palantir/code-review-best-practices-19e02780015f  
做者：Robert F. (https://github.com/uschi2000)

圖片來自 https://xkcd.com/1513/

本文談論瞭如下話題：

• 代碼審查之爲何、查什麼、什麼時候查
• 準備好被審查的代碼
• 代碼審查的執行
• 代碼審查實例

動機

之因此要執行代碼審查（code reviews），就是爲了籍此改善代碼質量，並向團隊和公司文化中注入正能量。好比：

• 提交者每每會清理未完成的細枝末節、合併 TODOs，或是進行通常性的改進；完成這些後，提交者則期待有其餘審查者對提交的變更進行檢查。讓許多開發者引覺得傲的，正是從同行那裏得到的編碼專業能力讚揚。

• 分享知識會在幾方面上幫到開發團隊：

  • 一次代碼審查能夠將 增、刪、改 等功能性改動清楚明瞭地傳達給團隊成員，以便其開展後續的工做
  • 審查者能夠學習到提交者所使用的某種技術或算法。更歸納的說，代碼審查有助於組織內部的質量提高
  • 審查者可能掌握着可以改善或精簡所提交代碼的編程技術知識或代碼庫；舉例來講，某人也許正好也在開發相似的特性或修復相似的問題
  • 積極的交互和溝通會增強團隊成員之間的社交連結

• 代碼庫中的一致性讓代碼易讀易懂，有助於預防 bug，並能促進開發者之間的合做

• 代碼片斷的易讀性對於將其親手寫出的做者來講是難以判斷的，而對於沒有完整上下文概念的審查者則容易的多。易讀的代碼更容易複用、bug 較少，也更不易過期

• 意外錯誤 (如錯別字) 及結構錯誤 (像是無效代碼、邏輯或算法錯誤、性能或架構上的關注點) 常常更容易被旁觀者清的挑剔審查者找出來。有研究顯示，即使是簡短、非正式的代碼審查也能顯著影響代碼質量和 bug 的出現的頻次

• 合規合法的環境一般須要審查。代碼審查是避免常見安全陷阱的很好途徑

審查什麼

對於這個問題沒有四海皆准的答案，每一個開發團隊都應該達成本身的共識。有些團隊樂於審查向 master 分支的每次合併，而其餘一些團隊有本身的細化標準以判斷是否須要審查 -- 要有效使用工程師（包括做者和審查者）的時間，也要維護代碼質量，得在二者之間兼顧平衡。在某些須要監管的環境中，即使是微小的調整也須要代碼審查。

代碼審覈不分尊卑長幼：做爲團隊中最資深的人也並不意味着其代碼就不須要審查。即使在不多的狀況下代碼真的完美無瑕，審查也向團隊成員和夥伴們提供了至少能從多元化的角度認識庫中的代碼的機會。

什麼時候審查

代碼審查應該晚於自動檢查（自動測試、樣式檢查、持續集成）成功完成後，但要早於代碼合併到倉庫的主線分支以前。一般並不對最後一次發佈前積累的總量改變執行正式的代碼審查。

對於那些應該做爲一個總體被合併到主線分支的複雜改變，只作一次代碼審查彷佛太大了，這時能夠考慮一種堆疊式的審查模式：建立一個基礎分支如 feature/big-feature，以及一些二級分支（如 feature/big-feature-api、feature/big-feature-testing 等等）；這些二級分支各自封裝一個功能性子集，並獨自進行代碼審查；一旦全部二級分支被合併到基礎分支 feature/big-feature，再建立一次爲了把後者合併到主分支的代碼審查。

爲審查準備好代碼

提交易於審查的代碼，以避免浪費審查者的時間和積極性，對於做者來講是義不容辭的：

• 做用域和體積。 全部改變都應該有一個覆蓋面窄、定義良好、自包含的做用域。舉例來講，一次調整多是實現一個新特性，或是修復一個 bug。小調整好過大改變。若是一次代碼審查要處理大量的改變，好比超過 5 個文件、超過一兩天的開發量，或是要花超過 20 分鐘去審查 -- 就要考慮將其分割成數次自包含的審查了。好比開發者可能提交一次根據接口或文檔爲新特性定義 API 的更改，而第二次更改則是依據那些接口增長實現。

• 只提交完成的、自我審查過的（藉助 diff）、自測過的代碼審查。爲了避免浪費審查者的時間，應在將審查指派給他們以前，測試已提交的改變（也就是運行測試套件）並保證其經過全部構建，也要保證全部測試和代碼質量在本地和持續集成服務器上被檢查過。

• 重構時不能改變行爲；相反的，會改變行爲的調整應該避免同時去重構或格式化代碼。這樣作的好處是：

  • 重構常常會影響多行代碼和多個文件，而這些波及之處在審查中容易被忽略。無心的行爲改變能夠神不知鬼不覺的滲透到代碼庫中。
  • 用 cherry-pick、rebase 等施展的版本控制小魔法，遇到大的重構也會玩不轉。想要撤銷一次由於重構而將行爲改變引入到版本庫中的提交是極爲麻煩的。
  • 昂貴的人工審查時間應該花在程序邏輯方面，而不是對樣式、語法或格式的辯論上 -- 那些應該用自動化工具解決掉。

Commit messages

下面是一個不錯的 commit message 示例，來自被廣爲引用的 tbaggery.com/2008/04/19/…

簡短扼要的概述，是必選的一個部分。有些狀況下，首行信息被視爲標題，剩下的部分則做爲正文對待。

可選的更多細節性的描述文字。

用祈使句（用於表達命令、請求、勸告、警告、禁止等的句子）
描述 commit message，
如："Fix bug" ，而不是 "Fixed bug" 或 "Fixes bug"。
用 git merge 或 git revert 等命令生成的
commit message 也符合這個約定。

可用空行分割段落。

- 用列表符號表示段落也不錯
複製代碼

應該同時表述 commit 的 what 和 how：

> 孬
再次編譯

> 好
增長 jcsv 依賴項以修復 IntelliJ 編譯
複製代碼

找到審查者

建議提交者去找一兩個熟悉代碼庫的審查者，其中的一個一般應該是項目領導或高級別工程師。超過兩個審查者就容易由於意見相左而效率底下甚至產生相反效果了，三個和尚沒水喝。

執行代碼審查

一次代碼審查就是一個在不一樣的團隊成員之間同步觀點的過程，天然也有延宕進程的隱患。所以代碼審覈應該麻利些（以小時計而非天），團隊成員和領導也須要優先對待審查並承諾完成的時間。若是你以爲不能按時完成一次審查，請讓提交者立刻知曉，以便另請高明。

一次審查應該足夠完全，也就是審查者能以一個合理的詳細程度向其餘開發者解釋代碼的改變。這確保了代碼庫中的細節能夠被不止一我的所熟知。

做爲審查者，你有責任強制實施編碼規範並保證編碼質量不斷提高。審查代碼與其說是一門科學，不如說是一門藝術。學習它的惟一途徑就是去實行它；一個有經驗的審查者應該考慮讓其餘經驗不足的審查者參與進來，並讓他們優先評審。

假如代碼做者已經遵循了上述原則（特別是自我審查和確保代碼能運行了），這裏還有一份審查者應該注意的清單：

意圖

• 代碼是否達成了做者的意圖？ 每次改變都應該有個明確的緣由（新特性、重構、修復 bug 等等）。提交的代碼是否真的完成了這些目的？

• 問問題。 函數和類的存在應該有意義；當審查者對其意義不明確時，可能就意味着這段代碼須要重寫、註釋或測試了。

實現

• 想一想若是換成你會怎樣解決問題。若是有差異，是爲何？你的代碼能處理更多（邊際）狀況嗎？在功能相同的前提下你的代碼會更短/更容易/更清晰/更快/更安全嗎？當前代碼中有沒有一些未處理的潛在問題被你發現了？

• 你看到了潛在的可用抽象嗎？特別是重複的代碼每每意味着一個更抽象或更通用的功能性片斷能夠被抽取出來，並在不一樣環境中複用。

• 以對手的角色思考，以友善的態度待人。經過找出程序性配置或數據輸入問題等破壞代碼的問題，嘗試着「抓住」做者的懈怠或遺漏。

• 考慮一下庫或既有的產品代碼。當某人從新實現了已有的功能時，多半隻是由於他不知道已經存在的解決方案。有時代碼或功能的重複是出於避免依賴等目的，在這種狀況下，就應該用註釋清晰解釋其意圖。

• 代碼的更改是否遵循了標準的模式？代碼庫每每都有本身的模式，如命名約定、程序邏輯分解習慣、數據類型定義等等。

• 所作更改是否增長了編譯時或運行時的依賴（特別是在子項目中）？咱們但願保持產品的鬆耦合，依賴越少越好。對依賴和構建系統的改變應該事無鉅細的檢查。

易讀性和樣式

• 考慮你的閱讀體驗。 你能在合理的時間內領會相關概念嗎？流程是否健全？變量和方法的命名是否易懂？你在多個文件或函數中能全神貫注嗎？你有沒有被先後不一致的命名弄暈過？

• 代碼是否聽從了編碼規範？ 在樣式、API 約定等方面，代碼是否和項目中保持了一致？固然，上面提到這些，最好仍是能用自動化工具解決掉，以避免各費口舌。

• 代碼中是否還有 TODOs ？ TODOs 若只是堆積在代碼中就只會隨着時間流逝變得過期，尤爲是沒有註釋的 TODO 部分更不該該被接受。起碼來講，做者應該將問題提交到 GitHub Issues 或 JIRA 上以待解決，並將相應單號寫在 TODO 的註釋中。

可維護性

• 讀一讀測試。 若是該有測試的地方卻沒寫，就讓做者去寫。真正不可測試的特性是少數，事實上真正糟糕的常見情況是一些代碼根本沒被測試。在檢查測試時也要注意：它們是否覆蓋了值得關注和容易出問題的狀況？它們是否可讀？審查的部分是否總體覆蓋率較低？另外測試代碼自己的樣式標準常常和核心業務代碼迥異，但也十分重要。

• 本次測試是否引入了新的風險？ 好比破壞測試代碼、破壞臨時堆棧，或破壞集成測試等。和審查相伴的 pre-commit/merge 等常常不被檢查，但出現這類問題會讓每一個人都好受不了。須要特殊關注都事情包括：對測試工具和模式的刪改、對配置的改變，以及人爲的改變項目結構等。

• 本次改變是否破壞了向後兼容？ 若是是的話，是當下就合併更改仍是延遲到下次發佈時再 merge 呢？這種破壞包括了數據庫或架構的更改、公共 API 的更改、用戶工做流的改變，等等。

• 這塊代碼須要集成測試嗎？ 有時，僅靠單元測試沒法充分驗證代碼，特別是代碼和外部系統或配置存在交互時。

• 代碼註釋，以及 commit message。 過於繁複的註釋讓代碼變得混亂，而過於簡單的 commit message 又會讓後來者一頭霧水。儘管不老是可行，但在高質量註釋和 commit message 上的付出老是值得的。

• 外部文檔是否更新了？ 若是你的項目維護了 README、CHANGELOG，或其餘文檔，新的改變是否反映到了這些文檔中呢？過時的文檔比沒有文檔更坑人，等到那時再去修正它比如今就更新它也要花大得多的代價。

不要忘記讚美 簡介/可讀/有效/優雅 的代碼。反之，在一次審查中婉拒或反對也並不是無禮。若改動是多餘或不對的，解釋後拒絕掉就是了。若是你由於一個或多個大的瑕疵以爲此次改動不可接受，那就不要同意，一樣得解釋清楚。有時一次代碼審查的正確結局就是 「讓咱們用徹底不一樣的方法來解決這個吧」 甚至 「乾脆別這麼幹了」。

尊重提交審覈的夥伴。雖然「對手思惟」頗有效，但那畢竟不是你的代碼，你考慮的並不必定那麼周全。若是經過代碼你沒法苟同，那麼面對面交流一下，或是尋求第三方意見或許更有效。

安全性

覈實 API 端與代碼庫中其餘部分保持一致，執行了適當的認證和鑑權。檢查其餘常見薄弱環節，如弱配置、惡意用戶輸入、缺乏 log 事件 等等。若是有疑問，尋求安全專家的幫助。

註釋：簡明、友好、可行

審查者的註釋 應該簡明，而且用人話寫。評論代碼，而不是用做者的口氣。 當有些問題不甚清楚時，詢問後弄清楚好過假設那就是愚蠢的。避免人物之間的比較，帶上評價就更很差：「你改代碼以前個人代碼明明能運行的」、「你的函數有 bug」 等等。避免絕對的判斷：「這樣根本運行不了」、「結果老是錯的」。

嘗試着分清 建議（如 「建議：抽取成方法以增長可讀性」）、須要改變（如 「增長 @Override」），和 澄清（如 「該行爲的確恰當嗎？若是是的話，請增長一個註釋來解釋這個邏輯」）。也能夠考慮提供連接等，以深刻解釋問題。

當你完成一個代碼審覈以後，指明你但願做者在何種程度上響應你的評論，以及是否想要在本次審查出的問題都被解決後從新審查一次（舉例來講，"放輕鬆些，完成那幾個小建議的地方後合併一下就好了" 對比於 "請考慮個人建議，並讓我知道什麼時候能再看一眼"）。

迴應審查

代碼審查的目的之一就是督促做者不斷改進；所以，不要由於審查者的建議悶悶不樂，即使你不覺得然也要嚴肅對待。迴應每一條註釋，即使只是用 「知道了」 或 「已完成」。解釋你作出某些決策的緣由，爲什麼一些函數存在等等。若是你不能和審查者達成共識，線下交流或尋求外部的意見。

修復應該被提交到相同的分支，但要獨立的提交。在審查過程當中就不斷擠進新的提交會讓審查者無所適從。

不一樣的團隊有不一樣的 merge 策略：有些團隊只容許項目擁有者合併，其餘的則容許貢獻者在代碼審查獲得確定後合併代碼。

面對面的代碼審查

對於多數代碼審查，基於 diff 的異步工具，諸如 Reviewable、Gerrit 或 GitHub 是很好的選擇。而在專業和經驗迥異的各方進行復雜的變更或審查時，面對面進行可能更有效；不管是在同一塊屏幕或投影儀面前，或者利用遠程分享屏幕工具，都是能夠的。

例子

在下面的例子中，代碼塊中的建議性審查註釋以 //R: ... 的形式標出：

不一致的命名

class MyClass {
  private int countTotalPageVisits; //R: 變量命名要統一
  private int uniqueUsersCount;
}
複製代碼

不一致的方法簽名

interface MyInterface {
  /** 當 s 沒法被提取時，返回 {@link Optional#empty} */
  public Optional<String> extractString(String s);
  /** 當 {@code s} 不能被重寫時，返回 null */
  //R: 應該統一返回值：都用 Optional<> 
  public String rewriteString(String s);
}
複製代碼

使用庫

//R: 移除這個，並用 Guava 的 MapJoiner 庫代替
String joinAndConcatenate(Map<String, String> map, String keyValueSeparator, String keySeparator);
複製代碼

我的口味

int dayCount; //R: 那誰: 我更喜歡用 numFoo 而非 fooCount; 是你說了算，但咱們應該在這個項目中保持一致性
複製代碼

Bugs

//R: 這裏執行了 numIterations+1 迭代, 是故意的嗎？
// 若是是的話，考慮改變 numIterations 的含義吧？
for (int i = 0; i <= numIterations; ++i) {
  ...
}
複製代碼

架構上的關注點

otherService.call(); //R: 我覺着咱們應該避免對 OtherService 的依賴。咱們當面聊一下如何？
複製代碼

--End--

搜索 fewelife 關注公衆號

轉載請註明出處