Google 教我如何面對 Code Review

每天我都要被 code review 很多次,而如何準備 code review 時需要的內容與心態呢?這篇文章是我閱讀 Google 的「The CL author’s guide to getting through code review」的筆記還有自己的小感想。

詳細撰寫 CL 的內容

CL (change list) 的內容主要是要表達 (1) 改了什麼 和 (2) 為什麼要做這樣的修改。當我們進 code 並經由 pull request 填寫 CL 的內容,再之後 merge 進入 master 便會成為 commit 的歷史紀錄。這樣的紀錄能提供後續查閱程式碼的人一個概略的介紹與理解,而不需要直接深入每一行程式碼,追程式碼、看註解(如果有寫註解的話)來慢慢的抽絲剝繭、去理解所有的內容。

第一行要寫什麼?

CL 的描述的第一行應該要

內文必須要有用且足夠的資訊

CL 的內文(body)必須要有用且足夠的資訊,因此 CL 的內文必須要有

錯誤的 CL 描述

填寫 CL 描述時必須要能提供有用且足夠的資訊,以下是錯誤的範例:

好的 CL 描述

以下是好的 CL 描述範例。

功能變更(Functionality Change)

RPC:移除 RPC 伺服器的訊息的內存分配器的空間限制。 —–(1)

如同 FizzBuzz 這樣的伺服器擁有很大的訊息量,若能重用則可得到很大的效能改善。將內存分配器的空間加大,並加上輕量的執行緒管理器來緩慢的釋放用不到的空間,這樣閒置的伺服器最後就能釋放所有的可利用空間。 —–(2)


說明

重構(Refactoring)

利用 TimeKeeper 建立一個 Task 來使用其 TimeStr 與 Now 方法。—–(1)

在 Task 中加入一個 Now 方法來取代 borglet() getter 方法(目前只有用在 OOMCandidate 用以呼叫 borglet 的 Now 方法)。之後會由 TimeKeeper 來使用 Now 方法。為了移除 Borglet 的相依性,目前是利用 Task 來使用 Now 方法,但最終的目的是要直接使用 TimeKeeper,目前會利用持續重構 Borglet 的結構的方式一步步達成目標。—–(2)


說明

就算是很小的 CL,其內文也應該要仔細撰寫

為 status.py 這個檔案建立 Python3 打包版本的規則。—–(1)

允許已使用這個打包版本到 Python3 的客戶來根據原本已建立的規則來建立規則,而不是重新建立新的規則。這樣能鼓勵客戶使用 Python3 而非 Python2,並且大大簡化了當前正在使用的某些自動構建文件重構工具。—–(2)


同樣的,說明

由工具自動產生的 CL

有些 CL 是由工具自動產生的,而這些 CL 應該也要遵守以上規則。

在提交前要仔細檢查 CL 的描述

在提交前要仔細檢查 CL 的描述,確保這些描述內容能適當表達 CL 的變更。


同場加映:如何撰寫 Pull Request 的內容

看完以上的建議,可能會感到對於到底該怎麼撰寫 CL 的內容依舊感到模糊。幸好已有許多現成的樣板可供使用,那就來用吧。

下圖是目前我所在的團隊所使用的樣板的部份內容,是要在提交 pull request 時仔細撰寫相關資訊的。

詳細填寫 CL 的內容

squash & merge 進入 master 後,點進去這個 commit log 便能看到之前填寫的詳細內容。

詳細填寫 CL 的內容

那麼,要怎麼在自己的 repo 設定樣板呢?

完工後,便可在之後提交 pull request 時,依照樣板填寫該有的資訊,就不用煩惱到底該提供什麼樣的內容給審閱者了。


小的 CL

為什麼要提交小的 CL?

小的、簡單的 CL 意味著

注意,審閱者可能會因為 CL 過大而拒絕這個 CL,進而要求 (1) 將這個大型的 CL 做拆分,而拆分的工作是很麻煩的;或 (2) 需要開發者來說服審閱者為何要同意如此大的 CL。因此在一開始就提交小的 CL 會讓這整個審閱過程更加容易。

什麼是小的 CL?

通常,CL 適當的大小是一個「獨立的變更」,意思是

如何界定 CL 是大還是小?

沒有硬性規定多大是太大,而通常一個 CL 在 100 行以內的程式碼是可接受的範圍,若是 1000 行就真的太大了,但通常要由審閱者來決定。變更所散布的檔案數也會影響所謂的大小,一個檔案若有 200 行程式碼的變更也許是可以的,但散佈在 50 個檔案就是太大。

