跟 Google 學如何 Code Review — (5) 給予回饋的原則以及後續抗力之應對方針

Whyayen
嗨,世界
Published in
8 min readApr 6, 2020

給予審查回饋的四大原則

在我們閱讀完一系列 Google 提供 Code Review 的方針後,我們已經知道如何進行程式碼審查,然而在審查的過程中,給予開發者回饋也是相當重要的一環,因此本篇將探討「給予審查回饋的四大原則」。

Photo by NESA by Makers on Unsplash

概要

  • 保持友善與尊重
  • 說明你的理由
  • 給出明確的指示及點出問題,並讓開發人員做決定
  • 鼓勵開發人員簡化程式碼或添加程式碼註解,而不僅是向你解釋其複雜度
[討論]
可以參考 SMART 原則
Specific: 明確點出問題或是審查者自己不了解的地方。
Measurable: 如果是建議,則應包含想要達成的效果,開發人員才有辦法評估後續修 改。CL 描述中應該帶有此 CL 的功能規格,可作為評估修改的基準。
Achievable: 提供的建議應該是開發者能力可及的,尤其是雙方經驗差異過大時,給予回饋前需要先思考對方是否有辦法理解。
Relevant: 盡量不做與此 CL 無關之評論,如果真的很重要則可以另外開 Issue 處理。Time-based: 評論/建議必須是時限內可完成的。

友善與尊重

通常保持友善與尊重是很重要的,同時也要對你正在審查該程式碼的開發者給予清楚且有幫助的評論。其中一種方法就是確定你的評論是關於程式碼本身,而不是開發者。你不必總是遵循這種作法,但是在說出可能令人不愉快或用爭議的內容時,你一定要這麼做。

舉例:

  • 糟糕 為什麼你要在這邊使用 Threads?它顯然沒有辦法從 Concurrency 中獲得任何效益。
  • 良好 這裡的 Concurrency Model 增加了系統複雜度,但我沒有看到任何效能上的效益。由於沒有任何效能上的好處,因此應該將程式碼改為 Single Thread,而不是使用 Multiple Threads。
[討論]
雖然後者有增加了理由以及建議方式,但如果我們進一步透過審查來促進對話就可以修改如下:
在這裡使用 concurrency model 的原因是?如果是效能考量的話,由於此處不是效能瓶頸,concurrency model 放在這目前看不到什麼效益,可以考慮先處理其他效能瓶頸,待此處有效能需求時再來優化。

說明你的理由

你從上面良好的例子中可以注意到一件事情,那就是它可以幫助開發者了解你為什麼發表此評論。你不一定需要這樣做,但有時候進一步地說明你的目的、你所遵循的最佳實踐或是你的建議如何提升程式碼品質是更合適的。

提供引導:給出明確的指示及點出問題,並讓開發人員做決定

一般來說修正一個 CL 是開發者的職責,而不是審查者,你無需提供一個詳細完整的解決方案,或為開發人員撰寫程式碼。

不過這並不意味著審查者沒有任何幫助。通常你應該在指出問題和給予直接指導間取得一個適當的平衡點。指出問題並讓開發人員自行處理通常可以幫助開發人員學習,並使程式碼審查更加順暢。這也可能產生更好的解決方案,因為開發人員比審查者更熟悉程式碼。

但有時候直接的指示、建議,甚至程式碼更有幫助。別忘了程式碼審查 的主要目標是得到更好的 CL,第二個目標是提升開發人員的技能,使他們未來僅需愈來愈少的程式碼審查。

[討論]
這裡討論的多是資深審查資淺的情境,如果是相反的情境呢?或是另一種例子,有時候某個功能整間公司就是開發者最懂/整間公司開發者最為資深,審查的人無論如何不會有更多更深的理解的時候要如何進行審查?
1. 先想想自己會怎麼做,再對照對方處理方式自己是否看得懂,不懂的就詢問
2. 程度差異較大時,審查 CL 的時消化時間會變長,這段消化學習的時間需要考慮進去
3. 承 2, 這種審查的目的除了更好的 CL 之外,也包含了知識傳遞的成份在內
4. 所謂更好的 CL 則是可以從不同的觀點來看看 CL 是否好理解好維護,畢竟程式碼是整個團隊在維護的,如果審查者看不懂則之後維護的人也有可能看不懂。

鼓勵開發人員簡化程式碼或添加程式碼註解,而不僅是向你解釋其複雜度

如果你要求開發人員解釋一段你不理解的程式碼,通常應使他們重寫成更好理解的程式碼。有時侯在程式碼中添加註解也是一種適當的回應,只要它不只是解釋過於復雜的程式碼即可。僅在 程式碼審查工具中的解釋或說明對未來閱讀程式碼的人並沒有幫助。它們僅在某些情況下是可以接受的,例如:當你查看不太熟悉的區域,且開發人員解釋了其他此段程式碼讀者已經知道的內容時。

參考資料

