[譯] 程式碼審查之最佳實踐

原文：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 關注公眾號

轉載請註明出處