開發者畢竟是最熟悉程式碼的人,因此對於開發者來說的「很小」也許對於審閱者來說是「很大」的。推薦的做法是開發者提交比自己認知還要小的 CL,況且審閱者很少會抱怨 CL 太小 (  ̄ 3 ̄)y▂ξ

怎樣的狀況下允許大的 CL?

以下是允許大的 CL 的情況

依照檔案來做切分

大的 CL 可依照檔案來做拆分,這些檔案的分類方式是依據要給不同的審閱者來分組,但依舊是獨立的變更,範例如下:

獨立重構的 CL

重構(refactor)的 CL 最好要與功能開發或修復 bug 的 CL 分開提交,例如:移動或重新命名 class,這樣審閱者會比較容易理解每個 CL 的脈絡與目的。

小的重構(例如:調整變數名稱)可以包含在功能開發或修復 bug 的 CL,但功能開發或修復 bug 的 CL 若要包含重構,這個重構到底可以調整到什麼程度仍須由開發者和審閱者來決定,畢竟包含太大的重構可能會造成審閱上的困難。

CL 必須包含相關的測試程式

CL 必須包含相關的測試程式。「小」的 CL 只是一個概念,用來表示 CL 必須專注在特定的功能或目的,而不是指程式碼的行數很少。

當 CL 內涵邏輯新增或變更,就需要包含相關邏輯新增或變更的測試程式,就算是重構而沒有變更任何邏輯仍需要有測試程式。理想上,重構的部份應該已經有現成的測試程式了,如果沒有就請補上。

然而,測試程式是可以獨立被提交單獨的 CL 的,就如同前面提到的拆分大的 CL 的概念,包含以下的狀況都適用:

不要搞壞系統

若提交的多個 CL 而彼此間有關聯,則必須確保每個 CL 分別上線時不會搞壞系統。

CL 真的很大怎麼辦?

解法如下

如何面對審閱者的評論

當開發者提交 CL 來做審閱時,審閱者應該會回覆一些評論,以下提供一些有用的建議來陪伴開發者度過這個過程。

他不是針對你啦!別放在心上

審閱程式碼的目的是為了維持程式庫與產品的品質。當審閱者提出評論時,當成這是在幫助開發者、程式庫與產品,而非攻擊開發者與開發者的能力,回顧前面提到的,審閱者也會盡量讓自己保持禮貌、快速回應、解釋評論的原因和提出有用的建議與接受討論。

有時審閱者會在評論中表達自己的沮喪,這樣做是不妥的,但開發者應該要堅強一點,問你自己「審閱者想要跟我說什麼呢?我希望可以得到一些有建設性的事情。」然後就再溝通吧,絕對不要意氣用事,像是憤怒的回覆審閱者的評論,這樣很不專業的…由於這些紀錄會永久保存在版本控管的系統中,建議想辦法讓自己平靜之後再來面對。

通常來說,當審閱者沒有提供禮貌且有建設性的回應,建議面對面或是經由視訊來針對這個 CL 來做討論。若無法面對面或是經由視訊來討論,私下寫信也是可以的-禮貌的跟他們解釋你喜歡與不喜歡怎麼做。若還是無法理性溝通,建議請主管來處理。

解釋與註解

假設審閱者提出他們無法理解的部份,開發者應該要先解釋這一部份。若無法解釋,則要加入註解說明為什麼這段程式碼要在那裡。

又,假設審閱者提出他們無法理解某段程式碼,那麼之後接手的開發者可能也會有相同的疑惑,這時候回覆在 CL 的評論就顯得沒有幫助,建議解釋程式碼後,要在程式碼中加上註解。

為你自己想想

提交 CL 的準備工作真的不少,當提交的那一刻應該是準備充足了吧!?因此當審閱者給予評論而需要改進時,開發者很容易認為評論是錯的,或是審閱者阻擋了這個 CL,應該要直接 approve 才對吧。當開發者有這樣的想法時,首先應該先捫心自問「審閱者的建議是有用的嗎?審閱者是對的嗎?」若仍有疑惑,就要請審閱者來說明這些評論,必說出自己為何認為這樣是好的解法與提供審閱者更多相關資訊。不要把審閱者當成敵人,試著溝通和思考審閱者的建議,最後通常雙方是可以達成共識的。

解決衝突

若發生衝突,試著與審閱者達成共識,若無法達成共識,請參考這裡的建議。

參考資料


code review pull request GitHub 閱讀筆記