關於如何撰寫 Code Review 評論,可以參考成功大學 Jserv 老師所開的 Linux 核心設計課程資料。雖然這些開發紀錄並非一個團隊共同協作的專案,但有許多值得參考的評論可以被採用於專案的 CL 中,如:參考語言規格書、避免不精確的用詞、收到反饋後學生如何去改善他們的程式碼等。

面對程式碼審查中的抗力

有時候開發者會推遲處理程式碼審查中的評論,要麼他們不同意你的意見,要麼抱怨你太過嚴格了。

誰是對的?

當開發者不同意你的建議,先花一點時間思考一下他們說的是否正確。通常他們比你更接近程式碼,因此實際上他們可能對某些程式碼有更好的了解。他們的論點有意義嗎?從程式碼的角度來看他是否合理?如果合理,讓他們知道他們是對的,並停止這個議題。

然而開發者並不總是對的,在這種情況下審查者應該進一步解釋為什麼他們認為自己的建議是正確的。良好的解釋既可以說明對開發者回覆的理解,也可以說明為何要求進行改動等資訊。

尤其是當審查者認為他們的建議將改善程式碼的品質時,且所需的額外工作是合理的,便應該繼續主張自己的論點。改善程式碼的品質,往往在小地方開始發生。

有時候需要花幾番解釋,才能被充分地理解,記得維持友善與尊重的態度,讓開發者知道你有聽到他們在說什麼,只是你持著不同的論點。

使開發者沮喪

審查者有時認為如果堅持要改進,會使開發者感到沮喪。有時開發人員確實會感到沮喪,但這通常是短暫的,後來他們甚至非常感謝您幫助他們改善了程式碼品質。通常只要你在評論保持禮貌,開發者實際上根本不會感到沮喪,這些擔憂僅存在於審查者心中而已。沮喪通常與評論的撰寫方式有關,而不是與審查者對程式碼品質的堅持有關。

[討論]
除了保持友善與尊重,也可以試著多理解開發者的思考脈絡,這樣比較容易與對方溝通。撰寫方式是表面,更深層的問題是對於審查制度的信心以及態度。當我們對於審查者或審查過程抱持開放態度、具有信心時,也比較不會產生沮喪的心情。因此身為審查者必須要思考,如何透過溝通來建立雙方的信任度,讓未來的審查可以更順利的進行。

稍後清理

推遲處理的一個常見原因是開發人員想要完成任務(可以理解),他們不想進行另一輪審查來通過此 CL。所以他們會說他們將在之後的 CL 進行改善,因此你現在應該給審查通過。當然一些開發者很擅長這點,馬上撰寫一個後續的 CL 來解決此議題。但是根據以往經驗,在開發人員撰寫原始 CL 之後,隨著時間的流逝,清理工作的可能性會降低。實際上,通常除非開發者在當前的 CL 批准後立即進行清理,否則清理工作永遠不會發生。這並不是因為開發人員不負責任,而是因為他們有很多工作要做,並且清理工作在其他工作中被遺忘。因此最好是堅持要求開發人員在程式碼合併回主要程式碼前立即清理其 CL。讓人們「稍後清理」有時候等同於降低程式碼健康度。

如果 CL 引入了新的複雜性,除非是緊急情況,否則必須在提交之前將其移除。如果 CL 有些相關的問題且現在無法解決,則開發人員應紀錄錯誤任務並將其分配給自己,以免後續遺忘以致丟失問題。他們也可以選擇在程式碼中留下 TODO 的註解,連結至引起的問題。

[討論]
什麼東西可以稍後清理?什麼東西不可以?
CL 定義的功能一定要完成,其他都可能可以稍後處理,重點是審查過程中發現的議題需要記錄下來並連結在一起,後續要能找到的地方,不然無法進行後續追蹤。以我們團隊來說,可能會放的地方依照緊急程度會是:
1. 插單
2. 下個 planning 議程 (確保下次 planning 會討論到)
3. 下個 refinement 議程 (確保下次 refinement 會討論到)
4. PRD 相關功能的未來規劃 (確保下次做這功能時會看到)

嚴格審核常見的抱怨

如果你以前對程式碼的審查鬆懈,因而轉為對程式碼進行嚴格的審查,那麼一些開發人員將大聲抱怨。提高程式碼審查的速度通常會使這些抱怨消失。

有時這些抱怨可能要花費數月的時間才能消失,但是最終開發人員往往會看到嚴格的程式碼審查所帶來的價值,因為他們看到了他們可以幫助產生的出色程式碼。且一旦發生某種事情,最強烈的抗議者甚至成為你最堅強的支持者,這會使他們看到嚴格審查所帶來的價值。

[討論] 與開發者沮喪的章節類似,也是比較精神層面的議題,審查與被審查雙方都應該要抱持開放心態,才有利於討論的進行。如果有一方有所抱怨,那麼,總有人要負責改善(通常就是看這篇文章的你),負責思考如何讓這個審查可以更加順利的進行。

解決衝突

如果您遵循了以上所有條件,但仍然遇到無法解決的開發人員與你之間的衝突,請參閱《跟 Google 學如何 Code Review — (1) 基本原則》,以獲取有助於解決衝突的準則和原則。

--

--