Skill

Google 如何進行 Code Review - 6

一窺 Google 內部程式審核的指導原則 - Handling Pushback in Code Reviews

ta-ching chen

1 minute read

原文

Handling Pushback in Code Reviews (如何面對被推遲處理的評論)

有時開發人員會推遲處理程式審查產生的評論。要麼他們不同意你的建議,要麼他們會抱怨你太嚴格了。

Who is right? (誰是對的)

當開發人員不同意你的建議時,請先花點時間考慮一下他們是否是正確的。因為通常他們比你更接近程式碼,所以他們可能真的比起你來說對程式的某些層面具有更好的洞察力。 他們的論點有意義嗎? 從程式品質的角度來看它是否合理? 如果是的話,讓他們知道他們是對的,然後讓問題沈下去。

但開發人員也並不總是對的。在這種情況下,審核者應該更進一步解釋為什麼認為自己的建議是正確的。 一個好的解釋通常展示了「對開發人員的回覆的理解」以及有關「為什麼被要求對出做出改動」等資訊。

尤其是當審核人員認為給出的建議會改善程式品質時,便應該繼續宣揚自己的論點。只要他們相信所需的額外的工作量最終會改善整體程式品質。

提高整體程式健康狀況這件事,往往是經由每個微小的改動來發生

有時往往需要幾番解釋一個建議才能讓對方真正理解你的用意。切記,始終保持禮貌, 讓開發人員知道你有聽到他們在說什麼,只是你不同意該論點而已。

Google 如何進行 Code Review - 5

一窺 Google 內部程式審核的指導原則 - How to write code review comments

ta-ching chen

1 minute read

原文

How to write code review comments (如何寫審核評論)

Summary (總結)

  • 和藹且有禮貌的
  • 解釋你的理由
  • 在「給出明確的方向 (explicit directions) 」與「只點出問題點,讓開發者做決定」之間作出權衡
  • 鼓勵開發者簡化程式或增加註釋,而非向你解釋這問題有多複雜

Courtesy (禮貌)

一般而言,對你正在審查其程式的開發人員來說,保持禮貌與尊重的同時,確保評論是非常清楚和有幫助。

一種方法是確保

永遠對程式碼發表評論,而永遠不要對開發人員發表評論

你可能並不總是必須遵循這種做法,但你絕對要在說出會使人不安或有爭議的事情時使用這種方法。例如:

  • 糟糕的例子:

Why did you use threads here when there’s obviously no benefit to be gained from concurrency?

為什麼「你」要在使用併發沒有明顯益處時使用 threads?

  • 好的例子

The concurrency model here is adding complexity to the system without any actual performance benefit that I can see. Because there’s no performance benefit, it’s best for this code to be single-threaded instead of using multiple threads.

在我看來,這裡使用併發模型會增加整體系統的複雜性,卻沒有帶來任何實際性能效益。由於沒有性能上的效益,我們最好在這個程式裡使用 single-threaded 而非 multiple threads。[1]

Google 如何進行 Code Review - 4

一窺 Google 內部程式審核的指導原則 - Speed of Code Reviews

ta-ching chen

1 minute read

原文

Speed of Code Reviews (程式審核的速度)

Why Should Code Reviews Be Fast? (為什麼審核速度要快)

在 Google 我們優化開發團隊共同生產產品的速度,而不是優化個人開發程式的速度。

個人的開發速度很重要,但它不如整個團隊的開發速度 (velocity of the entire team) 那般重要。

當程式審核很慢時,會發生以下幾件事:

  • 團隊整體的速度下降

是的,沒有快速回應的個人 (individual) 的確完成自己所屬的工作。但同時也表示,對於團隊其他人來說重要的新功能與缺陷修復將會被延遲數天、數週甚至數月,只因為它們正在等待審核與再審核。

  • 開發人員開始抗議程式審查流程

若審核者每隔幾天才回應一次,但每次都要求對 CL 進行重大更改的話,那麼開發人員可能會感到非常沮喪與覺得困難,而這通常會演變成對評論者的「嚴格」程度的抱怨。 如果審核者請求相同的實質性更改 (且確實可以改善程式品質狀況),但在每次開發人員提交新的改動時都能快速反應的話,這些抱怨往往會消失。

大多數關於程式審查過程的抱怨,往往可以通過加快流程來解決。

  • 程式品質會受到影響

當審核評論很慢時,會造成開發者提交不完全盡如人意的 CL 的壓力越來越大 [1]。評論的越慢也會阻止他人進行程式清理 (cleanups)、重構 (refactorings)、甚至是對現有 CL 的進一步改進。

  • [1] 還記得前面所說的嗎? 沒有完美的程式只有最佳的程式!

How Fast Should Code Reviews Be? (那程式審核應該要多快)

如果你並沒有處於需要專注工作 (focused task) 中間時,那麼你應該在 CL 被提交後儘快進行審查。

回應程式審查最長的極限是一個工作日是 (意即隔天早上的第一件事)

若遵循以上指南,意味著一般的 CL 應該在一天內得到多輪審核 (如果必要的話)。

Google 如何進行 Code Review - 3

一窺 Google 內部程式審核的指導原則 - Navigating a CL in review

ta-ching chen

1 minute read

原文

Navigating a CL in review (審核中該如何在 CL 巡航)

Summary (總結)

