當前位置:編程學習大全網 - 編程語言 - Google: 如何做code review?

Google: 如何做code review?

本文的名詞解釋:

如果嫌文章太長,可以直接拖到文章最後看總結

cr的標準

cr(Code review)主要目的在於確保Google 的代碼庫代碼質量越來越好。而所有相關的工具與流程皆是因應這個目的而生。為達到此目的,勢必需要做出壹連串的權衡與取舍。

首先,開發人員必須能夠在自己負責的任務上有所進展。如果妳從來沒有向代碼庫提交改進過的代碼,那麽代碼庫就永遠不會改進。另外,如果壹個reviewer使cr都很難進行的話,那麽開發人員就不願意在將來進行改進。

另壹方面,reviewer有責任確保每個change list( 下稱CL )的質量,保證其代碼庫的整體代碼質量不會越來越差。這可能很棘手,因為通常會隨著時間的推移,代碼需要降級才能讓代碼運行起來,特別是當團隊受到嚴重的時間限制時,大家覺得必須走捷徑才能實現他們的目標。

此外,reviewer對他們正在review的代碼擁有所有權和責任。他們希望確保代碼保持壹致、可維護,以及下文的“ 在cr中可以得到什麽 ”中提到的內容。

因此,我們有以下規則,作為我們在cr中所期望的標準:

壹般來說,當CL存在的時候,reviewer應該贊成它,此時它肯定會提高正在工作的系統的整體代碼質量,即使這個CL並不完美。這是所有cr中的首要原則。當然,這是有局限性的。例如,如果壹個CL添加了壹個reviewer不希望在其系統中使用的功能,那麽reviewer當然可以拒絕,即使代碼設計得很好。

這裏的壹個關鍵點是,沒有“完美”的代碼,只有更好的代碼。reviewer不應該要求作者在approve之前對壹篇文章的每壹小段進行潤色。相反,reviewer應該權衡發展的需要和他們所建議的change的重要性。reviewer不應追求完美,而應追求持續改進。作為壹個整體,提高系統的可維護性、可讀性和可理解性的CL不應該因為它不是“完美的”而被延遲幾天或幾周。

reviewer應該隨時可以留下評論,表達壹些更好的東西,但如果不是很重要,可以在評論前加上“nit:”這樣的前綴,讓作者知道這只是壹個他們可以選擇忽略的修飾點。

cr有壹個重要的功能,教開發人員壹些關於語言、框架或壹般軟件設計原則的新知識。留下有助於開發人員學習新知識的評論是可以的。隨著時間的推移,***享知識是提高系統代碼 健康 度的壹部分。妳只需要記住,如果妳的評論純粹是教育性的,但對達到本文檔中描述的標準並不重要,請在其前面加上“nit:”,或者以其他方式表明作者不必在本CL中解決它。

現實和數據推翻了個人喜好。在代碼風格方面,風格指南(style guide)是絕對的權威。任何不在style guide中的壹些點(如空格等)都是個人偏好的問題。代碼風格應該與現有的壹致。如果項目沒有以前的統壹風格,那就接受作者的風格。

軟件設計的各個方面從來都不僅僅是壹個純粹的代碼風格問題或者是個人喜好問題。它們是以基本原則為基礎的,應當以這些原則為依據,而不僅僅是以個人意見為依據,有時幾乎都沒有選擇的。如果作者能夠證明(通過數據或基於原理的壹些事實)他的方法是同樣有效的,那麽reviewer應該接受作者的偏好。否則,代碼風格選擇取決於軟件設計的標準原則。

如果沒有其他規則適用,那麽reviewer可以要求作者與當前代碼庫中的內容保持壹致,只要這些代碼不會惡化系統的整體代碼 健康 狀況。

在cr的任何沖突中,第壹步應該始終是開發人員和reviewer根據本文和《CL Author’s Guide》,嘗試達成***識。

當達成***識變得特別困難時,reviewer和作者需要進行面對面會議,而不是僅僅試圖通過cr的註釋來解決沖突(不過,如果這樣做,請確保將討論結果記錄在CL的評論中,以供將來的讀者閱讀)。

