程式碼審查過程

drowzju發表於2015-03-18

對我而言,把程式碼產品化而沒有合適的審查流程,就像是一場抽抽樂遊戲。程式碼當然也有可能會挺好,不過總還是有一定概率某人的哪塊積木沒抽好,然後一切就轟然崩塌。無論是採用持續整合服務、結對審查、QA審查,還是所有這些方案的組合,都可以大大降低引入風險的概率。

程式設計團隊規模已經超過了你的掌控能力

我在Think Through Math(下文簡稱 TTM)的工作有18個開發者組成的工程團隊,其中有兩位是QA專家。這算是有挺多人(相對而言)在同時編碼了。我們的架構中有好幾種不同的服務,所以不是每個人都能對程式碼的每個角落了如指掌。我們中有些人偏重於前端的工作,有些人側重於資料倉儲和報表,還有些人則在後端折騰Ruby程式碼。我們都會經常重新搭配分組以相互傳播知識,不過始終還是有相當多的人在為不同的專案工作,而沒有一個人能把整個系統吃透。

我知道在這種事上我們並不孤單——有其他開發者也處於差不多的情況。一些人只是按部就班開發著應用,一些人則嘗試把龐大的Rails應用拆分成小粒度的服務,還有一些人在遺留程式碼拯救專案。不管是什麼型別的專案,都可以從某種恰當的程式碼審查流程獲得很大收益。對按部就班的開發專案而言,可以確保做修改,新增新特性時一切正常;對大型系統拆分而言,是可以在程式碼質量改進並正確分解的時候確保每個新服務都仍保持功能;而對於拯救專案,則是可以確保程式碼適配新標準時,所需的功能都正常。更棒的是,流程合理的話這些都可以自動完成。

TTM當前審查流程

在我們TTM的整個審查流程的迭代中,我不記得有哪次是不好使的,不過總有一些比其他更有效。最新的一代似乎是目前為止最好的(如人所願),不過仍然存在改進空間。下一篇帖子裡我會很樂意解釋我們針對審查流程達成共識和改進的新方法,不過這不在本帖討論範圍。下面是我們當前審查流程執行的要點:

程式碼審查流程

首先,開發者(通常是一組結對),完成了bug修改,新特性或重構,把程式碼提交給Github。如果他們只是想要CI伺服器在他們提交的程式碼分支執行測試套件,他們會打上我們的WIP(半成品)標籤,這樣其他開發者和機器人就知道這次什麼都不用做。這時(還有每次程式碼提交PR時),我們的Hound伺服器會執行來檢查我們的程式碼風格,以確保提交的程式碼的風格滿足我們所選用的程式碼風格。

程式碼一旦就緒,開發者就為PR打上“需要程式碼審查”標籤(Needs Code Review label)。他(她)還會附上完備的清單形式的關於QA該如何檢測PR的指南。在預設的時間以後,我們的能幹的聊天機器人,mathbot會來檢查帶著“需要程式碼審查”標籤”的PR(Pull Request),給它們分配一兩名其他的開發人員來審查程式碼。若是某個程式碼審查者覺得審查自己不熟悉領域的程式碼不太舒服,他們只需在聊天室裡請求熟悉相關程式碼的人來替換。

我們通常會要求開發者仔細檢視程式碼,而不是簡單地做個人肉 lint 工具或者編譯器。從我們的流程定義頁面照搬的對審查風格的要求是:

  • 保持好奇,審查PR的作者為何如此實現;
  • 查詢邏輯上的瑕疵,寫反了的布林判斷,潛在的物件的錯誤選擇,型別不匹配;
  • 尋求程式碼命名和動機的清晰;
  • 尋找可以優化的地方。太細的優化並不常需要,主要是能夠避免的東西如:

迴圈內部的偶發DB呼叫
迴圈內部可以避免的繁重工作
頻繁使用和代價高昂的操作,卻沒有使用快取或記錄的機制

