我們是怎麼做Code Review的

標籤：

前幾天看了《Code Review 程式員的寄望與哀傷》，想到我們團隊開展Code Review也有2年了，結果還算比較滿意，有些經驗應該可以和大家一起分享、探討。
我們為什麼要推行Code Review呢？我們當時面臨著代碼混亂、Bug頻出的狀況。
當時我覺得要有所改變，希望能提高產品的代碼品質，改善Team Dev面臨的困境。並且我個人在開發上有很多經驗，也希望這些知識能夠在團隊內傳播。
各種考慮後，我們最後認為推行Code Review能改善或解決我們面臨的很多問題。

這篇文章的目的不是告訴大家怎麼在一個團隊內推行Code Review，首先因為我個人僅在一家公司內推行過，並沒有很多經驗。
其次每家公司、每個團隊的情況都不太一樣，應該根據公司或團隊的實際情況選擇恰當的方案，並根據成員的反饋來及時調整，推動Code Review的實施。
所以，本文是介紹我們公司是如何實施Code Review的，我們是如何解決我們遇到的問題的，希望我們的經驗能給大家帶來些協助。
行文倉促，如有遺漏或錯誤，歡迎指正。

一、流程和規則

經過簡單的對比、試用，我們最後採用了Git Flow+Pull Request（PR）模式來做Code Review。（PR模式詳情可參見 Git工作流程指南：Pull Request工作流程）

Pull Request(PR)簡單的說就是你沒有許可權往一個特定的倉庫或分支提交代碼，你請求有許可權的人把你提交的代碼從你的倉庫或分支合并到指定的倉庫或分支。
由於PR需要有許可權的人確認，所以非常適合在這個過程中做Code Review，是否接受或者拒絕就取決於Code Review的結果。
在支援PR模式的軟體裡，每一個PR都有一個新增代碼的對比（diff）介面。
代碼審核者可以線上瀏覽請求合并的新增代碼，並針對有疑問的程式碼添加評論，通過這種方式來實現Code Review。
評論可以被所有有許可權查看倉庫的人看到，每個人都可以回複任何人的評論，有點像論壇裡某個文章的討論。
這種模式是事後審核，也就是代碼已經提交到了中心倉庫，Review過程中頻繁的改動會造成曆史簽入記錄的混亂。
當然Git可以採用更改記錄來解決這個問題，由於容易誤操作，我們一般只在基礎類庫這類要求比較嚴格的項目上實施。

我們所瞭解到的支援PR模式的軟體都採用Git作為原始碼版本控制工具，所以我們的原始碼版本控制工具也遷移到了Git。
由於Git太靈活了，因此誕生了很多的Git流程，用來規範Git的使用。
常見的有集中式工作流程、功能分支工作流程、Gitflow工作流程、Forking工作流程、Github工作流程。
我們對Git Flow做了些調整，調整後的流程被命名為Baza Flow，定義見後文。
根據Baza Flow，我們大部分倉庫只定義了2個主幹分支，master和develop。(例外，我們有一個倉庫有3個開發小組同時進行開發，定義了4個主幹分支，目前還比較順暢，再多估計主幹分支之間的合并就比較繁瑣了。)
master對應生產環境代碼，所有面向生產環境的發布來源都是master分支的代碼。develop則對應本地測試環境的代碼。
絕大多數情況下，QA（測試）只測試develop分支和master分支的代碼。

由於開發人員都在一個團隊內，所以我們沒有採用基於倉庫的PR，採用的是基於分支的PR。
我們對主幹分支的操作許可權做了限制，只有特定的人才能操作，develop分支是項目開發Leader和架構師，master分支是QA。
有許可權往主幹分支合并的成員會按照約定的規則來執行合并，不會合并沒有完成審核的PR。
上面這點其實蠻重要的，所以我們會對有許可權合并的人有特別的約定，在什麼情況下才能合并代碼。（見後文PR的說明）
PR的發起人要主動的推動PR的審核，Leader也會密切關注PR審核的進度，在需要的時候及時介入。

我們配置了CI伺服器（什麼是CI）只編譯特定的分支，通常是develop和master分支。
所有的代碼合并到了主幹分支之後，都會自動觸發編譯和本地測試環境的發布，QA無需依賴開發人員編譯的代碼來測試，也無需自己手工操作這些，保證了開發人員和測試人員的相互獨立。
我們本地測試環境的發布包含了資料庫和網站的發布，全自動的，發布完成以後就是一個可用的產品，有時間這部分也可以分享一下。

我們還使用了Scrum裡面一個很重要的概念：完成定義。
就是我們規定了我們一個任務的完成被定義為：代碼編寫完成，經過自測，提交的PR經過審核並且合并到主幹分支。
也就是說，所有的代碼被合并到了主幹分支之後任務才算是完成，而被合并到主幹分支必須要經過Code Review，這是強制的。

 

Baza Flow

目前的版本 V0.9

Baza Flow 由 Git Flow 演化而來，Git Flow的開發模式如所示：