如果這不能解決問題,最常見的解決方法就是升級。通常情況下,升級的途徑是進行更廣泛的團隊討論,讓team leader參與進來,請求代碼維護人員做出決定,或者請求技術經理提供幫助。不要因為作者和審稿人不能達成壹致意見而讓壹個其他人袖手旁觀。

在cr中要看些什麽

註意:在考慮每壹點時,壹定要考慮cr的標準。

在cr中重要的是看CL的總體設計。CL中不同代碼段的交互是否有意義?此更改屬於妳的業務代碼庫還是屬於引進來的其他代碼庫?它是否與系統的其他部分很好地集成?現在是添加此功能的合適時機嗎?

這個CL做了開發者想要的嗎?開發者對這些代碼的設計初衷用戶有好處嗎?“用戶”通常既是最終用戶(當他們受到更改的影響時)又是開發人員(他們將來必須“使用”此代碼)。

大多數情況下,我們希望開發人員在進行cr時能夠對CL進行充分的測試,使其能夠正常工作。但是,作為reviewer,仍然應該考慮邊緣狀況,尋找問題,嘗試像用戶壹樣思考,並確保僅通過閱讀代碼就不會看到錯誤。

如果妳願意的話,妳可以自己去驗證CL。如果改動會直接帶來的用戶可見的影響,比如說ui改動,驗證CL的變化是非常關鍵的。

在閱讀代碼時,很難理解某些更改會對用戶產生怎樣的影響。對於這樣的更改,如果不方便自己測試,則可以讓開發人員演示該功能(demo)。

另外,在cr期間考慮功能性特別重要的點是,cl中是否並發式編程,理論上可能導致死鎖或競爭條件。這些類型的問題很難通過運行代碼來發現,通常需要有人(開發人員和reviewer)仔細考慮,以確保不會引入問題(註意,這也是不使用平行式編程的壹個很好的理由,在這種情況下,可能出現競爭條件或死鎖,這會使代碼檢查或理解代碼變得非常復雜)。

壹個CL是否復雜到超過預期的必須?針對任何層級的CL必須確認這幾點:單行代碼是否過於復雜?函數是否過於復雜?class是否過於復雜?“復雜”通常意味著該代碼很難閱讀,很有可能也意味著其他開發者在修改這段代碼的時候引入其他bug。

其中壹種復雜性就是過度設計(Over-engineering)造成的,開發者會讓那段代碼過度通用化,超過了原本所需,或者還新增了系統目前不需要的功能。reviewer應特別註意壹下過度設計。鼓勵開發者解決他們知道現在需要解決的問題,而不是推測將來可能需要解決的問題。當那些將來出現的問題出現的時候才開始解決它們,因為那時候妳可以更加清晰看見問題的原樣子。

請將單元測試、整合測試、端到端測試視為要求CL所做的適當變更。壹般CL內除了生產環境的業務代碼外,測試也應該要被加入其中。除非該CL是為了處理某個緊急事情而存在。

另外,也要確保測試是正確、合理、有用的。測試並非來測試它們本身,壹般也極少為了測試而測試(如測試壹下測試代碼有沒有問題又走了測試流程),因此我們要保證測試是有效的。

當代碼真的有問題,測試是否會失敗?如果被測試的程序發生改動時,測試是否會產生誤報?每壹個測試是否做出了簡單而有用的斷言?不同的測試方法之間的測試是否適當分開?

命名

開發者是否為了每壹個東西都挑了壹個適當的名字?壹個好的命名意味著,通過名字就足以完整表達該東西的作用是什麽或者要做什麽。但是同時名字也不要長得難以閱讀。

推薦參考文章:Clean code 101 — Meaningful names and functions

註釋

開發者是否用可理解的英文留下清晰的註釋? 這些註釋是否真的必要?

通常註釋是解析這段代碼為什麽存在的時候是相當有用的,而不應該去解釋某段代碼正在做什麽。如果代碼本身不能解釋清楚的話,意味著它更加需要簡化了。當然也有例外,比如解釋正規的表達式或者復雜的算法正在做什麽的時候,註釋解釋這段代碼正在做什麽就相當有用。但對於大部分註釋來說是用來說明那些不包含在程序本身但資訊,比如說為什麽要這樣子做的理由。

