Google 是如何做 Code Review 的?

Google 是如何做 Code Review 的?| 原力計劃

作者 | 帥昕 xindoo

出品 | CSDN 博客

我和幾個小夥伴一起翻譯了Google前一段時間放出來的Google’s Engineering Practices documentation(https://github.com/google/eng-practices),翻譯後的GitHub倉庫:https://github.com/xindoo/eng-practices-cn,歡迎加star。目前只是翻譯完了,因為譯者水平有限,還需要審校。另外後續Google肯定還會有新內容放出來,歡迎想參與貢獻的小夥伴加入,也歡迎在GitHub上加star。

這篇文章是Google’s Engineering Practices documentation的第一章Code Review實踐指南:https://xindoo.github.io/eng-practices-cn/review/。

谷歌Code Review指南, 包含兩個子章節:

  • 評審者指南:https://xindoo.github.io/eng-practices-cn/review/reviewer/

  • 開發者指南:https://xindoo.github.io/eng-practices-cn/review/developer/

Google 是如何做 Code Review 的?| 原力计划

術語

部分文檔中會用到一些谷歌內部的術語,特在此說明:

  • CL: “changelist"的縮寫,代表已經進入版本控制軟件或者正在進行代碼評審的變更。其他組織經常稱為"change"或者"patch”。

  • LGTM: "Looks Good to Me."的縮寫,看起來不錯,評審者批准CL時會這麼說。

Google 是如何做 Code Review 的?| 原力计划

評審者指南

Code Review標準

Code Review的主要目的是始終保證隨著時間的推移,谷歌代碼越來越健康,所有Code Review的工具和流程也是針對於此設計的。

為了完成這點,我們不得不權衡利弊。

首先,開發者應當在他們代碼中做一些 改進 ,如果你永遠都不做出改進,代碼庫整體質量也不會提升。但是如果審查者為難所有變更,開發者未來也會失去改進的動力。

另一方面,保證代碼庫隨時間推移越來越健康是審查者的責任,而不是讓代碼庫質量變得越來越差。這很棘手,因為代碼質量一般都會隨著時間推移越來越差,尤其是在團隊有明確時間限制、而且他們覺得不得不採取一些投機取巧的方式才能完成任務的情況下。

但是,代碼評審者得對他們Review的代碼負責,所以他們想始終確保代碼庫一致、可維護(其他指標見"Code Review應該關注什麼?")

依據這些,我們將以下準則作為我們期望的Code Review標準:

通常而言,只要代碼對系統有明顯的提升且正常工作,即便不完美,評審者也應該傾向於通過這次變更。

這是所有Code Review指南中的高級原則。

當然這也有些侷限。例如,如果變更里加入了有些評審者在系統裡不想要的功能,即便代碼設計的很好,評審者也可以拒絕掉。

關鍵點是沒有任何完美的代碼,只有更好的代碼。代碼評審者絕對不應該要求開發者在批准前對變更中的每一個小塊進行打磨,相反,評審者應該權衡向前推進和他們採納他們建議二者的重要性。評審者應該追求 持續提高 ,而不是追求完美。那些可以提升整個系統可維護性、可讀性和可以理解性的變更不應該因為代碼不夠完美而被推遲幾天甚至幾周。

評審者要 始終 不拘謹於在代碼評論裡提示可以更好的想法。但如果不是很重要信息,可以在評論前面加上標識告訴他們可以忽略。

注意:這篇文檔中沒有任何地方辯解在變更中的檢查會讓整個系統代碼變得 更糟糕 。你唯一能做的在緊急狀況中說明。

指導性

Code Review有個重要的作用,那就是可以教會開發者關於語言、框架或者通用軟件設計原理。在Code Review中留下評論來幫助開發者學習新東西是很值得提倡的,畢竟共享知識也是長期提升系統代碼健康度的一部分。但請注意,如果你的評論純粹是教育性的,並且不是這篇文檔中提到的關鍵標準,請在前面加上“Nit:”標識,或者明確指出不需要在這次變更中解決。

原則

  • 技術和數據高於意見和個人偏好。

  • 關於風格問題, 風格指南是絕對的權威。任何不在樣式指南中指出的樣式(比如空格等)都是個人偏好的問題。風格應該與現有的一致。如果沒有以前的風格,就按作者的風格來。

  • 軟件設計從來不是純粹的代碼風格或是個人偏好問題,它們是基於一些應當被權衡的規則而不僅僅是個人傾向。有時候也會有多種有效的選項,如果開發者能證明(通過數據或者原理)這些方法都同樣有效,那麼評審者應該接受作者的偏好,否則應該遵從軟件設計標準。

  • 如果沒有其他的規則使用,只要保證不會影響系統的健康度,評審者可以要求開發者保持和現有的代碼庫一致。

解決代碼衝突

如果Code Review中有任何衝突,開發人員和評審人員都應該首先根據開發者指南和評審者指南中其他文檔的內容,嘗試達成一致意見。

當很難達成一致時,開發者和評審者不應該在Code Review評論裡解決衝突,而是應該召開面對面會議或者找個權威的人來協商。(如果你在評論裡協商,確保在評論裡記錄了討論結果,以便日後其他人翻閱。)

如果這樣都解決不了問題,那解決問題的方式就應該升級了。通常的方式是拉著團隊一起討論、讓團隊主管來權衡、參考代碼維護者的意見,或者讓管理層來決定。不要因為開發者和評審者不能達成一致而把變更一直放在那裡。

Code Review應該

關注什麼?

注意:當我們考慮以下點時,應當始終遵循Code Review標準。

設計

Code Review中最重要的一個點就是把握住變更中的整體設計。變更中各個部分的代碼交互是否正常?整個改動是否屬於你負責的代碼庫?是否和你係統中其他部分交互正常?現在是否是添加整個功能的恰當時間?

功能性

開發者在這個變更中想做什麼?開發人員打算為該代碼的用戶帶來什麼好處?(這裡”用戶“是指受變更影響到的實際用戶,和將來會使用到這些代碼的開發者)

重要的是,我們希望開發者能充分測試代碼,以確保代碼在Code Review期間正常運行。但無論如何,你作為審查者也要考慮一些特殊情況,像是有些併發問題。站在用戶的角度,確保你正在看的代碼沒有bug。

你可以驗證變更,尤其是在有面向用戶的影響時,評審者應該仔細檢查整個變更。有時候單純看代碼很難理解這個變更如何影響到用戶,這種情況下如果你不方便自己在CL中打補丁並親自嘗試,你可以讓開發者為你提供一個功能性的demo。

另一個在Code Review時需要特別關注的點是,CL中是否有 併發編程,併發理論上可能會導致死鎖或資源爭搶,這種問題在代碼運行時很難被檢測出來,所以需要有人(開發者和評審者)仔細考慮整個邏輯,以確保不會引入線程安全的問題。(所以除了死鎖和資源爭搶之外,增加Code Review複雜度也可以成為拒絕使用多線程模型的一個理由。)

複雜性

變更是否比預期的更復雜?檢測變更的每個級別,是否個別行太複雜?功能是否太複雜?類是否太複雜?”複雜“意味著代碼閱讀者很難快速理解代碼,也可意味著開發者嘗試調用或修改此代碼時可能會引入bug。

一個典型的複雜性問題就是 過度設計,當開發者讓代碼變得更通用,或者增加了許多當前不需要的功能時,評審者應該額外注意是否過度設計。鼓勵開發者解決現在遇到的問題,而不是揣測未來可能遇到的問題。未來的問題應當在遇到時解決,到那個時候你才能看到問題本質和實際需求。

測試

開發人員應當進行單元測試、集成測試或端到端測試。一般來說,變更中應該包含測試,除非這個變更只是為了處理緊急情況。

確保變更中的測試是正確、合理和有用的。因為測試本身無法測試自己,而且我們很少會為測試編寫測試,所以必須確保測試是有效的。

如果代碼出了問題,測試會失敗嗎?如果代碼發生改動,它們會誤報嗎?每一個測試都有斷言嗎?是否按照不同的測試方法對測試進行分類?

記住,不要以為測試不是二進制中的一部分就不關注其複雜度。

命名

開發人員是否使用了良好的命名方式?好的命名要能夠充分表達一個項(變量、類名等)是什麼或者用來做什麼,但又不至於讓人難以閱讀。

註釋

開發者有沒有寫出清晰易懂的註釋?所有的註釋都是必要的嗎?通常註釋應該解釋清楚為什麼這麼做,而不是做了什麼。如果代碼不清晰,不能清楚地解釋自己,那麼代碼可以寫的更簡單。當然也有些例外(比如正則表達式和複雜的算法,如果能夠解釋清楚他們在做什麼,肯定會讓閱讀代碼的人受益良多),但大多數註釋應該指代碼中沒有包含的信息,比如代碼背後的決策。

變更中附帶的其他註釋也很重要,比如列出一個可以移除的待辦事項,或者一個不要做出代碼變更的建議,等等。

注意,註釋不同於類、模塊或函數文檔,文檔是為了說明代碼片段如何使用和使用時代碼的行為。

風格

在谷歌內部,主流語言甚至部分非主流語言都有編碼風格指南 ,確保變更遵循了這些風格規範。

如果你想對風格指南中沒有提及的風格做出改進,可以在註釋前面加上“Nit:”,讓開發人員知道這是一個你認為可以改進的地方,但不是強制性的。但請不要只是基於個人偏好來阻止變更的提交。

開發人員不應該將風格變更與其他變更放在一起,這樣很難看出變更裡有哪些變化,讓合併和回滾變得更加複雜,也會導致更多的其他問題。如果開發者想要重新格式化整個文件,讓他們將重新格式化後的文件作為單獨的變更,並將功能變更作為另一個變更。

文檔

如果變更改變了用戶構建、測試、交互或者發佈代碼相關的邏輯,檢測是否也更新了相關文檔,包括READMEs, g3doc頁面,以及任何相關文檔。如果開發者刪除或者棄用某些代碼,考慮是否也應該刪除相關文檔。如果文檔有確實,讓開發者補充。

代碼細節

查看你整個Code Review中的每一行代碼,比如你可以掃到的數據文件,生成的代碼或大型數據結構,但不要只掃一遍手寫的類,函數或者代碼塊,然後假設它們都能正常運行。顯然,有些代碼需要仔細檢查,這完全取決於你,但你至少應該理解所有的代碼都在做什麼。

如果你很難看懂代碼,導致Code Review的速度慢了下來,你要讓開發者知道,並且在Review前澄清原因。在谷歌,我們有最優秀的工程師,你也是其中之一。如果你都看不懂,很可能其他人也看不懂。所以要求開發者理清代碼邏輯也是在幫助未來的開發者。

如果你理解代碼,但又覺得沒有資格做代碼評審,確保有資格的變更評審人員在代碼評審時考慮到了安全性、併發性、可訪問性、國際化等問題。

上下文

看改動上下文代碼對Code Review很有幫助,因為通常Code Review工具只會顯示改動部分上下幾行代碼,但有時你必須查看整個文件確保這次變更可以正常運行。例如,你可能看到加了4行代碼,但是看到整個文件時才會發現這加的4行代碼是在一個50多行的方法裡,這個方法現在應該被拆分為多個小的方法了。

Code Review時考慮到整個系統的上下文也很重要,這次改動提升了系統健康度?或者增加了系統複雜性?少了測試用例?…… 不要通過哪些會損害系統健康的代碼。很多系統變複雜都是因為一點一點的小改動日積月累造成的,所以千萬不要放進來任何增加複雜性的改動。

亮點

如果你看到變更中做的好的地方,告訴開發者他們做的很棒,尤其是當他們用某種很精巧的方式解決你評論中提到的問題時更應不吝讚美。Code Review過多

關注於錯誤,但你也應該為開發者展現出好的一面點贊。在指導他人的時候,有時候告訴開發者正確的做法比告訴他們錯誤的做法更有價值。

總結

在做Code Review時,你需要注意以下:

  • 代碼設計良好。

  • 代碼功能對用戶有用。

  • 任何UI改動應當是深思熟慮過而且看起來不錯的。

  • 保證線程安全。

  • 代碼沒有增加不必要的複雜性。

  • 開發者沒有寫有些將來需要但現在不知道是否需要的東西。

  • 代碼有適當的單元測試。

  • 測試邏輯設計良好。

  • 開發者使用了清晰明瞭的命名。

  • 註釋清晰明瞭實用,通常解釋清楚了為什麼這麼做,而不是做了啥。

  • 代碼又相應完善的文檔。

  • 代碼風格符合規範。

確保你review了要求你看的每一行代碼,確保你正在提升代碼質量,並且為開發者做的提升點贊。

Code Review步驟

概要

現在你知道了要從CL中得到什麼,那能在眾多文件中完成評審的方法是什麼?

  1. 這條改動是否生效?這次變更是不是描述清晰?

  2. 首先閱讀CL最重要的一部分,整體上是否設計良好?

  3. 按照合適的順序閱讀剩下的變更。

第一步: 綜觀這個改動

閱讀CL描述並且明白CL大體內容。這些修改是否有意義?首先如果這個修改不應該有,請立刻說明為什麼這些修改不應該有。當你因為這個拒絕了這次改動提交時,告訴開發人員該怎麼去做是非常好的。

例如,您可能會說:“看起來您為此做了一些出色的工作,謝謝!但是,我們實際上正著手刪除FooWidget系統,您正在此處進行修改,因此我們不想進行任何新的修改現在。所以,您可以重構我們的新BarWidget類嗎?"

請注意,審閱者不僅拒絕了當前的CL並提供了替代建議,但他們禮貌地做到了。這種禮貌是重要,因為我們想表明我們甚至在開發人員之間也相互尊重,尤其是當我們意見不同時。

如果您得到的多個CL並且您不想進行的更改,您應該考慮重新設計團隊的開發流程或發佈的外部貢獻者的流程,以便在CL完成之前進行更多的交流。最好在做大量工作之前告訴人們“不必做”,因為現在要將其丟棄或徹底重寫。

第二步: 檢查CL的主要部分

查找屬於此CL的“主要”部分的文件。通常來說一個CL最重要的部分是它邏輯修改數最多的那個文件。這樣有助於瞭解相關的CL,通常來說會加快code review。如果CL太大,您無法確定哪些部分是主要部分,請開發人員告訴你應該首先看什麼,或要求他們將CL拆分為多個CL。

如果您發現CL的這一部分存在一些主要的設計問題,則即使您現在沒有時間審查CL的其餘部分,也應立即寫下這些評註。實際上,檢查其餘的CL可能會浪費時間,因為如果設計問題足夠嚴重,那麼許多其他正在檢查的代碼將消失並且無論如何都不會發生。

立即寫下這些主要設計評註非常重要有兩個主要原因:

  • 開發人員通常會發送給審核者一個CL,然後在等待審核時立即基於該CL進行新工作。如果您正在審查的CL中存在重大設計問題,那麼他們也將不得不重新設計其以後的CL。你能在他們做太多無用功之前制止他們。

  • 重要的設計變更比小的變更需要更長的時間。開發人員幾乎都有截止日期;為了在截止日期之前完成工作,並在代碼庫中保留高質量的代碼,開發人員需要儘快開始CL的所有主要的重做。

第三步:依序閱讀CL的其餘部分

確認CL整體上沒有大的設計問題後,請嘗試找出邏輯順序來瀏覽文件,同時還要確保不要錯過對任何文件的審查。通常,在瀏覽了主要文件之後,按照代碼審查工具向您展示它們的順序瀏覽每個文件是最簡單的。有時在閱讀主要代碼之前先閱讀測試代碼也是有幫助的,因為這樣您就可以知道更改應該做什麼。

Code Review的速度

為什麼Code Review應該快?

在谷歌內部,我們在持續優化團隊開發新產品的速度,而不是優化單個開發人員編寫代碼的速度。個人開發的速度固然重要,但它沒有團隊整體的速度那麼重要。

如果Code Review速度慢了,可能會發生以下這些事:

  • 整個團隊的速度會降低,是的,你不快速響應別人的Code Review也可以完成其他的工作。但是,整個團隊的新功能、bug修復可能會因為沒有人做cr被延遲幾天、幾周甚至幾個月。

  • 開發者應維護Code Review的流程,如果審查者很少回覆Code Review,但是每次都對CL提出大量的改動,這可能會打擊到開發者。通常,開發者可能會抱怨審查者太嚴格。如果審查者能在開發者更新後快速響應,並提出有實質性提升的建議(能顯著提升代碼運行狀況的CL),抱怨就會消失。Code Review中絕大多數抱怨會隨著CR速度的提升而消失。

  • 可能影響到代碼質量。如果CR慢了,可能會給開發者一種需要提交不太好代碼的壓力。CR慢了,也會影響到代碼清理、重構、和現有CL的進一步提升。

應該以什麼樣的速度做Code Review?

如果你不是在做需要高度專注的任務,Code Review應該越快越好。應當在一個工作日之內回應Code Review請求。遵循這些指導意味著典型的CL應該在一天內進行多輪審查(如果需要)。

速度 vs 中斷

只有一種情況下,個人的速度要勝過團隊速度。如果您正在處理諸如編寫代碼之類的重要工作,請不要打斷自己的工作去做Code Review,研究人在專注中被打斷後需要很長的時間才能重新恢復到專注狀態。所以,為了不讓其他開發者等會而且中斷自己的編碼工作,明顯得不償失。

所以,建議你在工作斷點的時候回應Code Review,比如寫完代碼、午飯後、會議結束後、從茶水間回來……

快速響應

當我們談論Code Review的速度時,一方面是指響應時間,另一方面是指整個Review從提交到通過的時間。整個過程應該足夠快,但是對於每個人來說,迅速做出反應比迅速完成整個過程更為重要。

即使有時需要很長時間才能完成整個Code Review流程,評審者在整個過程中的快速響應也可以大大緩解開發人員因為Code Review“慢”而產生的挫敗感。

如果你太忙不能全身心投入到Code Review中,你也應該讓開發者知道你什麼時候會去Review,或者建議其他評審者快速響應,再或者提供一些初步的建議。(注意:這不是建議你中斷自己的工作,而是在工作間隙合理響應)

評審者有必要花時間去做Code Review來保證代碼符合標準,不管怎麼樣,每個回應應當保證足夠快速。

跨時區Code Review

當遇到跨時區的Code Review時,儘量在作者回家前回復。如果他們已經回家了,儘量在他們每天來公司前完成Code Review。

LGTM和註釋

為了讓CR更快,有些情況下也應該讓CR提前通過,即便有些評論沒有被解決,比如:

  • 審查者信任開發者能適當解決所有評審者的建議。

  • 其餘的改動很小,開發者不必做。

除非另有明確說明,評審者應指明他們打算使用這些選項中的哪一個。

當開發者和評審者在不同時區時,應當著重考慮下通過CR,否則開發者還得等一天。

大的CL

如果有人給你提交了一個非常大的Code Review,你也不確定你有時間看,你最好建議開發者把CL拆分成幾個小的部分,分多次Code Review,而不是一次性全部提交上來。這即便開發者多做一些額外的工作,也是可以把它拆分開的,而且拆分也有利於評審者。

如果CL不能拆分成多個小CL,你也沒有時間快速完整的Review整個代碼,只是對整體設計提一些建議,然後發回給開發者改進。作為評審者,你的目標之一是在不犧牲代碼質量的前提下,不阻礙開發者的進程或者儘可能讓他們向前推進。

持續提升Code Review

如果你遵從這些建議並在Code Review中嚴格執行這些準則,你就會發現整個Code Review的流程會越來越快。開發者將瞭解健康代碼的要求,並從一開始就交出完美的代碼,然後Code Review的時間會越來越少。評審者將學會如何快速做出響應,並且不是在這個Code Review過程中增加不必要的延遲。

但是,不要為了提升速度犧牲Code Review的標準和代碼質量。從長遠來看,這並不會提升速度。

緊急情況

當然也有一些緊急的CL需要快速走完這個Code Review流程,這時候在質量上的把控可以稍微放鬆一些。可以參考緊急事件一文來了解哪些是緊急事件哪些不是。

怎樣寫代碼評論

概要

  • 禮貌

  • 解釋你的觀點.

  • 明確指出方向和問題,幫助開發人員去權衡作出決定.

  • 鼓勵開發人員通過註釋和精簡代碼來解決你的困惑而不是通過解釋

禮貌

通常來說當你Code Review代碼時保持禮貌和尊重能使開發人員更加清晰,得到更多幫助。這樣是為了保證你的代碼評論僅僅針對的是code而不是針對開發人員。你不必一直這麼去做,但是當你的評論會讓開發人員生氣或者產生爭執時有必要這麼去做。比如:

不好的例子: “你為什麼會在這裡使用線程,這樣做難道會有任何好處?”

好的例子: "我並沒有發現這個併發模塊給程序帶來了多少幫助,並且還增加了程序的複雜性,因此我認為這段代碼最好是用單線程而不是多線程。

解釋清楚原因

從上面“好”的例子當中你能發現,這樣有助於開發人員理解為什麼你寫了這些評註。你不一定非得包含這些信息在你的評註裡面,但是適當的多解釋你的意圖或者多給出一些提升代碼質量的建議都是非常好的實踐。

給予指導

通常來說修復CL是開發人員的職責而不是評審人員的。你不需要向開發人員提供詳細的解決方案或者代碼。

但是這並不意味著評審員就不應該提供幫助。你需要在指出問題和提供直接指導之間找到平衡。指出問題並且幫助開發人員決策能夠幫助開發人員學同事讓code review變得更加簡單。這樣也能產生更好的方案,因為開發人員比評審者更加了解代碼。

儘管這樣,有時候直接給出指導,建議甚至是代碼更有幫助。code review的主要目的是儘可能得到最好的CL。其次是提高開發人員的技能這樣就能減少以後評審的次數。

接受解釋

與其要求讓開發人員解釋一段你看不懂的代碼,其實更應該做的是讓他們重寫代碼,讓代碼更清晰。當一段代碼不是太過於複雜的時候通過加一些註釋偶爾也是一種不錯的做法。

解釋僅僅是寫在Code Review工具裡的,不利於將來的代碼閱讀者。只有極少數情況是可行的,比如你對你評審的需求不太熟悉,但是開發人員解釋的是大多數人都知道的。

處理Code Review中的反駁

有時開發者會在Code Review中反駁你,他們可能不同意你的意見,或者抱怨你太嚴格了。

誰是誰非?

當開發者不同意你的建議時,首先花點思考下他們是否是對的,但通常而言你比他們更熟悉代碼,所以可能在某個方面理解更深。他們的爭論有意義嗎?從代碼健康的角度來看他們的反駁有意義嗎?如果是,讓他們知道他們是對的,然後這個問題就解決了。

然而,開發者不總是對的,這種情況下評審者應當進一步介紹為什麼他們的建議是對的。好的解釋不僅得體現出對開發人員回覆的理解,而且還要說明為什麼要這麼改。

如果評審者認為他們的建議可以改善代碼質量,並且他們認為帶來的代碼質量改進值得開發者做這些額外的工作,評審者就應該堅持自己的立場。

不積跬步無以至千里,不積小流無以成江河

有時候要讓開發者接受,你得花很多時間反覆解釋,但始終確保該有的禮貌,並讓開發者知道在知道他們了什麼,即便是你不同意。

惹惱開發者

有時評審者會認為堅持讓開發者做出改動,可能會惹惱開發者。有時開發者確實會惱怒,但這種情況通常都很短暫,而且之後他們會感激你幫助他們提高了代碼質量。如果你在Code Review中很有禮貌,開發者根本不會被惹惱,這種擔心是多餘的。通常,令惹惱開發者的是你寫註解的方式,而不是你對代碼質量的堅持。

稍後解決

一種常見的反駁原因是開發者希望能儘快完成任務。他們不想一輪又一輪地做Code Review,然後就會說他們會在後續CL中處理這些問題,你只需要通過就行。有些開發者做的很好,他們會立馬提交後續CL處理這些問題。然而,經驗告訴我們原始CL通過之後拖的時間越久,就越不可能修復。除非開發者在當前CL通過後立馬修復,否則他們就不可能修復。這並不是開發者不負責任,而是因為他們有好多工作要做,而修復工作也會因為工作壓力而被遺忘。所以最好堅持讓開發者現在就在CL中處理掉這些問題,“留著以後清理”是一種不可取的方式。

如果CL中引入了新的複雜性,提交之前必須清理掉,除非是緊急情況。如果CL中暴露出一些目前還無法定位的問題,開發者應該記錄下這些bug,並將其分配給他們自己,確保這些問題不會被遺忘。他們還可以在代碼中加入 TODO 註釋,指向已經記錄好的 bug。

抱怨太嚴格

如果你之前Code Review很寬容,然後突然變得嚴格起來,可能會引起一些開發者的抱怨。不過沒關係,加快Code Review的速度通常會讓這些抱怨消失。

有時,這些抱怨可能需要幾個月的時間才能消除,但最終開發者在看到產出的優質代碼時會理解嚴格Code Review帶來的價值。有時候,一旦發生某些事讓他們真正看到嚴格Code Review的價值,抗議最大聲的人甚至會成為你最堅定的支持者。

解決衝突

如果你遵循了上述方法,但仍然會在Code Review中遇到無法解決的衝突,請再次參閱Code Review標準,瞭解那些有助於解決衝突的指導和原則。

Google 是如何做 Code Review 的?| 原力计划

開發者指南

寫一個好的CL描述

一個CL描述是做了什麼更改以及為什麼的公開記錄。

它將成為我們版本控制歷史的永久組成部分,多年來除你的評審者以外,還可能有數百人會閱讀它。

將來的開發者將會基於它的描述來搜索你的CL。

將來可能會有人由於沒有現成的細節,而根據他模糊的記憶來尋找你的改變。

如果所有重要的細節都在代碼中而不是描述中,那麼他們將很難定位你的CL。

第一行

  • 言簡意賅

  • 語義完整

  • 空行隔開

CL描述的第一行應該是對CL正在做的具體工作的簡短總結,緊跟一個空行。在未來,這是大多數的代碼搜索者在瀏覽一段代碼的版本控制歷史時會看到的,因此第一行應該足夠有信息量,他們不必閱讀你的CL或它的整個描述就可以大致瞭解你的CL實際做了什麼。

按照傳統,CL描述的第一行是一個完整的句子,就像它是一個命令(一個祈使句)。例如,說"Delete the FizzBuzz RPC and replace it with the new system.",而不是"Deleting the FizzBuzz RPC and replacing it with the new system."。

不過,你不必把其餘的描述寫成祈使句。

正文內容豐富

其餘描述應該是具有豐富的內容。它可能包括對正在解決的問題的簡要描述,以及為什麼這是最好的方法。如果這種方法有任何缺點,就應該提到。將相關的背景信息(例如bug數目,基準測試結果),以及設計文檔的鏈接。即使是很小的CL也值得關注細節。請將來龍去脈放入CL中。

反例

"Fix bug"是一個不充分的CL描述。是什麼bug?你是怎麼修復它的?

其他一些類似的不好的CL描述:

  • “Fix build.”

  • “Add patch.”

  • “Moving code from A to B.”

  • “Phase 1.”

  • “Add convenience functions.”

  • “kill weird URLs.”

這裡有一些是真實的CL描述。他們的作者可能認為他們提供的信息是有用的,但他們並沒有給出一個CL描述的意圖。

好的CL描述

這裡有一些好的CL描述的例子。

功能改變

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.

前幾個詞描述了CL描述實際做了什麼。其餘描述在說明解決的問題,為什麼這是一個好的方案,以及具體實現的細節。

重構

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的來龍去脈、解決方案的不理想以及未來可能的方向。它同樣是解釋為什麼要進行這個修改。

需要上下文的小CL

Create a Python3 build rule for status.py.

This allows consumers who are already using this as in Python3 to depend on a rule that is next to the original status build rule instead of somewhere in their own tree. It encourages new consumers to use Python3 if they can, instead of Python2, and significantly simplifies some automated build file refactoring tools being worked on currently.

第一句描述了實際做了什麼。其餘的描述解釋了為什麼要進行更改,並給了reviewer很多背景。

在提交CL之前,請檢查描述

在Review期間CL可能會經歷重大改變。在提交CL之前,有必要review CL描述,以確保描述仍然反映CL所做的事情。

簡短化的CL

為什麼要寫成一系列簡短的CL?

簡短的CL有這些好處:

  • Code Review更快 與比起花30分鐘審查一個大型的CL相比,對審查者來說花5分鐘審查一系列小的CL更加容易.

  • 審查更加徹底。進行較大的更改後,審閱者和作者往往會因大量詳細評論的來回移動而感到沮喪,有時這些評論會漏掉或遺漏重要的觀點。

  • 減少導致bug的可能性。由於您所做的更改較少,因此您和您的審閱者更容易有效地推斷出CL的影響,並查看是否導致bug。

  • 減少不必要的工作 當你寫了一個巨大的CL,然後審查者覺得你總體方向錯了,這會導致你浪費大量的工作。

  • 更方便合併代碼 因為大型的CL會導致大量的衝突,因此合併大型的CL會浪費很多時間,並且這將會是你經常做的工作。

  • 有助於你作出更好的設計 優雅的設計並且完善一個小的改動比大的改動更加容易

  • 降低審查者的難度 提審部分改動,不會影響你繼續編碼。

  • 回滾更容易 大型CL很有可能會在初始CL提交和回滾CL之間更新修改文件,從而使回滾變得複雜(中型CL也可能也需要回滾可能也會這樣)。

注意!審查者可以因為你的改動過於巨大直接拒絕掉你通常,他們會感謝您的貢獻,但要求您以某種方式使它成為一系列較小的更改。不管是你把這些改動拆分成小的改動,還是和審查者爭論讓他接受都會耗費你大量的時間。但是編寫小型改動就不會有這樣的問題。

怎麼樣算簡短?

通常,CL的正確大小是一個獨立的更改。這意味著:

  • CL所做的最小更改僅解決了一件事情。通常,這只是功能的一部分,而不是一次完整的功能。通常,寧可編寫過小的CL也不要寫太大的CL。你可以和你的審查者商量找到合適的尺度。

  • 審閱者需要了解的有關CL的所有內容(將來的開發除外)都在CL,CL的描述,現有代碼庫或他們已經查看過的CL中。

  • 檢入CL後,系統將繼續對其用戶和開發人員正常運行。

  • CL不夠小的話會導致其難以理解。如果你新增加了api,那麼在同一個CL應該包括這個api用到的方法,以便審查者更好的理解。也能防止檢入沒有用的api。

多大算大,沒有一個明確的要求。對於CL而言,100行通常是一個合理的大小,而1000行通常太大,但這取決於您的審閱者的判斷。更改分佈的文件數量也會影響其“大小”。可以在一個文件中進行200行的更改,但是將其分佈在50個文件中通常會太大。

請記住,儘管從您開始編寫代碼起就一直與您緊密聯繫,但審閱者通常沒有上下文。對您來說,看起來像可接受大小的CL可能會讓您的審閱者不知所措。毫無疑問,儘可能把CL些小是正確的。審查者很少抱怨CL太小

什麼時候大型的CL也是可以的?

在某些情況下,較大的更改沒有那麼糟糕:

  • 通常,您可以將整個文件的刪除視為更改的一行,因為它不會花費很長時間來審閱審閱者。

  • 有時,您完全信任的自動重構工具已經生成了一個較大的CL,而審閱者的工作只是檢查健全性,然後指出他們認為需要修改的地方。這些CL可能更大,儘管會有一些風險(例如合併和測試)仍然適用。

按文件分類

拆分CL的另一種方法是通過將文件分組,如果這些文件將是獨立的更改,可以分配各不同的審閱者。

比如: 你提交一個修改的CL,創建一個修改的緩衝區,另一個CL的代碼修改也可以提交到這裡面。你必須按照順序提交CL,但是審閱者可以同時進行審閱. 如果這麼做,你需要儘可能告訴所有審閱者另一個CL的信息,以便他們能得到上線文信息。

另一個示例:您發送一個CL進行代碼更改,而另一個CL則發送使用該代碼的配置或實驗;如果需要,這也更容易回滾,因為有時將配置/實驗文件推送到生產環境中比更改代碼更快。

把代碼重構分離出來

通常重構最好不要和功能修改或者bug fix一起提CL。比如移動或者重命名一個Class,最好和這個Class的bug fix分開提CL。這樣對於審閱者來說,理解每個CL單獨引入的更改要容易得多。

不過一些小的代碼清理工作比如變量重命名可以包含在功能修改或者bug fix的CL種。這取決於開發者和審閱者的判斷,當然如果巨大的重構包含在同一個CL中會大大增加審閱的難度。

將相關的測試代碼保存在同一CL中

避免將測試代碼拆分為單獨的CL。驗證代碼修改的測試應該進入相同的CL,即使這樣做會增加代碼行數。

但是根據重構準則,獨立的測試修改可以單獨寫入CL。比如

  • 使用新測試驗證先前存在的提交代碼。

  • 重構測試代碼(例如,引入輔助函數)。

  • 引入更大的測試框架代碼(例如集成測試)。

不要破壞結構

如果您有多個相互依賴的CL,則需要找到一種方法來確保在提交每個CL之後整個系統都能正常工作。否則,可能破壞代碼結構導致後面的開發者浪費大量的時間等你的提交(如果這些CL提交出了問題,則更長的時間)。

小到不能再小

有時候,您會遇到CL很大的情況。這很少是真的。練習編寫小型CL的作者幾乎總是可以找到一種將功能分解為一系列小的更改的方法。

在寫大型CL之前,思考下是不是能夠將重構分離出來,這是一個更加清晰的思路。多和你的隊友交流學習下他們對縮小CL的實踐和方法。

如果所有這些選項都失敗了(這種情況很少見),那麼請事先獲得您的審閱者的同意,告訴他們一個巨大的CL將要來臨。在這種情況下,審查過程會耗費極其長的實踐,這樣請花費更多的時間去寫測試代碼,避免引入bug。

如何處理評審者的評論

當你提交了一個變更做Code Review時,很可能你都會收到評審者在變更中的評論。這裡有一些處理這些評論的建議。

不要摻雜個人情感

Code Review的目標是維護代碼庫和產品的質量。如果評審者批評了你的代碼,可以理解為他們在幫你、幫整個代碼庫、甚至是幫整個公司,而不是攻擊你或者是質疑你的能力。

有時候評審者會情緒低落,然後在評論中說出一些令人沮喪的話,雖然評審者這樣做不對,但作為開發者你應當有心理準備。問問你自己,“評審者試圖與我交流的建設性意見是什麼?” 然後照他們說的那些去做。

永遠不要回應充滿怒氣的評論,Code Review工具中違反職業禮儀的情況永遠存在。如果你真的忍無可忍,建議先離開電腦也會兒,或者幹一些其他的事,等心情平復下來再回復。

通常,如果評審者沒有以禮貌的方式提供反饋,請親自向他們解釋。如果你無法親自或者通過視頻和他們聯繫,就向他們發一份私人郵件。友好地告訴他們你不喜歡的事情以及你希望他們做些什麼。如果他們也以非建設性的方式對此私密討論做出回應,或者沒有起到預期的作用,請酌情上報給您的經理。如果他們已經以不禮貌的方式回應,或者沒有取得預期的效果,視情況彙報給你的經理。

修復代碼

如果評審者說他們理解不了你代碼中的某些內容,你首先應該把代碼寫清晰。如果讓代碼更清晰,添加註釋來解釋清楚代碼的邏輯。如果評論似乎毫無意義,那麼您的答覆應該只是代碼查看工具中的解釋。

如果評審者無法理解你的某部分代碼,那邊可能未來的閱讀者也可能理解不了。在Code Review工具中回應幫不了未來的讀者,但是代碼中的註釋可以。

自我反思

寫一個變更會花費你很大的精力,提價Code Review時會感覺如釋負重,而且自己也相當確定所有工作已經做完了。所以當評審者提出改進建議時,你很容易認為那些都是錯的,或者認為是評審者給你不必要的阻撓,再或者覺得評審者應該讓你提價變更。無論如何,不管你怎麼想,花點時間回想下評審者給你的反饋有助於提升公司的代碼質量。你始終問下自己“如果評審者是對的呢?”

如果你回答不了評審者的問題,那可能說明評審者的評論不夠清楚。

如果你認真考慮過後依舊認為你是對的,放心大膽地解釋清楚為什麼你的方法對公司更有利。通常,評審者只是提供建議,並且希望你能思考出更好的方法。也許你已經知道一些評審者不知道的關於用戶、代碼庫、或者變更,把這些都寫下來,給評審者更多的上下文信息,通常你都可以根據某些事實和評審者達成某些共識。

解決衝突

解決衝突的第一步,和你的評審者達成共識,如果無法達成共識,參閱Code Review的標準獲取更多內容。

版權聲明:本文為CSDN博主「帥昕 xindoo」翻譯,版權歸作者所有。

技術的道路一個人走著極為艱難?

一身的本領得不施展?

優質的文章得不到曝光?

別擔心,

即刻起,CSDN 將為你帶來創新創造創變展現的大舞臺,

掃描下方二維碼,歡迎加入 CSDN 「原力計劃」!

Google 是如何做 Code Review 的?| 原力计划

【End】


分享到:


相關文章: