【譯】Google 官方文章——如何去作coder review

時間 2019-11-06

標籤 google 官方文章如何去作 coder review 欄目 Google 简体版

原文原文鏈接

本文做者：IMWeb IMWeb團隊原文出處：IMWeb社區未經贊成，禁止轉載html

原文地址: google.github.io/eng-practic…前端

本文的名詞解釋：git

CR: code review
cl: change list，指此次改動
reviewer: CR的那個review人
nit: 全稱nitpick，意思是雞蛋裏挑骨頭
做者: 也就是本次CL的開發者，原文中是以author來稱開發者的。以老外的思惟，意思是「CL的做者」

若是嫌文章太長，能夠直接到後面看總結github

CR的標準

CR(Code review)主要目的在於確保Google 的代碼庫代碼質量愈來愈好。而全部相關的工具與流程皆是因應這個目的而生。爲達到此目的，勢必須要作出一連串的權衡與取捨web

首先，開發人員必須可以在本身負責的任務上有所進展。若是你歷來沒有向代碼庫提交改進過的代碼，那麼代碼庫就永遠不會改進。另外，若是一個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應特別注意一下過分設計。鼓勵開發者解決他們知道如今須要解決的問題，而不是推測未來可能須要解決的問題。當那些未來出現的問題出現的時候纔開始解決它們，由於那時候你能夠更加清晰看見問題的原樣子

Donald Knuth說過：過早的優化是萬惡之源(Premature optimization is the root of all evil)

測試

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

另外，也要確保測試是正確、合理、有用的。測試並不是來測試它們自己，通常也極少爲了測試而測試（如測試一下測試代碼有沒有問題又走了測試流程），所以咱們要保證測試是有效的。

當代碼真的有問題，測試是否會失敗？若是被測試的程序發生改動時，測試是否會產生誤報？每個測試是否作出了簡單而有用的斷言？不一樣的測試方法之間的測試是否適當分開？

請記住，測試代碼也是必須維護的代碼，不要由於它們不在主要關注的範圍內。

命名

開發者是否爲了每個東西都挑了一個適當的名字？一個好的命名意味着，經過名字就足以完整表達該東西的做用是什麼或者要作什麼。可是同時名字也不要長得難以閱讀

註釋

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

一般註釋是解析這段代碼爲何存在的時候是至關有用的，而不該該去解釋某段代碼正在作什麼。若是代碼自己不能解釋清楚的話，意味着它更加須要簡化了。固然也有例外，好比解釋正規的表達式或者複雜的算法正在作什麼的時候，註釋解釋這段代碼正在作什麼就至關有用。但對於大部分註釋來講是用來講明那些不包含在程序自己但資訊，好比說爲何要這樣子作的理由

查看該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。由於大多數系統是因爲許多小改動的積累而變得複雜，所以阻止新的改動引入複雜性(儘管很小)也很是重要。

優勢

當你在CL裏看到優勢時記得告訴開發者，尤爲是當他們用很棒的方式來解決你的評論時。CR一般只關注存在的錯誤，但它們也應該同時應該爲良好實踐提供鼓勵與欣賞。這點在指導開發者時尤爲重要:與其告訴他們作錯什麼，還不如告訴他們作對了什麼。

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

總結

在CR時，請務必確保：

代碼通過完善的設計
功能性對於使用者們是好的
對於任何UI改動要合理且好看
任何並行編程的實現是安全
代碼不該該複雜超過本來所必須的
開發者不應實現一個如今不用而將來可能須要的功能
代碼有適當的單元測試
測試通過完善的設計
開發者對於每樣東西有使用清晰、明瞭的命名
註釋要清楚且有用，並只用來解釋why而非what
代碼有適當的寫下文件(通常在g3doc)
代碼風格符合style guide
確保你查看被要求review的每一行代碼、確認上下文、確保你正在改善代碼質量，並讚賞開發人員所作的好事與優勢吧!

如何瀏覽CL

如今你已經知道review時要看些什麼，但怎樣纔是review分散在多個文件中的改動最有效的方法?

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

步驟1: 用宏觀的角度來看待改動，查看CL描述以及它作什麼

該改動是否有意義、合理？若是在第一時間認爲不該該發生這種變化，請當即說明爲何不應這樣作的緣由。當拒絕相似這樣的更改時，向開發人員提供建議告訴他們應該怎麼作什麼也是一個好主意。

例如，你能夠說：「看起來你已經完成一些不錯的事情，謝謝!但咱們實際上正朝着刪除你正在修改的FooWidget系統的方向前進，因此現階段咱們不想對它進行任何新的修改。不如重構咱們的新BarWidget class如何?」

須要注意的是，reviewer在委婉拒絕該CL的同時也要提供替代方案，並且仍然保持禮貌。這種禮貌是很重要，由於咱們但願代表即便不一樣意也會相互保持尊重。

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

提供替代方案讓對方知道該怎麼作，而非讓其自行獨自摸索。

步驟2: 檢查CL主要的部分