查看該CL之前的註釋也很有幫助,或許有壹個todo項目現在可以壹處、壹個評論建議為什麽不要進行這種更改等等。

要註意的是,註釋與class、module、function的文件不同。後三者要能夠表達壹段代碼的目的、如何使用它、使用時行為。

Google 對於主要語言都有提供風格指南(style guide),甚至包括大多數冷門語言,因此要確保CL遵循適當的指南上的說明。

如果妳想改進CL中某些不包含在風格指南中的要點時,請在評論前加上Nit: ,讓開發人員知道這是妳認為可以改善代碼的小問題且並非強制性的。但記住,不要僅根據個人風格偏好阻止提交CL。

開發者不應該在 CL內同時包含主要風格的改動與其他代碼的修改,這樣會導致難以看出CL到底做出什麽改動。同時也會讓合並和回滾更為復雜,並產生其他問題。例如,如果作者想要重新格式化代碼的話,讓他們將新格式在壹個新CL裏面重構。

如果CL更改了構建、測試、交互、發布的時候,請檢查是否也同時更新相關文檔, 包括README,g3doc 頁面和其他生成(generated) 的參考文件。如果CL 刪除或棄用 了壹些代碼,請考慮是否應該刪除對應文檔,如果缺少文檔時請詢問。

仔細review分配給妳的每壹行代碼。有些東西,比如資料文件(data files)、生成的代碼(generated code)、大型數據結構(large data structures),妳可以稍微掃過。千萬不要在掃過開發者寫的 class、函數、代碼區塊 時,去假設它內部是沒問題的。很顯然的某些代碼需要比其他代碼更仔細的review。這是必須由妳做出的判斷,但至少妳應該確定妳理解所有代碼在做些什麽。

如果閱讀代碼過於復雜並且減慢review速度時,那麽妳再繼續review前,要讓開發者知道這件事,並等待他們為這段代碼做出解釋、說清楚。在Google 我們聘請許多優秀的軟件工程師,而妳也是其中的壹員。如果連妳也無法理解的話,很可能其他人也不會。因此,妳要求開發者去說清楚這段代碼時,同時也在幫助未來的開發人員理解這些代碼。

如果妳能夠理解,但覺得沒有資格進行某部分的審核,請確保 reviewer中有壹個適合(合格) 的人來review該部分。尤其是針對安全性、並發性、可訪問性、國際化等復雜問題時。

在充足的上下文下查看CL通常很有幫助。壹般來說,cr工具只會顯示修改部分周圍的幾行代碼而已。但有時妳必須查看整個文件以確保改動是否合理。比方說,妳可能只看到添加4行新代碼,但實際上查看整個文件時會發現這4行是被加在有50行的代碼裏,此時需要將它拆解為更小的函數。

以整個系統的角度出發來思考CL也是很有用的。該CL是否改善整體系統的代碼質量,亦或是會讓整個系統更加復雜?是否缺少測試?千萬不要接受會降低整體系統的代碼質量的CL。因為大多數系統是由於許多小改動的積累而變得復雜,因此阻止新的改動引入復雜性(盡管很小)也非常重要。

個人認為並非不要指出錯誤,而是多以鼓勵來代替指出錯誤,讓其他開發者更有動力想將事情做好。其實透過簡單的壹句話讓對方知道哪裏做得很好,未來他們會將繼續保持下去,並為其他開發者帶來的正面影響。

標明每個commit 修改什麽,幫助reviewer快速了解情況

此時就不要吝嗇妳的稱贊了!

在cr時,請務必確保:

如何瀏覽CL

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

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

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

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

如果妳有幾個CL裏包含妳不想這樣做的改動時,妳應該重新考慮開發團隊的開發過程,或發布開發流程給外部貢獻者知道,以便在任何CL被撰寫前有更多的溝通。最好是在他們開始投入前說“不”,避免那些已經投入心血的工作現在必須被拋棄或徹底重寫。

提供替代方案讓對方知道該怎麽做,而非讓其自行獨自摸索。

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

