Hero Image

讓自己當個更好的代碼審查者及提交者

我常想身為一個 programmer,該具備什麼能力,才適合幫別人做 code reveiw,對於自身的期許,總覺得沒有達到一定程度,不太該做這個角色,因為,錯誤的建議,不如不要建議;尤其,過去待過的幾間公司,基本上,大家都不是很喜歡自己的代碼被別人做 code review,被別人 code review 這件事情,每個人都會產生是不是自己能力不足,被別人評斷的感覺,抑或是被別人找渣的感覺.

Google how to code and review

在懞懂的做了一年多的 code reviewer 之後,還是決定下定決心,要把這個難題給解決,也是因為離職的工程師的代碼,在維護他的程式碼的過程中太痛苦,code review 不應該像是,上位者對下位者代碼的批判,而是推進專案代碼品質的一個方式,難的地方就在於,如何適當的 評論別人的代碼,以及接受別人評論,儘量合理理性的做出回應.

下面就是我根據,google 釋出的 Code Review Developer Guide 整理的概要.

CL:changelist,表示已提交給版本控製或正在進行代碼審查的一項獨立變更。 (涵蓋的意思是 change、patch 或 pull-request)
LGTM:意思是“我覺得不錯”。 這是代碼審查員在批准 CL 時所說的。
Nit: 雞蛋裡挑骨頭.催毛求疵
TODO: 代辦事項

代碼審核應該要考慮的方向

當我們打開代碼,該關注哪一些事情,google 整理了以下的 六個面向 的建議去做 代碼審核 ( Code Reveiws ):

  • 設計: 代碼是不是有良好的設計並且符合這個系統需求?
  • 功能: 代碼是不是有達成功能的需求?而且功能的表現讓使用者 (end-user, developer) 容易理解?
  • 複雜度:代碼是不是可以更簡單清晰的完成功能? 其他的開發人員可不可以快速地了解這段代碼並且在未來可以輕易的使用?
  • 測試: 代碼有沒有相對應並且設計良好的自動測試?
  • 命名: 開發者有沒有選擇清晰易懂的名稱來命名他的變數、類別、方法等等?
  • 註解: 註解是不是清晰易懂而且是有幫助的?
  • 代碼風格: 代碼風格有沒有遵守共同規定的風格?
  • 文件:開發者有沒有更新跟代碼相關的文檔?

當然,代碼的審核,理所當然的要挑對最好的的 審核者,因為一個最好的 審核者 能給你全面能且正確的建議.

所以,實際上該怎麼做?

代碼審核,是為了程式碼的品質,以及交付代碼的責任感,每個工程師來到一個新個公司,都不會希望自己手上的代碼一團亂,沒辦法輕易地理解邏輯,所以沒辦法做更好的開發.

有了原則之後,實際上做起來可能會遇到一些突發狀況,根據這些情況,可以再把這些步驟細分為:

  • 代碼審核的標準
  • 到底在代碼審核中要看什麼
  • 在審核中瀏覽改變的內容
  • 代碼審核的速度
  • 如何寫出好的代碼審核評論
  • 處理被駁回的建議

代碼審核的標準

代碼審核的意義在於確保程式碼的品質而且可以不斷的提升.

一般來說,即使 CL 並不完美,只要 CL 處於可以肯定改善正在處理的系統的整體代碼健康狀況的狀態,審閱者就應該贊成批准它。

Reviewer 有權限去決定,一個 CL 該不該被加入到 code base,即使那個功能有良好的設計,reviwer 都有權利去決定這個系統該不該有這個功能.

但是一個 Reviwer ,也應該注意,這世界上沒有所謂的 完美的代碼,永遠只有更好的代碼.每次做 reveiw 的時候,都應該去權衡取得專案進展的需要與他們建議的更改的重要性,而不是一位的追求完美,持續的進步 ( continuous improvement )才應該是追求的重點

Reviwer 也應當沒有負擔的去表達,他們認為怎麼做可以讓程式碼變得更好,但是如果這個評論不是太重要,可選擇用 Nit: 去表示

在風格問題上,風格指南是絕對權威。任何不在樣式指南中的純樣式點(空格等)都是個人喜好問題。風格應該與現有的一致。如果沒有以前的風格,接受作者的。

軟件設計的各個方面幾乎從來都不是純粹的風格問題或只是個人喜好。它們基於基本原則,應根據這些原則進行權衡,而不僅僅是個人意見。有時有幾個有效的選項。如果作者可以證明(通過數據或基於可靠的工程原理)幾種方法同樣有效,那麼審稿人應該接受作者的偏好。否則,選擇由軟件設計的標準原則決定。

如果沒有其他規則適用,那麼審閱者可能會要求作者與當前代碼庫中的內容保持一致,只要這不會惡化系統的整體代碼健康。

  • 技術事實和數據 > 個人偏好.
  • 風格指南 (style guide) > 個人喜好.
  • 對設計原則權衡 > 個人意見.

當衝突無法解決的時候,試著透過面對面溝通,或是尋求更高層級的主管,或是工程經理做最後的決定.

到底在代碼審核中要看什麼

這邊的內容為一開始提到的主要 六個面向 的細節,其實就是 審核代碼標準 的延伸,所以當閱讀下面的內容時,都要記得以 審核代碼標準中 提到的重點為輔.

設計

主要看代碼的整體設計,瞭解這次的 CL 是否有意義?更改的是什麼?更改代碼的內容是這產品需要的嗎?合併他的時機點對嗎?

功能性

