代码评审的不可能三角
Code Review 是保證代碼質量的重要手段之一,但許多研發團隊中它常常由于各種原因并未得到真正的落地。為什么會這樣呢?本文希望用一個非常簡單的觀點來理解這個現象,并據此給出一點優化的想法。
觀點
我們的觀點可以用一句話概括,那就是代碼評審非常難同時滿足高覆蓋率、強約束力和低開銷這三個條件。這三個條件分別有什么含義呢?
- 高覆蓋率,意味著評審需要覆蓋項目中幾乎所有的提交,而不是只評審新人的代碼或者是批改暑假作業般的隨機「抽查」。
- 強約束力,意味著在保證評審本身質量的基礎上,評審中指出的問題都需要得到切實的解決,否則不應合并代碼或發布正式版本。
- 低開銷 (overhead),意味著評審不應占用過多寶貴的開發時間,更不應像某些會議那樣提起來就讓人皺眉頭。
論證
滿足上述三個條件的代碼評審,應該是每一位對代碼質量有追求的開發同學都不會排斥的。但為什么我們認為這樣的評審可行性不高呢?簡單地組合一下上述的條件,就不難發現矛盾了。
- 同時滿足高覆蓋率和強約束力的評審,時間是不可控的:為一些強調代碼質量的開源項目貢獻過 non-trivial 代碼的同學,應該都知道即便是一個簡單的 fix,其 PR 都可能因為實現手法和維護者的理解有偏差而長期保持在 Open 狀態(俗稱合不進去),更別說全新的特性與 API 了。
- 同時滿足高覆蓋率和低開銷的評審,很容易流于形式:如果制度上約定必須對全部代碼做評審,又不能耽誤版本進度,那么這時候只要時間稍微一緊,評審就會變成日常回復 LGTM (Look Good To Me) 的走過場了。
- 同時滿足強約束力和低開銷的評審,很難覆蓋到全部的代碼庫:一個版本中通常會有一些全新的特性。如果評審者并未參與這個新特性的開發,那么全量評審一個新特性的上千行代碼,其難度跟打開一個沒有讀過的開源項目并馬上指出其中的 bug 差不多。
折中
如果上述三者不可得兼,我們應該如何權衡呢?在目前的大環境下,多數軟件項目快速迭代的性質與我們對「早點下班」的渴望,使得低開銷這一條件通常很難被犧牲。那么在覆蓋率和約束力之間該如何取舍呢?讓我們回到「代碼評審有什么作用」這個話題上吧。我們知道代碼評審可以:
- 減少代碼中的暗坑
- 提高被評審者的代碼質量
- 讓團隊成員熟悉代碼
如果評審本身就形同虛設,上面這些好處自然也只是空談而已。因此,我們仍然很難放棄對約束力的要求。那么,如何改善這時覆蓋率的問題呢?這里給出兩點不成熟的想法供參考。
首先,來自 Google Subversion 團隊的經驗可以給我們一些啟發:他們將代碼評審與即時通信、會議、文檔一起,視作團隊中的溝通方式,而不是流程。這樣,溝通方式之間就可以取長補短地提高團隊效率。實際上,在評審全新特性時「讀不過來」的問題,就可以通過設計階段的文檔來緩解:文檔與評審同樣是一對多的溝通,并且對文檔中方案的討論顯然比直接討論細節要更容易。一個需要 2 周時間左右開發出的全新特性,按照 問題定義 → 基本思路 → 實現概述 → 改進優化 的結構化方式編寫的文檔,其長度應該僅在千字左右,編寫文檔所需時間與開發時間應當不在一個量級,還能夠節約在缺失文檔時向其他同學當面溝通該特性的時間(當然,對那種順手就能搞定的需求也要求文檔化,就有些繁冗了)。
另外,代碼評審的覆蓋率問題,還可以通過一定的提交約定來優化。在筆者翻譯的 conventional-commits 規范中,每一次提交都可以通過形如 fix / feat / chore / refactor 的不同類型來做區分,來達成細粒度的可讀提交歷史。那么,在評審的 Pull Request / Merge Request 粒度上,為什么不能同樣地應用該規范呢?如果我們按照這種方式區分了 PR 類型,這里就有不少的想象空間:
- 可以首先將評審的資源集中在 refactor 與 perf 一類 PR 的評審上。
- 對于 feat 類型存在大量新代碼的 PR,只需其提供了確保團隊成員理解的文檔,那么就只需要保證方案設計可接受,做保證代碼風格、命名、路徑等隱式約定一致級別的評審即可。
- 我們可以選擇性忽略測試階段可能數量眾多的常規 fix 類 PR,但對版本發布后補充的 hotfix 類 PR,仍然需要評審。
- 對不影響代碼質量的 chore 與 doc 類 PR 可以忽略評審。
總之,代碼評審是一種溝通方式,希望它能夠成為團隊日常開發「文化」的一部分,而非束縛效率的死板流程。希望本文的想法對同樣被評審困擾的同學有幫助 :)
P.S. 我們 base 廈門的前端工具(編輯器)團隊缺人中,有意戳這里了解詳情哈。
總結
以上是生活随笔為你收集整理的代码评审的不可能三角的全部內容,希望文章能夠幫你解決所遇到的問題。
- 上一篇: apache应用进阶
- 下一篇: 低开销、简单的网站监控工具Checkle