找到CL最核心的部分的那些文件。通常CL內會有文件包含大量的邏輯改動,而它正是CL的主要部分。因此我們要首先查看這些主要部分。這有助於為CL的其他較小部分提供適當上下文,而且這樣通常可以提高review速度。如果CL太大導致於無法確定哪裏是主要部分時,請向開發者詢問首先應當查看的內容,或者要求他們將CL拆分為多個CL。

如果在主要部分發現存在壹些主要的設計問題時,即使沒有時間立即查看CL的其余部分,也應立即留下評論告知此問題。因為事實上,因為該設計問題足夠嚴重的話,繼續review其他部分的代碼可能只是浪費時間,因為其他正在審查的其他代碼可能都將無關緊或消失。

立刻發送關於主要設計的評論非常重要有兩個主要原因:

壹旦確認整個CL沒有重大的設計問題時,試著找出壹個有邏輯順序來review剩余檔案,並確保不會錯過其中任何壹個。通常在瀏覽主要部分後,最簡單的方式是按照cr工具提供的順序來瀏覽每個文件。有時在閱讀主要代碼前先閱讀測試也是非常有幫助的,如此壹來妳就可以了解應該做、看些什麽。

在Google我們優化開發團隊***同生產產品的速度,而不是優化個人開發的速度。個人的開發速度很重要,但它不如整個團隊的開發速度重要。當cr很慢時,會發生以下幾件事:

如果妳並沒有處於需要專註工作的時候,那麽妳應該在CL被提交後盡快進行review。review回復最長的極限是壹個工作日。若遵循以上指南,意味著壹般的CL應該在壹天內得到多輪review(如果必要的話)。

但有時候個人的速度優先度會勝過團隊速度。如果妳處於需要專註工作的時候(比方說寫代碼),不要打斷自己去做cr。

研究證實,若開發者在被打斷後會需要很長時間才能恢復到原本順暢的開發流程。因此,開發的時候,打斷自己實際上會比讓另壹位開發者等待review來得更加昂貴。

取而代之的是,我們可以在投入到處理他人給的review評論之前,找個適當的時機點來進行cr。這有可能是當妳的當前開發任務完成後、午餐、剛從會議脫身或從微型廚房回來等等。

總的來說個人回應評論的速度,比起讓整個cr過程快速結束來得更為重要。即使有時需要很長時間才能完成整個流程,但若在整個過程中能快速獲得來自reviewer的回應,這將會大大減輕開發人員對於緩慢的cr過程的挫敗感。

如果真的忙到難以抽身而無法對CL進行全面review時,妳依然可以快速的回應讓開發者知道妳什麽時候會開始審核、建議其他能夠更快回復reviewer,又或者提供壹些初步的廣泛評論。(註意:這並不意味著妳應該中斷開發去回復——請找到適當的中斷時間點去做)。

很重要的是,reviewer員要花足夠的時間來進行review,確保他們給出的LGTM,意味著“此代碼符合我們的標準”。

盡管如此,理想的個人的回應速度還是越快越好。

在面對時區不同的問題時,盡量在他們還在辦公室時回復作者。如果他們已經回家了,那麽請確保在他們第二天回到辦公室前完成。

為加快速度,在某些情況下reviewer可以給予LGTM或Approval,即便CL上仍有未解決的評論。類似情況會發生在:

LGTM 評論對雙方處於不同的時區時尤其值得考慮,否則開發人員會等待壹整天才得到“LGTM,Approval”。

如果有人要求reivew時,但由於改動過於龐大導致妳難以確定何時才有時間review它時,妳通常該做的是要求開發人員將CL拆解成多個較小的CL,而不是壹次review巨大的CL。這種事是可能發生的,而且對於reviewer非常有幫助,即便它需要開發人員付出額外人力來處理。

如果CL無法分解為較小的CL,且妳沒有足夠時間快速查看整個CL內容時,那麽至少對它的整體設計寫些評論,並發送回開發人員以便進行改進。身為reviewer,妳的目標之壹是在不犧牲代碼質量的狀況下,避免阻擋開發人員進度,或讓他們能夠快速采取其他更進壹步的動作。