找到CL最核心的部分的那些文件。一般CL內會有文件包含大量的邏輯改動，而它正是CL的主要部分。所以咱們要首先查看這些主要部分。這有助於爲CL的其餘較小部分提供適當上下文，並且這樣一般能夠提升review速度。若是CL太大致使於沒法肯定哪裏是主要部分時，請向開發者詢問首先應當查看的內容，或者要求他們將CL拆分爲多個CL。

若是在主要部分發現存在一些主要的設計問題時，即便沒有時間當即查看CL的其他部分，也應當即留下評論告知此問題。由於事實上，由於該設計問題足夠嚴重的話，繼續review其餘部分的代碼可能只是浪費時間，由於其餘正在審查的其餘代碼可能都將無關緊或消失。

馬上發送關於主要設計的評論很是重要有兩個主要緣由:

一般開發者在送出CL後，在等待review過程當中便已經開始着手基於該CL的新工做。此時若是正在review的CL存在重大設計問題的話，開發者將不得不從新寫全部基於它的後續全部CL。所以你要在他們有問題的設計上投入以前阻止他們。
主要設計變動一般須要較長時間才能完成，但每一個開發人員幾乎都有本身deadline。爲了遇上deadline並保證代碼質量，開發者須要儘快開始或重作CL 任何的主要設計變動。

步驟3: 用合理的順序看CL 其他的改動

一旦確認整個CL沒有重大的設計問題時，試着找出一個有邏輯順序來review剩餘檔案，並確保不會錯過其中任何一個。一般在瀏覽主要部分後，最簡單的方式是按照CR工具提供的順序來瀏覽每一個文件。有時在閱讀主要代碼前先閱讀測試也是很是有幫助的，如此一來你就能夠了解應該作、看些什麼。

review的速度

爲何review速度要快

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

團隊總體的速度降低。review慢的話，對於團隊其餘人來講重要的新功能與缺陷修復將會被延遲數天、數週甚至數月，只由於它們正在或者等待review。
開發人員開始抗議CR。若reviewer每隔幾天纔回應一次，但每次都要求對CL進行重大更改的話，那麼開發人員可能會感到很是沮喪與以爲困難，而這一般會演變成抱怨。若是reviewer請求相同的實質性更改(且確實能夠改善代碼質量情況)，但在每次開發人員提交新的改動時都能快速反應的話，這些抱怨每每會消失。大多數關於CR的抱怨，每每能夠經過加快流程來解決。
代碼質量會受到影響。當review很慢時，會形成開發者提交不徹底盡如人意的CL 的壓力愈來愈大。評論的越慢也會阻止他人進行代碼清理、重構、甚至是對現有CL 的進一步改進。

CR應該要多快？

若是你並無處於須要專一工做的時候，那麼你應該在CL被提交後儘快進行review。review回覆最長的極限是一個工做日。若遵循以上指南，意味着通常的CL應該在一天內獲得多輪review(若是必要的話)。

速度vs中斷

但有時候我的的速度優先度會賽過團隊速度。若是你處於須要專一工做的時候(比方說寫代碼)，不要打斷本身去作CR。

研究證明，若開發者在被打斷後會須要很長時間才能恢復到本來順暢的開發流程。所以，開發的時候，打斷本身實際上會比讓另外一位開發者等待review來得更加昂貴。

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

快速回應

當咱們談論CR的速度時，咱們關注的是迴應時間，而非CL須要多長時間才能完成並提交。在理想狀況下，整個過程應該是快速的。

總的來講我的迴應評論的速度，比起讓整個CR過程快速結束來得更爲重要。即便有時須要很長時間才能完成整個流程，但若在整個過程當中能快速得到來自reviewer的迴應，這將會大大減輕開發人員對於緩慢的CR過程的挫敗感。

若是真的忙到難以抽身而沒法對CL進行全面review時，你依然能夠快速的迴應讓開發者知道你何時會開始審覈、建議其餘可以更快回復reviewer，又或者提供一些初步的普遍評論。(注意:這並不意味着你應該中斷開發去回覆——請找到適當的中斷時間點去作)

很重要的是，reviewer員要花足夠的時間來進行review，確保他們給出的LGTM，意味着「此代碼符合咱們的標準」。

儘管如此，理想的我的的迴應速度仍是越快越好。

跨時區review

在面對時區不一樣的問題時，儘可能在他們還在辦公室時回覆做者。若是他們已經回家了，那麼請確保在他們次日回到辦公室前完成。

LGTM 評論

爲加快速度，在某些狀況下reviewer能夠給予LGTM或Approval，即使CL上仍有未解決的評論。相似狀況會發生在:

reviewer確信開發人員會適當地處理全部剩餘的評論
剩餘的評論是微不足道的或它們不需由開發者來處理
reviewer必須清楚指明他們是指上面哪一種狀況

LGTM 評論對雙方處於不一樣的時區時尤爲值得考慮，不然開發人員會等待一成天才獲得「LGTM，Approval」。