由於我們的託管軟體對於Pull Request的限制，我們對Git Flow做了改動，改動的地方有：
1、每一個大功能我們會建立一個單獨的feature分支，項目開發人員基於這個單獨的feature分支建立自己的任務分支。
      比如，對於CS 2項目來說，啟動的時候分支的建立是：master -> develop -> feature/v2。
      開發人員應該基於這個大特性分支feature/v2來建立自己的任務分支，比如建立XXXX，可以用一個單獨的分支feature/v2-xxxx。
      完成這個任務以後，立即向上遊分支（feature/v2）提交pull request。然後從feature/v2-xxxx 建立自己的下一個任務分支，比如YYYY編輯 feature/v2-yyyy。
      請注意，合并到上遊分支的功能必須相對獨立而且是可用的，分支任務工作量0.5-1個工作日，不宜超過2個工作日，超過2個工作日不向上遊合并，需要向團隊解釋。
      代碼經過Review以後，可能會進行必要的修改，修改在原分支修改，修改完畢代碼合并進上遊分支，原分支會定期刪除。
      項目群組成員在收到合并成功的通知後，請自行從上遊大特性分支向下合并到自己當前的開發分支。
      提交pull request後建立新任務分支的時候務必知會一下相關配合約事（比如前端的同事），讓他們在新的分支上繼續開發。

2、對於小功能，預計在0.5-1個（不超過2個）工作日工作量的開發工作單位，直接基於develop分支建立特性分支即可。

3、在各個分支遇到的bug，請基於該分支建立一個Bug分支。
      如果在缺陷跟蹤管理系統上有對應的項，命名請使用缺陷跟蹤管理系統的ID，比如BAZABUG-1354 比如這個Bug的分支命名就是bugfix/BAZABUG-1354。
      如果在缺陷跟蹤管理系統上沒有對應的項，命名請簡短的說明修改內容，比如“JX 9df2b01 引用bootstrap css虛擬路徑重寫，避免出現字型無法找到的問題”，分支命名可以是bugfix/miss-font。
      完成修改以後提交並推送到中心倉庫然後立即向上遊分支提交pull request。
4、發起pull request以後，請將pull request的連結在IM上發給代碼審核者，以此通知對方及時進行審核。

二、執行

我們在團隊內部提倡品質優先，Team Dev不能為了進度犧牲品質，並在團隊內部達成了共識。
所以，無論進度有多麼緊迫，Code Review的過程都一定會做。
所有的問題一定會被提出，只是會根據進度的緊迫程度，以及問題的大小，改動成本，決定問題是現在解決，還是加一個TODO，並記錄在缺陷跟蹤管理系統內，以防日後遺忘。
多數情況下，我們都會要求立即解決，哪怕因此造成了發布的延遲。
我們深知，其實多數情況下，現在不解決，日後不知道猴年馬月才能解決。

我們在團隊內推行Code Review的過程中沒有遇到太多阻力。
原因大概有兩點，首先管理層方面瞭解之前遇到的各種問題，也迫切希望能有所改善，所以從一開始就是支援的態度。
其次，絕大部分開發人員覺得在這個過程中能自己能學習到東西，並沒有抵觸，遇到很好的意見時大家都還是很高興的。
最後，慢慢的形成了一種氛圍，整個團隊都會自覺的維護它。
附一張我們審核的對話圖，這位童鞋嘗試對系統內部散落各地發業務郵件的代碼做一個整理，用一套模式來處理，調整了3版才定調，然後修改了很多細節才通過了合并，前後大概用一個多星期時間：

表面上看來Code Review會延緩項目的進度，但是在我們2年多的執行過程中，大多數時候沒感覺到有延緩。
原因是，雖然代碼合并的周期變長了，但是由於代碼品質提高了，導致Bug變少了，由於Bug引起的返工問題也變少了，因此整體的進度其實並沒有延緩。
我個人認為對一個成熟的團隊其實做Code Review反而會加快整體的項目進度，但是手頭上沒有統計資料支撐我的觀點。（對於軟體開發的度量，歡迎有心得的同學告知我）

我們每個分支有許可權合并的人都不止一個，這樣可以保證有人請假不在的時候，代碼仍然可以被其他同事審核通過之後合并。

半年前，我們團隊加入了很多新成員，剛加入的新同事對規範、項目、產品的熟悉程度都不高，導致了有一段時間，我們遇到了PR審核周期變長的問題。
加上之前遇到的一些問題，我們總結了一個說明，目的是減輕Code Review對開發人員工作的負擔，加快PR審核通過的過程。
說明如下： 

Pull Request 的說明 

任務完成才能提交PR。
PR應該在一個工作日內被合并或者被拒絕。
PR在有嚴重問題（包括但不限於架構問題、安全問題、設計問題），太多問題，或者任務無效的情況下會被拒絕。
嚴禁一個PR裡面有多個任務，除非它們是緊密關聯的。
PR提交之後只允許針對Review發現問題再次提交代碼，除非有充足的理由，嚴禁在同一個PR中再次提交其它任務的代碼。 

提交PR時候有一個描述框，內容會自動根據Commit的message合并而成。
切記，如果一次提交的內容包含很多Commit，請不要使用自動產生的描述。
請用簡短但是足夠說明問題的語言（理想是控制在3句話之內）來描述： 