總結 在進行代碼審查時,您應該確保:

  • 代碼有良好的設計
  • 該功能對代碼的使用者有好處。
  • 任何 UI 更改都是明智的並且看起來不錯。
  • 任何並行編程都是安全地完成的。
  • 代碼並不比它需要的複雜。
  • 開發人員沒有實現他們將來可能需要但不知道他們現在需要的東西。
  • 代碼有適當的單元測試。
  • 測試有良好的設計。
  • 開發人員對所有內容都使用了清晰的名稱。
  • 評論清晰有用,主要解釋為什麼而不是是什麼。
  • 代碼有適當的文檔記錄。
  • 代碼符合我們的風格指南。

記得,審核代碼應該 每一行 代碼都要去檢查,並且了解代碼的上下文意義,確定這代碼有在 改善代碼的品質;如果開發人員提交了一個好的 CL 時,別忘了要稱讚他.

在審核中瀏覽改變的內容

當知道 到底在代碼審核中要看什麼之後,就可以去想該怎麼有效率的的去管理 分散的檔案

  1. 整體去看 (注意代碼的上下文)
  2. 詳細檢查 CL 主要的改變部分
  3. 以適當的順序瀏覽剩餘的部分

代碼審核的速度

這邊建議是,越快的審核代碼是越好的.但是,談論到代碼審核的速度時,這邊指的是對 author 回應時間,而不是 CL 需要多長時間才能通過整個審查並提交。 理想情況下,整個審核的過程應該很快就可以結束,但對這個CL反應的速度,比整個審查過程的速度更重要。

這邊建議不要超過一天去做代碼的回覆.如果可行的話,應該在所有的休息時間後做代碼審核 (隔天早晨,會議之後,廁所,喝水,午餐等等);當然,當你有一個需要專注完成任務的時候,不應該打斷自己的任務,去做代碼審核,根據研究,開發人員在被打斷後可能需要很長時間才能恢復平穩的開發流程。

不過,要能做到代碼審核快速,必需要讓每次的 CL,儘可能的維持在最小的範圍,小的範圍可能是不到100行的代碼,但也可能是,小但是可行的功能:當對方的 CL 太大時,還是要回應對方,自己會在哪段時間有空,把整個

當審核的速度或者說是效率太低,很容易會發生以下的情況:

  • 整個團隊的開發速度會下降
  • 程序員會開始抗拒代碼審核
  • 程式代碼的品質會被影響

但是,不要為了想要達到想像中的代碼審核的速度,而妥協 代碼審核標準 以及 程式碼的品質.除了,緊急的事情發生

緊急的事情:

  • Deadline
  • hotfix

如何寫出好的代碼審核評論

  • 友善
  • 解釋原因
  • 解釋代碼的問題並且給出適當的方向讓開發者選擇
  • 鼓勵開發者去寫出更好懂的代碼而不是去解釋這難懂的代碼

處理被駁回的建議

有時開發人員會推遲代碼審查。 他們有時候會不同意你的建議,或是會抱怨你審查上過於嚴格。

  • 我之後再改 (Cleaning It Up Later)

這是許多開發者,都會做的事情,因為他們想趕快完成主管的交代,越快的完成主管交代的任務,代表績效更好,但是,實際上當這段代碼被審核通過之後,鮮少有開發者會回去修正個個代碼;有時候這些開發者的代碼,沒有在一開始就

當你必需要 submission 一個 complexity code, 必須在 comment 留下 TODO: 去描述目前這個代碼有什麼還沒修正的問題,

代碼提交者

  • 寫一個好的 CL 描述
  • 小的 CL
  • 如何處理 Code Reviewer 的評論

寫一個好的 CL 描述

一個好的 CL 應該儘量地描述清楚,這個改動是為了什麼而改動,建議 第一行 為標題,空格一行後為內文.

以下為 Google 示範的 完整 CL 樣子

Functionality change

Example:

rpc: remove size limit on RPC server message freelist.

Servers like FizzBuzz have very large messages and would benefit from reuse. Make the freelist larger, and add a goroutine that frees the freelist entries slowly over time, so that idle servers eventually release all freelist entries.

Refactoring

Example:

Construct a Task with a TimeKeeper to use its TimeStr and Now methods.

Add a Now method to Task, so the borglet() getter method can be removed (which was only used by OOMCandidate to call borglet’s Now method). This replaces the methods on Borglet that delegate to a TimeKeeper.

Allowing Tasks to supply Now is a step toward eliminating the dependency on Borglet. Eventually, collaborators that depend on getting Now from the Task should be changed to use a TimeKeeper directly, but this has been an accommodation to refactoring in small steps.

Continuing the long-range goal of refactoring the Borglet Hierarchy.

更小得CL

CL 內容要精要,除了幫助自己釐清自己異動的內容,也可以幫助 reviwer 做更快速地瀏覽.

如何處理 Code Reviewer 的評論

不要從個人的角度去看待別人對你的評論,即使覺得對方的評論可能帶有攻擊性,也不要帶有憤怒的語氣去回應,應該去思考他的評論有沒有意義,如果有意義就應該去修改代碼;有不懂的地方的時候,就應該請對方指出明確的方向去修改;儘量以團隊的角度去思考問題,當有不同意見的評論時,應該解釋自己為何要這麼做,而不是直接拒絕 reviwer 的建議.

https://cythilya.github.io/2020/11/01/google-how-to-do-a-code-review/

https://cythilya.github.io/2020/12/10/google-how-to-get-through-code-review/

https://google.github.io/eng-practices/review/