現在你已經知道審核時要看些什麼,但怎樣才是審核分散在多個文件中的改動最有效的方法?

  1. 改動是否合理? 他是否有好的描述 (description)
  2. 優先看 CL 最重要的改動。它整體是否有完善的設計?
  3. 用合理的順序看 CL 剩餘的改動

Step One: Take a broad view of the change (用宏觀的角度來看待改動)

Look at the CL CL description and what the CL does in general.

查看 CL 描述以及它做什麼

該改動是否有意義、合理?如果在第一時間認為不應該發生這種變化,請立即說明為什麼不該這樣做的原因。 當拒絕類似這樣的更改時,向開發人員提供建議告訴他們應該怎麼做什麼也是一個好主意 [1]。

例如,你可以說「看起來你已經完成一些不錯的事情,謝謝! 但我們實際上正朝著刪除你正在修改的 FooWidget 系統的方向前進,所以現階段我們不想對它進行任何新的修改。 你覺得不如重構我們的新 BarWidget class 的主意如何?」

值得注意的是,審核者在委婉拒絕該 CL 的同時提供替代方案,而且仍然保持「禮貌」。這種禮貌是很重要,因為我們希望表明即使不同意也會相互保持尊重。

如果你有幾個 CL 裡頭包含你不想這樣做的改動時 (a few CLs that represent changes you don’t want to make), 你應該考慮重新開發團隊的開發過程,或發佈開發流程給外部貢獻者 (contributor) 知道,以便在任何 CL 被撰寫前有更多的溝通。 最好是在他們開始投入大量工作前説「不」,避免那些已經投入心血的工作現在必須被拋棄或徹底重寫。

  • [1] 提供替代方案讓對方知道該怎麼做,而非讓其自行獨自摸索。
suggestion

點出可能問題,告知替代方案或指點方向

Step Two: Examine the main parts of the CL (檢查 CL 主要的部分)

Find the file or files that are the “main” part of this CL.

找到 CL 最核心的部分的那些文件

Google 如何進行 Code Review - 2

一窺 Google 內部程式審核的指導原則 - What to look for in a code review

ta-ching chen

2 minute read

原文

What to look for in a code review (程式審核過程中要看些什麼?)

Design (設計)

程式審核裡最重要的一環是 CL 的整體設計

CL 內多個程式片段的改動之間的交互作用是否合理? 改動是屬於程式庫 (codebase) 還是函式庫 (library)? 此次改動是否能和系統其他部分很好地整合? 現在是加入該功能的適當時機嗎?

Functionality (功能性)

CL 是否真的有符合開發者意圖? 開發者的意圖對於使用者們是好的嗎? 這裡使用者通常係指終端使用者 (受到改動而影響)開發者(未來會用到該程式的人)雙方。

在大多數情況,我們會期望開發者徹底測試他們的 CL,並期望在接受審核時它能夠正常運作。但身為審核者你仍必須去思考可能的「極端案例 (edge cases)」、找出「併發問題 (concurrency problem)」、 嘗試「作為一個使用者去思考」,並且確保「在單純閱讀程式碼時沒有明顯的程式缺陷 (bug)」。

願意的話你也可以去親自去驗證 CL - 如果當改動會帶來直面使用者的影響 (user-facing impact) 時,比方說 UI 改動,驗證 CL 的變化便會是審核者在整個過程中最重要的事情。 單純閱讀程式碼有時很難體會它會如何影響使用者,面對如此的改動如果覺得不方便自己測試的話,可以要求開發者展示 (demo) 此次改動的功能性。

另外,審核功能性時格外重要的一點是,要注意 CL 中是否有「平行程式設計 (parallel programming)」的存在,並是否在理論上會造成 deadlocks 或 race conditions。 這類的問題較難透過執行程式來查覺,通常會需要有人 (開發者跟審核者) 一起仔細思考,確保該問題不會被引入程式庫內。

Note: 這也是不使用可能會造成 deadlocks 或 race conditions 的併發模型 (concurrency model) 很好的理由 - 它可能會讓程式審核或理解上造成困難。

Complexity (複雜性)

一個 CL 是否複雜到超過原本所必須的? 針對任何層級的 CL 請務必確認這幾點 - 單行程式是否過於複雜? 函式是否過於複雜? Class 是否過於複雜?

「複雜」通常代表著該程式很難快速被閱讀程式的人所理解

也意味著很有可能其他開發者在呼叫或修改這段程式時引入缺陷

其中就是一種複雜性便是「過度設計 (Over-engineering)」,開發者會讓程式過度通用化 (more generic) 超過它原本所需的,或是新增現在系統不需要用到的功能 [1]。 審核者對於必須非常警惕過度設計的發生。

鼓勵開發者解決他們現在需要解決的問題,而那些猜測未來可能會需要被解決的問題

可以在真正面臨到那些未來的問題時才解決它們,因屆時你才能更清晰看到問題的樣貌 (actual shape) 與需求。

  • [1] 個人認為和 Donald Knuth 說得「過早最佳化是萬惡的根源 (Premature optimization is the root of all evil)」有類似意思。 自己在開發過程中也常常落入相同窠臼,認為未來可能會需要所以現在先寫起來放。但更常見的狀況是,當實際面臨問題時,狀況可能和原先大相徑庭導致需要重寫。