大型改動

若是有人要求reivew時，但因爲改動過於龐大致使你難以肯定什麼時候纔有時間review它時，你一般該作的是要求開發人員將CL拆解成多個較小的CL，而不是一次review巨大的CL。這種事是可能發生的，並且對於reviewer很是有幫助，即使它須要開發人員付出額外人力來處理。

若是CL沒法分解爲較小的CL，且你沒有足夠時間快速查看整個CL內容時，那麼至少對它的總體設計寫些評論，併發送回開發人員以便進行改進。身爲reviewer，你的目標之一是在不犧牲代碼質量的情況下，避免阻檔開發人員進度，或讓他們可以快速採起其餘更進一步的動做。

CR的能力會隨着時間進步

若是你遵循這些準則，而且對於CR很是嚴格的話，後面你會發現整個CR流程會愈來愈快。由於開發者學到什麼是質量好的代碼，並於開頭就提交一個很棒的CL，而這正剛好只須要愈來愈少的時間。而reviewer則學會快速回應，而非在過程當中加入沒必要要的延期。但不要爲提升想象中的速度，而對CR標準和代碼質量作出妥協，畢竟從長遠來看它實際上並不會讓任何事情發生得更快。

緊急情況

在某些緊急狀況下，CL會但願放寬標準以求迅速地經過整個CR過程。但請看什麼是緊急狀況來知道哪些狀況實際上屬於緊急狀況，而哪些不符合。

如何寫review評論

如何面對被推遲處理的評論

有時開發人員會推遲處理CR產生的評論。要麼他們不一樣意你的建議，要麼他們會抱怨你太嚴格了。

誰是對的

當開發人員不一樣意你的建議時，請先花點時間考慮一下他們是不是正確的。由於一般他們比你更瞭解代碼，因此他們可能真的比起你來講對代碼的某些層面具備更好的洞察力。他們的論點有意義嗎?從代碼質量的角度來看它是否合理?若是是的話，讓他們知道他們是對的，而後讓問題沉下去。

但開發人員也並不老是對的。在這種狀況下，reviewer應該更進一步解釋爲何認爲本身的建議是正確的。一個好的解釋一般展現了「對開發人員的回覆的理解」以及有關「爲何被要求對出作出改動」等信息。尤爲是當reviewer認爲給出的建議會改善代碼質量時，便應該繼續宣揚本身的論點。只要他們相信所需的額外的工做量最終會改善總體代碼質量。提升總體代碼質量這件事，每每是經由每一個微小的改動來發生。有時每每須要幾番解釋一個建議才能讓對方真正理解你的用意。切記，始終保持禮貌，讓開發人員知道你有聽到他們在說什麼，只是你不一樣意該論點而已。

讓開發者沮喪

reviewer有時會認爲若本身堅持改進的話，會讓開發人員以爲沮喪不安。的確開發人員有時會感到很沮喪，但這一般是十分短暫的，甚至後來他們很是感謝你幫助他們提升代碼質量。通常來講，只要你在評論中表現得頗有禮貌，開發人員實際上根本不會感到沮喪，這些擔心都僅存在於reviewer心中而已。沮喪不少時候是對於CR評論的寫做方式有關，而並不是來自reviewer對於代碼質量的堅持。

晚點再來整理乾淨

一個常見的推遲緣由是開發人員但願完成任務(這能夠理解)。他們不想經過另外一輪CR來批准這個CL。此時他們一般會說在之後的CL進行整理，因此你如今應該要給LGTM。固然部分開發人員很是擅長這一點，而且當即送出一個修復問題的後續CL (follow-up CL)，但根據過往經驗，這種後續「清理行爲」很是少見。除非開發者在CL得到批准以後馬上進行清理動做，不然這事根本不可能發生。這不是由於開發人員不負責任，而是由於他們可能有不少其餘工做要完成，因而清理工做便會在成堆的工做中被遺忘。所以，一般最好堅持開發人員在代碼在合併後清理它們。由於讓人們「稍後清理」是導致代碼庫質量情況降低最多見的情況。

若是CL引入了新的複雜性的話，除非是緊急狀況，不然必須在提交以前將其處理掉。若是CL致使暴露周圍的問題且如今沒法解決它們的話，開發人員應該將缺陷記錄下來並分配給本身，避免後續被遺忘。又或者他們能夠選擇在代碼中留下TODO的註釋並連接到剛記錄下的缺陷。

關於review嚴格性的常見抱怨

若是你先前以至關寬鬆的標準並轉趨嚴格的進行CR的話，一些開發人員會開始大聲地抱怨。通常來講，提升review的速度會讓這些抱怨逐漸消失。這些抱怨可能須要數個月纔會消失，但最終開發人員會看到嚴格的review所帶來的價值，由於嚴格的review幫助他們產生的優秀代碼。並且一旦發生某些事情時，最大聲的抗議者甚至可能會成爲你最堅決的支持者，由於他們會看到變得review變嚴格後所帶來的價值。