Code review

In this post:

  • 為何做 code review?
  • 走味的 code review。

為何做 code review?

實施 code review 的兩個主要理由:提高程式品質,以及傳遞知識。

提高程式品質

Code review 可提升程式碼的品質,包括可讀性、可維護性等等,並減少應用程式的 bug。

程式碼的品質提高,洩漏至產品的 bug 數量減少,便能降低未來產品上線後的問題反饋、排查、debug、和部署新版本的成本。

促進了解,傳遞知識

  • 新人可透過 code review 加速上手。
  • 減少知識壁壘。code review 過程是公開透明的,所有團隊成員都能看到。
  • 透過公開的溝通與討論,可了解其他人的想法,互相傳遞經驗與知識。

有了上述優點,實踐 code review 的團隊理論上應該會更快樂。然而實際上是否如此,恐怕要打個問號,因為在實施 code review 過程中還有許多「眉角」,一不小心就會走偏。接著就來看看有哪些需要注意的地方。

走味的 code review

這裡談一下走味的 code review,意思是實踐 code review 而導致的一些扭曲現象或者 anti-patterns。

馬馬虎虎

當團隊中的每一位成員忙於衝刺自己手上的工作,很可能沒有時間和心力去看別人寫的 code 並提出建議。也許今天比較有空,就看一下別人的 pull request;若沒空,就隨便瞄幾眼,或者跳過。(反正還有其他人會看,應該吧?)

GitHub 的 pull request (PR),在 GitLab 叫做 merge request (MR)。以下用 PR 來簡稱 pull request 或 merge request。

想像一下,某一個 PR 裡面包含上千行的程式碼,變動橫跨二、三十個檔案,這樣的 PR 要花多少心力和時間去仔細 review?

於是可能出現一種情形:少量變動的 PR,容易獲得其他人的 review 建議;若 PR 變動量大到一個程度,review 建議可能就只有四個字母:LGTM。也就是 looks good to me 的意思。(下圖取自 Reddit 論壇

然後,團隊成員可能會看到越來越多的 LGTM。至於有沒有真的看程式碼,就自由心證了。

合作無間

在強制要求 code review 的團隊中,可能會在 CI/CD pipeline 裡面加入一個卡控規則:至少要有另一個人的 review 和 approve,CI/CD pipeline 才能往下執行。

這可能演變出一種行為:兩個好朋友之間彼此搭配合作,讓 PR 迅速過關。實際的操作可能是發私訊給夥伴,跟對方說:

「哈囉,我剛剛發了 PR,可以幫我 approve 一下嗎?」

然後,可能就會有某一位團隊成員感到納悶:「為什麼有些人的 PR 總是一個小時內就 approve 了,我的 PR 卻常常要等一兩天?」

重重關卡

想像開發團隊分別為 testing、staging、和 production 三種環境訂下 code reivew 的關卡:

  • 部署應用程式的新版本至 testing 環境時,需要 developer 和 project manager 的 review。
  • QA 工程師在測試機完成測試之後,也需要 project manager 的核准才能將新版程式推送至 staging 環境。
  • 最後,程式要部署至 production 環境時,則需要 project manager 和 CTO 都核准才能放行。

儘管這些關卡提供了更安全的卡控,但整個開發、測試和部署流程非常倚賴 project manager 這個角色,可能成為瓶頸,替原本已經有點笨重的流程平添更多阻礙。

目標持續移動

收到通知被指定為某個 PR 的 reviewer 之後,你花了時間認真且仔細地閱讀程式碼,然後提出了一些修改建議。就在你覺得差不多完成 review 工作時又收到通知,這個 PR 有新的 commits 推上來。因此,你又得重頭檢查一遍。

再次確認新的變動沒有其他問題之後,趕緊點下 "Approve" 按鈕,結果這個 PR 的狀態卻還是「需要 review」的狀態。怎麼回事?原來又有新的 commits 推上來。

像這種 review 過程仍有新的變動不斷推送至 repository 的情形,容易令 reviewer 感到挫折與煩躁,引發內心小劇場:「怎麼不事先確定程式碼都寫好了才發 PR 呢?」

粗魯的評論

透過 code review 所獲得的修改建議都一定是好的、對的嗎?不一定。我們甚至可能會收到到一些粗心的,甚至粗魯的 review 建議。

以下列舉幾種可能碰到的情形:

  1. 僅挑枝微末節的毛病,卻忽略其中有更重要、需要修改的地方。
  2. 凸顯存在感和個人貢獻,把焦點往自己身上拉。
  3. 忽略客觀與其他面向,只要跟自己習慣不同就要求必須改。
  4. 只要求改,不提供具體理由。
  5. 陷入面子之爭。

對別人的工作產出提供建議,我覺得是有點嚴肅的事情,而且很容易冒犯到對方。故提出 review 建議的人,我想還是應該盡到說明的責任——除非錯誤非常明顯且毫無爭議(例如拼字錯誤)。提供連結指向 coding standard 或 best practice 也是 ok 的,但如果都不解釋原由,就不大好了。以下舉兩個比較誇張的例子,以凸顯什麼是令人感覺武斷、粗魯的 review 建議:

  • This code looks like shit. (:thumbdown)
  • Rename the PowerOff method to TurnPowerOff.

第一個例子很明顯,「looks like shit」是很粗魯的話,更別提如果只有簡單寫這句,毫無建設可言,那就是純粹的罵人,來找碴的。

第二個例子則有可能是好的,也可能是糟糕的建議。但如果只寫那樣一句,完全不說明任何理由,便可能讓接受該評論的原作者感到不被尊重,引起一些質疑:

  • 「我的方法命名完全沒有違反團隊的 coding standard,為什麼一定要改成你習慣的名稱?」
  • 「平日趕進度已經很忙,很多功能和品質方面的問題都等著處理,現在竟然要花時間去討論這種無關痛癢的小地方?」

Code review 是知識傳遞與彼此溝通想法的過程,其中必然有溝通的藝術成分,這點需要放在心上,以免因為忙碌而忘了基本的尊重。在提出 review 建議之前,不妨先站在對方的立場想一下他為什麼要這樣寫,而不那樣寫。如果想不到理由,或有任何不確定的地方,可以先詢問對方:

「請問這段程式碼這樣寫的原因是因為 XXX 嗎?或者是其他想法?我覺得如果改成這樣....」

當然啦,接受建議的那一方也不要過於敏感、太容易感到被人冒犯。如果是建設性的建議,保持開放的心胸與對方討論,虛心學習,會是比較好的。

結語

一般而言,code review 能提升程式碼與軟體產品的品質,降低日後的維護成本,也能促進團隊成員之間的溝通與知識傳遞。但實際上做起來仍有許多細節要留意,以免走偏了,甚至引發團隊成員之間的不愉快。其中的溝通藝術,是我認為最重要的部分。

總之,若有爭議,時時想著「什麼才是對產品最好的」,並以此前提來思考和回應。


這篇筆記就先寫到這裡吧。日後想到什麼再來補。

Keep coding.

Last modified: 2024-07-09