代碼評審的主要目的是確保 Google 代碼庫的總體代碼運行情況隨着時間的推移而不斷改善。代碼評審的全部工具和過程都是爲此設計的。html
爲了實現這一點,必須作出一系列的權衡。git
首先,開發人員必須可以在其任務上取得進展。若是開發人員從未向代碼庫提交過改進,那麼代碼庫將永遠不會獲得改善。另外,若是評審人員不進行任何更改,那麼以後開發人員也沒有動力進行改進。github
另外一方面,評審人員有責任確保每一個 CL(變動列表)的質量,使得其代碼庫的總體代碼運行情況不會隨着時間的流逝而減小。這可能很棘手,由於隨着時間的推移,代碼庫一般會因爲代碼運行情況的小幅降低而退化,尤爲是在開發團隊時間緊迫,認爲必須採起捷徑才能實現其目標的狀況下。正則表達式
此外,評審人員對他們正在評審的代碼要負起責任。確保代碼庫保持一致性和可維護性,以及「代碼評審中的查找內容」中提到的其餘事項 。算法
所以,咱們將如下規則做爲代碼評審中指望的標準:編程
一般,即便 CL 並不完美,不過若是達到能夠提高系統總體代碼質量的程度,評審人員就能夠批准它。安全
這是全部代碼評審指南中的最高原則。數據結構
固然,是有例外的。例如,若是 CL 添加了評審人員不但願在其系統中使用的功能,那麼即便代碼設計得當,評審人員也能夠確定的拒絕批准。併發
這裏的關鍵點是,沒有「完美」的代碼,只有更好的代碼。評審人員不該要求開發人員在批准前拋光 CL 的每一小塊細節。評審人員要追求的不該該是完美,而應該是持續改進。整體而言,若是一個 CL 能夠提升系統的可維護性,可讀性和可理解性,那就不該該由於它不是「完美的」而被延遲幾天甚至幾周。框架
評審人員應該隨時提供建議,這樣開發人員可能會作得更好,可是若是不是很重要的建議,能夠在其前面加上「 Nit:」之類的字眼,以使開發人員知道這只是一個改進建議,他們能夠選擇忽略。
代碼評審具備重要的功能,能夠傳授開發人員關於語言,框架或通用軟件設計原理的新知識。隨着時間的推移共享知識會成爲改善系統代碼運行質量的一部分。但要注意,若是你的建議純粹是帶有教育性質的,而且對於知足本文所描述的標準來講並非那麼重要,那麼請在前面加上「Nit:」,或者以其餘方式告訴開發人員,他們並不必定要在 CL 中解決這些問題。
在代碼評審中發生衝突時,第一步應始終是使開發人員和評審人員根據本檔以及《 CL 做者指南》 和《審閱者指南》中其餘文檔的內容達成共識。
當達成共識變得特別困難時,讓開發人員和評審人員進行面對面的會議或 VC 可能會有所幫助,而不只僅是嘗試經過代碼評審註釋來解決衝突。(可是,若是這樣作,請確保將討論的結果記錄在 CL 上的註釋中,以備未來閱讀。)
若是還不能解決問題,那麼就要考慮把問題升級。升級的途徑一般是進行更普遍的團隊討論,其中包括讓團隊負責人蔘與進來,請求代碼維護人員做出決定,或請求工程經理提供幫助。不要由於開發人員和評審人員沒法達成一致意見就讓 CL 一直掛在那裏。
代碼評審中最重要的部分是 CL 的整體設計。CL 中各類代碼的交互是否有意義?此更改是屬於代碼庫仍是屬於某個包?它與系統的其他部分是否集成良好?如今是添加此功能的好時機嗎?
此 CL 是否達到開發人員的預期目的?開發人員打算爲該代碼的用戶帶來什麼好處?「用戶」一般既是最終用戶(當他們受到更改影響時)又是開發人員(未來他們將不得不「使用」此代碼)。
一般,咱們但願開發人員可以對 CL 進行良好的測試,以確保它們在進行代碼審查時可以正常工做。可是,做爲評審人員,仍然應該考慮邊緣狀況,尋找併發性問題,嘗試像用戶同樣思考,並找出僅僅經過閱讀代碼不能看到的錯誤。
您能夠根據須要驗證 CL,對於評審人員來講,檢查 CL 的行爲最好的時機是當它對用戶產生了影響,例如 UI 更改。當只是閱讀代碼的時候,很難理解一些更改將如何影響用戶。對於這樣的更改,若是過於麻煩而沒法在 CL 中打補丁並本身嘗試,則可讓開發人員提供功能演示。
在代碼評審期間考慮功能性的另外一個時機點是,CL 中是否正在進行某種並行編程,從理論上講可能致使死鎖或競爭條件。僅經過運行代碼很難檢測到這類問題,一般須要有人(開發人員和評審人員)仔細考慮它們,以確保不會引入問題。
CL 是否比實際須要的要複雜?在 CL 的每一個級別都進行檢查 —— 個別行是否太複雜?功能太複雜?類太複雜?「過於複雜」一般意味着「代碼閱讀器沒法快速理解。」也可能意味着「開發人員在嘗試調用或修改此代碼時可能會引入錯誤。」
過分設計一種特殊的複雜性,即開發人員使代碼變得比實際須要的通用,或者增長了系統目前不須要的功能。評審人員應特別警戒過分設計。鼓勵開發人員解決他們如今須要解決的已知問題,而不是解決開發人員推測的未來可能須要解決的問題。將來的問題應該在它們出現以後再去解決,由於到了那個時候咱們能夠看到它們的實際情況和需求。
根據更改要求進行單元測試,集成測試或端到端測試。一般,除非 CL 處理緊急狀況,不然應在與生產代碼相同的 CL 中添加測試 。
確保 CL 中的測試正確,合理且有用。測試沒法自我測試,咱們不多爲測試編寫測試,因此必須確保測試有效。
代碼破損時,測試會失敗嗎?若是代碼在其下面更改,它們將開始產生誤報嗎?每一個測試都會作出簡單而有用的斷言嗎?測試是否在不一樣的測試方法之間適當地分開?
請記住,測試也是必須維護的代碼。
開發人員是否使用了良好的命名方式?好的命名要可以充分表達一個項(變量、類名等)是什麼或者用來作什麼,但又不會由於太長而難以閱讀。
開發人員是否用可理解的天然語言寫下清晰的註釋?全部註釋都是必要的嗎?一般,好的註釋應該解釋爲何存在某些代碼,而不該該解釋某些代碼在作什麼。若是代碼不夠清晰,沒法解釋自身,則應使簡化代碼。也有一些例外狀況(例如,正則表達式和複雜算法一般會在註釋中解釋它們的做用,會讓閱讀代碼的人受益不淺),但大多數註釋是針對代碼自己可能沒法包含的信息,好比這些代碼背後的原因。
查看此 CL 以前的註釋也會有所幫助。也許有一個待辦事項如今能夠刪除,有意見建議不要進行此更改,等等。
請注意,註釋與類,模塊或函數的文檔不一樣,註釋應表示一段代碼的用途,應如何使用以及使用時的行爲。
谷歌爲主要編程語言和大多數次要編程語言提供了代碼風格指南,確保 CL 遵循適當的風格指南。
若是您想改善指南中沒有的樣式點,請在註釋前面加上「 Nit:」,以使開發人員知道這是您認爲能夠改善代碼但不是強制性的選擇。但請不要只是基於我的偏好來阻止 CL 的提交。
開發人員不該將主要風格更改與其餘更改結合在一塊兒。這使得很難看到 CL 中的更改,使合併和回滾更加複雜,並致使其餘問題。例如,若是開發人員想要從新格式化整個文件,請他們僅將從新格式化的格式做爲一個 CL 發送給評審人員,而後再發送另外一個具備功能更改的 CL。
若是 CL 改變了用戶構建,測試,與代碼交互或釋放代碼的方式,請檢查其是否還更新了相關文檔,包括 README,g3doc 頁面和其餘生成的參考文檔。若是 CL 刪除或棄用了代碼,請考慮是否還應刪除該文檔。若是文檔缺失,要向開發人員索要。
查看每一行代碼。有時能夠掃描諸如數據文件,生成的代碼或大型數據結構之類的東西,但不要掃描人工編寫的類,函數或代碼塊,並假設其中的內容是能夠正常運行的。顯然,某些代碼比其餘代碼更須要仔細檢查 —— 至因而哪些代碼徹底取決於你,但你至少應該要理解這些代碼都在作些什麼。
若是代碼很複雜或者你難以快速看懂它們,這會使評審速度變慢,那麼你應該讓開發人員知道這一點,並等待他們解釋,而後再嘗試評審。Google 聘請了出色的軟件工程師,若是他們看不懂代碼,其餘開發人員也極可能看不懂。所以,要求開發人員進行解釋,也能夠幫助未來的開發人員理解此代碼。
若是您理解代碼,但又沒有資格進行代碼評審,請確保在 CL 上有一位合格的評審人員,特別是在複雜性問題(例如安全性,併發性,可訪問性,國際化等)上。
一般,代碼查看工具只會顯示須要更改部分的幾行代碼。可是有時必須查看整個文件,以確保更改確實有意義。例如,你可能會看到僅添加了四行代碼,可是當你查看整個文件時,您會看到這四行位於 50 行方法中,這個時候須要將其分解爲較小的方法。
須要基於整個系統來考量 CL 。此 CL 是在改善系統的代碼運行情況,仍是使整個系統更加複雜,更不可測等?不要接受會下降系統代碼運行情況的 CL。大多數系統會經過許多小的更改而變得複雜,所以,防止新更改中出現很小的複雜性是很重要的。
若是在 CL 中看到不錯的東西,請告訴開發人員,尤爲是當他們以出色的方式回答了你的評論時。代碼評審一般只關注錯誤,可是也應該鼓勵和讚揚良好的實踐。有時候告訴開發人員正確的作法比告訴他們錯誤的作法更有價值。
在進行代碼評審時,你應確保:
確保檢查的每一行代碼,查看上下文,確保改善代碼運行情況,並稱贊開發人員所作的出色工做。
在知道了代碼評審要關注哪些東西以後,如何有效地進行跨文件代碼評審呢?
查看 CL 說明以及 CL 的通常功能。這種變動有意義嗎?若是這個變動是沒必要要的,請當即作出答覆,並說明爲何不該該進行更改。當您拒絕這樣的更改時,最好仍是向開發人員建議他們應該作些什麼。
例如,您可能會說:「看起來您爲此作了一些出色的工做,謝謝!可是,實際上咱們正打算移除這個系統,所以咱們如今不但願對其進行任何新的修改。或許,您能夠重構咱們的新 BarWidget 類嗎?」
請注意,評審人員在拒絕當前的 CL 時要一併提出其餘建議,並且還要表現地禮貌。這種禮貌很重要,由於做爲開發人員,即便咱們不一樣意,也要代表咱們彼此尊重。
若是出現多個 CL 是你不想進行的更改,則應考慮從新設計團隊的開發流程或外部貢獻者的已發佈流程,以便在編寫 CL 以前進行更多的溝通。最好在人們完成大量如今必須扔掉或完全重寫的工做以前告訴他們「不」。
查找屬於此 CL 的「主要」部分的文件。一般,邏輯更改數量最多的文件是 CL 的主要部分。首先看這些主要部分。這有助於爲 CL 的全部較小部分提供上下文,並一般加快執行代碼評審的速度。若是 CL 太大,您沒法肯定哪些部分是主要的,請詢問開發人員您應該首先看什麼,或要求他們將 CL 分紅多個 CL。
若是您發現 CL 的這一部分存在一些主要的設計問題,要當即回覆開發人員,即便您如今沒有時間評審 CL 的其他部分。實際上,複查 CL 的其他部分可能會浪費時間,由於若是設計問題足夠嚴重,那麼許多其餘代碼都會變得可有可無。
爲何要當即回覆開發人員有兩個主要緣由:
確認 CL 總體上沒有大的設計問題後,請嘗試找出邏輯順序來瀏覽文件,同時還要確保不要錯過對任何文件的評審。一般,在瀏覽了主要文件以後,按照代碼評審工具向您展現它們的順序瀏覽每一個文件是最簡單的。有時在閱讀主要代碼以前先閱讀測試也是有幫助的,由於這樣您就能夠知道更改應該作什麼。
在谷歌,咱們對開發團隊的總體交付速度(而不是針對個體開發人員寫代碼的速度)進行了優化。個體開發速度也很重要,但其重要性比不上整個團隊的開發速度。
當代碼評審速度很慢時,會發生如下幾件事:
若是你不是在集中精力完成手頭的任務,那就應該在第一時間評審代碼。
一個工做日是響應代碼檢查請求所需的最長時間(即次日早上的第一件事)。
遵循這些準則意味着典型的 CL 應在一天以內進行多輪評審(若是須要)。
有一種狀況,我的速度的考慮賽過團隊速度。若是您正在處理諸如編寫代碼之類的重點任務,請不要打斷本身去進行代碼檢查。研究代表,開發人員在中斷以後要花很長時間才能恢復到平穩的開發流程。所以,在編寫代碼時打斷本身,實際上對團隊來講,要比讓其餘開發人員稍等一下子進行代碼評審更爲昂貴。
因此,對於這種狀況,能夠等到你手頭工做能夠停了再開始代碼評審。能夠是在完成手頭的編碼任務以後,午餐後,會議結束後,休息結束後等。
咱們所說代碼評審速度指的是響應時間,而不是 CL 經過整個評審流程並提交所花的時間。整個過程在理想狀況下也應該是很快的,但單次評審請求的響應速度比整個過程的響應速度更重要。
即便有時須要很長時間才能完成整個評審過程,在整個評審過程當中得到評審人員的快速響應也能夠極大地緩解開發人員對「緩慢」代碼評審的不滿。
若是您忙於在 CL 上進行全面評審時,仍然能夠先發送一個快速響應,以使開發人員知道何時能夠開始評審,或者建議讓其餘能夠更快作出響應的評審人員來評審代碼,或者提供一些初步反饋。
重要的是,評審人員應花費足夠的時間進行評審,以確保此代碼符合標準。但無論怎樣,最好響應速度仍是要快一些。
在處理跨時區代碼評審時,請在他們仍在辦公室時嘗試與開發人員聯繫。若是他們已經回家了,那麼最好能夠確保他們在次日回到辦公室時能夠看到代碼評審已經完成。
爲了加速代碼評審速度,在某些狀況下,即便審覈人員也將未解決的評論留在了 CL 上,評審人員仍應該給予 LGTM/批准。在如下狀況之一時完成此操做:
除非另有明確說明,不然評審人員應使用這些選項中的一個。
當開發人員和評審人員處於不一樣時區時,帶有註釋的 LGTM 特別值得考慮,不然開發人員將等待一成天才得到「 LGTM,批准」。
若是有人向您發送了一個很大的代碼評審,您不肯定什麼時候能夠有時間對其進行評審,一般的響應應該是要求開發人員將 CL 拆分爲多個相互構建的較小的 CL。 而沒必要一次審查全部巨大的 CL。這樣一般是合理的,而且對評審人員頗有幫助,即便須要開發人員作些額外的工做。
若是不能將一個 CL 分解爲較小的 CL,而且您沒有時間快速評審整個 CL,那麼至少要對 CL 的整體設計寫一些註釋,而後將其發回給開發人員進行改進。做爲評審人員,您的目標之一應該是是在不影響代碼質量的狀況下快速對開發人員作出響應,或者讓他們可以快速採起進一步行動。
若是您遵循了這些準則,而且嚴格執行代碼評審,則應該發現整個代碼評審過程會隨着時間的流逝而愈來愈快。開發人員知道爲了保證代碼質量須要作些什麼,並從一開始就向你發送很是棒的 CL,這樣評審所需的時間就會愈來愈少。評審人員學會如何快速作出響應,而且不會在評審過程當中增長沒必要要的延遲。可是,不要在代碼評審標準或質量上作出讓步以提升速度 —— 從長遠來看,這不會使任何事情更快地發生。
在某些緊急狀況下,CL必須很是快速地經過 整個審覈過程,而且質量準則將獲得放寬。可是,請參閱什麼是緊急狀況?描述哪些狀況實際上能夠視爲緊急狀況,哪些不能夠。
一般,重要的是要禮貌和尊重,同時也要對要查看其代碼的開發人員很是瞭解。一種方法是確保您的註釋是針對代碼,而是對開發人員。肅然沒必要老是遵循這種作法,可是在說出可能會使人沮喪或引發爭議的內容時,必定要使用它。例如:
很差的說法:「爲何要在這個地方使用線程,這樣作顯然不會得到任何好處」
好的說法:「併發模型在這裏增長了系統的複雜性,而我沒有看到任何實際的性能優點。因爲沒有任何性能上的好處,所以最好將此代碼爲單線程而不是使用多個線程。」
從上面的正確示例中能夠看出,這樣有助於開發人員理解爲何要給出這些建議。您不必定老是須要在評論註釋中包含此信息,可是有時適當的作法能夠成爲對您的意圖的理解,所遵循的最佳實踐或您的建議如何改善代碼質量進行更多說明。
一般,修復 CL 是開發人員的責任,而不是評審人員的責任。您無需執行解決方案的詳細設計或爲開發人員編寫代碼。
可是,這並不意味着評審人員不該該給予幫助。一般,您應該在指出問題和提供直接指導之間取得適當的平衡。指出問題並讓開發人員作出決定一般能夠幫助開發人員學習,並使代碼評審更容易。這也能夠帶來更好的解決方案,由於開發人員比評審人員更接近代碼。
可是,有時直接給出指令指令,建議甚至代碼會更有幫助。代碼評審的主要目的是得到最佳的 CL。第二個目的是提升開發人員的技能,以便他們以後的評審會愈來愈少。
若是您要求開發人員解釋一段您不理解的代碼,他們一般會去重寫代碼。有時,在代碼中添加註釋也是一種適當的響應,只要它不僅是解釋過於複雜的代碼便可。
僅在代碼檢查工具中編寫的說明對未來的代碼閱讀者沒有幫助。只有少數狀況能夠接受這種作法,例如,你對評審的東西不太熟悉,而開發人員的解釋倒是不少人所熟知的。
有時,開發人員會回推代碼評審。他們可能會不一樣意您的建議,或者會抱怨您整體上過於嚴格。
當開發人員不一樣意您的建議時,請先花點時間考慮一下它們是否正確。一般,它們比您更接近代碼,所以他們實際上可能對代碼的某些方面有更好的瞭解。他們的論點有意義嗎?從代碼質量的角度來看,這有意義嗎?若是是這樣,請讓他們知道他們是對的,而後問題就解決了。
可是,開發人員並不老是正確的。在這種狀況下,評審人員應進一步解釋爲何他們認爲本身的建議正確。良好的解釋不只說明了對開發人員的理解,並且還說明了爲什麼要求他們進行更改其餘信息。
若是評審人員認爲他們的建議能夠改善代碼質量,並認爲評審所帶來的代碼質量改進值得開發人員作出額外的工做,那麼他們就應該堅持。改善代碼質量每每是由一系列的小步驟組成的。
有時,在真正提出建議以前,須要花不少時間去解釋。請確保始終保持禮貌,並讓開發人員知道您瞭解他們在說什麼,您只是不一樣意。
評審人員有時認爲若是他們堅持要開發人員作出改動,會使開發人員感到沮喪。有時,開發人員確實會感到不高興,但這一般是短暫的,以後他們會很是感謝您幫助他們提升了代碼質量。若是您表現的頗有禮貌,那麼開發人員根本不會感到不開心,這種擔憂多是多餘的。煩惱一般與評審人員的註釋編寫方式有關 ,而不是與評審人員對代碼質量的堅持。
回退的一個常見緣由是開發人員想要完成任務(能夠理解)。他們不想僅僅爲了得到此 CL 而進行另外一輪評審。所以,他們說他們將在之後的 CL 中清理某些內容,所以評審人員如今應該 LGTM 此 CL。一些開發人員對此很是好,並會當即編寫後續的 CL 來解決此問題。可是,經驗代表,開發人員編寫原始 CL 的時間越長,他們進行後續修復的可能性就越小。實際上,除非開發人員在提交當前的 CL 以後馬上修復,不然在經過以後一般不會再去作這件事情。這不是由於開發人員不負責任,而是由於他們有不少工做要作,因此修復工做一般會被遺忘。所以,最好是堅持要求開發人員在代碼進入代碼庫並「完成」以前當即修復其 CL 。讓開發人員「稍後解決」是代碼庫退化的一種常見方法。
若是 CL 引入了新的複雜性,除非是緊急狀況,不然必須在提交以前將其清除。若是 CL 暴露了一些問題,而且如今沒法解決,那麼開發人員應把錯誤記錄下來,分配給本身,以避免丟失。他們還能夠選擇在引用已提交錯誤的代碼中編寫 TODO 註釋。
若是您之前對代碼的評審不是那麼嚴格,可是卻忽然變得嚴格起來,那麼一些開發人員將會大聲抱怨。不過不要緊,提升代碼評審的速度一般會使這些抱怨消失。
有時,這些抱怨可能須要通過數月的時間纔會消失,可是最後開發人員會看到嚴格的代碼評審的價值,由於他們會看到嚴格的代碼評審幫助本身產出優秀代碼。有時候,若是發生這種事情,聲音最強烈的抗議者甚至會成爲您最堅強的支持者。
若是您遵循上述全部內容,可是在代碼評審過程當中仍然遇到沒法解決的衝突,請再次參閱以上內容以獲取有助於解決衝突的指導準則。