如果審查者或其中一人認為程式碼可以調整,他們就會在Github的PR新增簡短評論,開發者就能獲得一個很清楚的標註,說那些地方有問題。之後審查者移除“需要程式碼審查”的標籤並替換成“需要修改”的標籤,提醒開發者關注評論,並修改實現,或者申辯這樣編寫程式碼的原因。

有一件重要的事情需要注意,這裡所有的評論都是以尊重和積極的態度完成的。我們不是為了對別人評頭論足。我們是為了幫助彼此儘可能寫出最好的程式碼,讓整個系統更完善。自我提升是我們整個團隊共有的目標,作為一個學習型的團隊,我們總是在技術和實踐上互相幫助。

要求的修改完成後,或者程式碼被證實合理後,開發者移除“需要修改”標籤,重新標記為“需要程式碼審查”,這樣審查者就可以繼續幹活兒了。一旦他們都簽發(sign off)了本次改動,“需要程式碼審查”標籤就翻成了“完成程式碼審查”,也就是不需要再做程式碼審查了。如果不需要使用者體驗評估,那麼”需要QA”標籤就會打上。

使用者體驗評估

我們的產品有廣大受眾——學生、學校的教職員、管理員等。所以我們非常關注我們版本的變化和使用者介面/體驗的變化。據此,一旦我們的系統前端做出了任何可能使站點上某項外觀變化的改動,我們都需要為PR加上“需要UX”標籤,以使UX小組的某人可以審查我們的變化。因為UX要改變通常也影響到程式碼,所以這個步驟一般和程式碼審查流程並行完成。如果需要在再修改,審查者移除標籤並設定為”需要修改“,如同之前程式碼的流程。一旦都好了,他們會移除”需要UX“標籤並帶上”需要QA“標籤。

QA審查

QA小組很小,不過職責重大,要確保我們的程式碼滿足業務需求,別上來就崩潰了。我們使用專案管理工具把PR連結到故事,做QA工作的人進行審查時,就可以快速地參考相應的驗收標準,以及目標和偏差。他們使用開發者提供的關於測試內容的說明,動用他們自己的能力嘗試去擊垮系統。如果有什麼地方不對了或者沒有滿足驗收標準,他們還是移除所有標籤,加上”需要修改“標籤,這樣在開發者實施修改之後幾乎總是會強制帶來新一輪的程式碼審查(通常只是修改部分)。當所有事情都做完,執行良好,程式碼會得到一個”Passed QA“標籤,族中何如我們的發行候選分支。到此程式碼會最終提交給專案master,並在QA和產品團隊決定發行版中應該包含的特性和bug修復後提交產品化。

如果這些標籤的玩意讓你迷惑,請看下面的流程圖。

為什麼?

流程看起來很多,其實在真實世界裡使用起來真的算極其簡單了。如果是一次小審查,一個需要儘快完成的特性,整個工作流甚至可以在十分鐘內完成。不過通常來說時間會長些,並沒有那麼急。一切都自然地協作,這套流程確保了我們有三類不同的人群,從不同視角看待我們的程式碼,確保它有很高的質量並完成應有的功能。這有助於保持我們的程式碼庫和測試的整潔、可讀,以及自我文件化。

好處可不只是你有了一個流程。我們釋出更少的bug,我們對進入程式碼庫的程式碼滿懷喜悅,而且還有人幫助我們每天提高。我無法強調有多少次第二雙眼睛阻止了我程式碼中愚蠢的錯誤或效能下降。還有QA,在我沒有檢查的時候,他們救了我無數次。我從未覺得有評論或者批評是帶著怨恨或者審判心態的,它們只是坦率的呵護,為了保持程式碼整潔,為了幫助我修正錯誤,清理程式碼,或者學習新的東西。

總結

在你實際工作中做一些審查的流程吧,或者花點時間去分析,回顧並改進你既有的策略。迭代你的流程,擁抱潛在的變化,因為下一個主意就可能更好地改善結果。軟體和真人同時審查你的程式碼,能避免你把愚蠢的錯誤釋出到產品中去,能讓你的程式碼倉庫中堆砌的不再是無法管理的程式碼。

相關文章