如果妳遵循這些準則,並且對於cr非常嚴格的話,後面妳會發現整個cr流程會越來越快。因為開發者學到什麽是質量好的代碼,並於開頭就提交壹個很棒的CL,而這正恰好只需要越來越少的時間。而reviewer則學會快速回應,而非在過程中加入不必要的延期。

但不要為提高想象中的速度,而對cr標準和代碼質量做出妥協,畢竟從長遠來看它實際上並不會讓任何事情發生得更快。

在某些緊急情況下,CL會希望放寬標準以求迅速地通過整個cr過程。但請看什麽是緊急情況來知道哪些情況實際上屬於緊急情況,而哪些不符合。

有時開發人員會推遲處理cr產生的評論。要麽他們不同意妳的建議,要麽他們會抱怨妳太嚴格了。

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

但開發人員也並不總是對的。在這種情況下,reviewer應該更進壹步解釋為什麽認為自己的建議是正確的。壹個好的解釋通常展示了“對開發人員的回覆的理解”以及有關“為什麽被要求做出改動”等信息。尤其是當reviewer認為給出的建議會改善代碼質量時,便應該繼續宣揚自己的論點。只要他們相信所需的額外的工作量最終會改善整體代碼質量。提高整體代碼質量這件事,往往是經由每個微小的改動來發生。有時往往需要幾番解釋壹個建議才能讓對方真正理解妳的用意。切記,始終保持禮貌,讓開發人員知道妳有聽到他們在說什麽,只是妳不同意該論點而已。

reviewer有時會認為若自己堅持改進的話,會讓開發人員覺得沮喪不安。的確開發人員有時會感到很沮喪,但這通常是十分短暫的,甚至後來他們非常感謝妳幫助他們提高代碼質量。壹般來說,只要妳在評論中表現得很有禮貌,開發人員實際上根本不會感到沮喪,這些擔憂都僅存在於reviewer心中而已。沮喪很多時候是對於cr評論的寫作方式有關,而並非來自reviewer對於代碼質量的堅持。

壹個常見的推遲原因是開發人員希望完成任務(這可以理解)。他們不想通過另壹輪cr來批準這個CL。此時他們通常會說在以後的CL進行整理,所以妳現在應該要給LGTM。當然部分開發人員非常擅長這壹點,並且立即送出壹個修復問題的後續CL (follow-up CL),但根據過往經驗,這種後續“清理行為”非常少見。除非開發者在CL獲得批準之後立刻進行清理動作,否則這事根本不可能發生。這不是因為開發人員不負責任,而是因為他們可能有很多其他工作要完成,於是清理工作便會在成堆的工作中被遺忘。因此,通常最好堅持開發人員在代碼在合並後清理它們。因為讓人們「稍後清理」是致使代碼庫質量狀況下降最常見的狀況。

如果CL引入了新的復雜性的話,除非是緊急情況,否則必須在提交之前將其處理掉。如果CL導致暴露周圍的問題且現在無法解決它們的話,開發人員應該將缺陷記錄下來並分配給自己,避免後續被遺忘。又或者他們可以選擇在代碼中留下TODO的註釋並鏈接到剛記錄下的缺陷。

如果妳先前以相當寬松的標準並轉趨嚴格的進行cr的話,壹些開發人員會開始大聲地抱怨。壹般來說,提高review的速度會讓這些抱怨逐漸消失。這些抱怨可能需要數個月才會消失,但最終開發人員會看到嚴格的review所帶來的價值,因為嚴格的review幫助他們產生的優秀代碼。而且壹旦發生某些事情時,最大聲的抗議者甚至可能會成為妳最堅定的支持者,因為他們會看到變得review變嚴格後所帶來的價值。

如果妳遵循前面所有操作,但仍然遇到無法解決的雙方之間的沖突時,請參考前面的cr的標準以獲取有助於解決沖突的準則和原則。

  • 上一篇:it培訓機構哪個比較好
  • 下一篇:各位電腦高手,小遊戲是怎麽壹回事,是用什麽制作的?怎樣制作?
  • copyright 2024編程學習大全網