你改動了什麼，解決了什麼問題，需要代碼審查的人留意那些影響比較大的改動。
特別需要留意，如果對基礎、公用的組件進行了改動，一定要另起一行特別說明。

 審核人員邀請原則： 

1. 在建立PR時，Reviewers（審核人）一欄裡主要填寫“必需審核人”。只有這些人審核都通過，才允許合并。
2. 除了“必需審核人”外，還有一些其它審核人，我們可以在Description裡做為“邀請審核嘉賓”@進來。
3. 主幹分支間的合并，如Develop => Master，或Master => Develop等，則需要把整個團隊（開發+QA）都列為“必需審核人”。 

必須審核人的列表由團隊決定，可能包括以下人選： 

• 團隊Leader
• 前端架構師（如果有前端代碼改動） （可以授權）
• 後端架構師（如果有後端代碼改動） （可以授權）
• 產品架構師
• 對此PR解決的問題比較熟悉的（之前一直負責這部分業務的同事）
• 此PR解決的問題對他影響比較大（比如認領的任務依賴此PR的同事）

 其它審核人，包括但不限於： 

需要知悉此處代碼改動的人但又不必非要其審核通過的同事
可以從這個PR中學習的同事 

可以授權指的是，根據約定，Bug修複之類的改動，或者影響較小的改動，前端架構師和後端架構師可以授權團隊內的某個資深開發人員，由這個資深開發人員代表他們進行審核。
主幹分支之間的合并，大型Feature的合并，前端架構師和後端架構師需要參與。 

上述審核人關注的視角不太一樣：
團隊Leader關注你是否完成了任務，前後端架構師關注是否符合公司統一的架構、風格、品質，產品架構師從整個產品層面來關注這個PR。
熟悉此問題的同事可以更好的保證問題被解決，確保沒有引入新問題。
被影響的同事可以及時瞭解他受到的影響。 

團隊Leader或者產品架構師如果覺得PR邀請的審核者不足或者過多，必須調整為合適的人員，其它同事可以在評論中建議。

三、收穫

我們團隊實施Code Review收穫不少，總結出來大概有以下幾點：

1、短期內迅速提高了代碼品質。
     原因有幾個，大家知道自己的代碼會被人審核之後寫得會比較認真。
     理論上代碼品質是由整個團隊內最優秀的那個人決定的。
     大家也能在Review的過程中學習到其它同事優秀的編碼。

2、Bug數量迅速減少。
    但是這個我們沒有資料統計比較，比較遺憾。
    我和QA聊過，他給我的資料是在我們的一個新項目每2周一次的大發布，平均只會發現1~2個Bug。
    這點提高了整個團隊的幸福感，大家不用經常被火燒眉毛。

3、團隊成員對項目的熟悉程度會比較均衡。
    新同事通過參與Code Review能很快熟悉團隊的規範。
    代碼不會只有個別人瞭解、熟悉，Bug誰都能改，新功能誰都能做。
    對公司來說避免了人員的風險，對個人來說比較輕鬆（誰都能來幫你），可以選自己喜歡的任務做。

4、改善團隊的氛圍
    Review的過程中會需要非常多的溝通，多溝通能拉近團隊成員的距離。
    並且無論層級高低，大家的代碼都是要經過Review的，可以在團隊內營造一個平等的氛圍。
    每個成員都可以審查別人的代碼，這很容易激發他們的積極性。

亮一下我們的資料:

我們從2014年1月17日開始第一個PR的提交，到2016年7月5日一共發出了6944個PR，其中6171個通過，739個拒絕。日均11.85個PR，最多的一天提了55個PR。
這些PR一共產生了30040個評論，平均每個PR有4.32個評論，最多的一個PR有239個評論。
參與上述PR評論的同事一共有53位，平均每位同事發出了539個評論，最多的使用者發出了5311個評論，最少的發了1個（剛推行Code Review就離職的同事）。
需要說明一下，只有簡單的問題會通過評論來提出。比較複雜的，比如涉及到架構、安全等方面的問題，其實都會面對面的溝通，因為這樣效率更高。

四、總結

雖然有合適的工具支援會更容易實施Code Review，但它本身並不特別依賴具體的工具，所以前文並沒有具體指明我們用了什麼工具，除了Git。
原因是基於分支的PR流程依賴於大量建立分支，而Git建立一個分支非常的簡單，所以PR模式+Git是一個很好的搭配。
我們在切換到Git之前，也做Code Review，採用的是提交代碼以後把commit的Id發給相關同事來審查的流程。
審核通過以後會在缺陷跟蹤管理系統裡面評論，QA同事沒見到審核通過的評論就認為任務沒有完成，拒絕進行測試。
雖然沒有現在這樣直接方便，但是也還是做起來了。

PR審核的過程中，新加入的團隊成員常見的問題是不符合代碼規範之類的，其實是可以通過原始碼檢查工具來解決的，這部分我們一直在計劃中（( ╯□╰ )），並沒有開始實施。

我們是怎麼做Code Review的