程式碼審查是一個很好的方式，不僅確保了程式碼質量和一致性，也在團隊中或團隊間分享了專案知識。程式碼審查者在審查程式碼時有非常多的東西需要關注，這些方法，能助你在程式碼審查時更加高效。

本文要點

• 程式碼審查者在審查程式碼時有非常多的東西需要關注。一個團隊需要明確對於自己的專案哪些點是重要的，並不斷在審查中就這些點進行檢查。

• 人工審查程式碼是十分昂貴的，因此儘可能地使用自動化方式進行審查，如：程式碼格式、程式碼樣式、檢查常見bug、確定常見安全問題以及執行自動化測試。

• 當針對性能進行審查時，瞭解系統的效能需求是明確潛在問題的關鍵。

• 一些簡單的人工檢查可以顯著提升應用的安全性。

• 程式碼審查是應該在互相溝通中進行討論的，而不是相互對抗。預先確定哪些是要點哪些不是，可以減少衝突並擬定預期。

寫在前面

眾所周知，在團隊中進行程式碼審查（Code Review）可以提升程式碼質量，分享專案知識、明確責任，最終達到構建更好的軟體、更好的團隊。如果你花幾秒鐘搜尋程式碼審查的相關資訊，你會看到許多關於程式碼審查帶來的價值的文章。也有許多方法來進行程式碼審查：在GitHub中提pull request，或使用像JetBrains的Upsource之類的工具。然而即使擁有清晰的流程和正確的工具，還遺留了一個大問題需要解決——我們需要找尋哪些問題。

可能沒有明確關於“我們需要找尋哪些問題”的文章，是因為有許多不同的要點需要考慮。正如任何其他的需求，各個團隊對各個方面都有不同的優先順序。

本文的目標是列出一些審查者可以找尋的要點，而各個方面的優先順序就因各個團隊而異了。

在我們繼續之前，讓我們考慮一下大家在程式碼審查時會討論到的問題。對於程式碼的格式、樣式和命名以及缺少測試這些問題是很常見的幾點。如果你想擁有可持續的、可維護的程式碼，這些是有用的檢查點。然而，在程式碼審查時討論這些就有些浪費時間，因為很多這樣的檢查可以（也應該）被自動化。

那哪些要點是隻能由人工進行審查而不能依靠工具的呢？

回答是有驚人數量的點只能由人工進行審查。在本文剩下的部分，我們會覆蓋一系列廣泛的特性，並深入其中的兩點具體的區域：效能和安全。

關於設計

• 如何讓新程式碼與全域性的架構保持一致？

• 程式碼是否遵循SOLID原則，是否遵循團隊使用的設計規範，如領域驅動開發等？

• 新程式碼使用了什麼設計模式？這樣使用是否合適？

• 基礎程式碼是否有結合使用了一些標準或設計樣式，新的程式碼是否遵循當前的規範？程式碼是否正確遷移，或參照了因不規範而淘汰的舊程式碼？

• 程式碼的位置是否正確？比如涉及訂單的新程式碼是否在訂單服務相關的位置？

• 新程式碼是否重用了現存的程式碼？新程式碼是否可以被現有程式碼重用？新程式碼是否有重複程式碼？如果是的話，是否應該重構成一個更可被重用的模式，還是當前還可以接受？

• 新程式碼是否被過度設計了？是否引入現在還不需要的重用設計？團隊如何平衡可重用和YAGNI(You Ain’t Gonna Need It)這兩種觀點？

可讀性和可維護性

• 欄位、變數、引數、方法、類的命名是否真實反映它們所代表的事物。

• 我是否可以通過讀程式碼理解它做了什麼？

• 我是否理解測試用例測了什麼？

• 測試是否很好地覆蓋了用例的各種情況？它們是否覆蓋了正常和異常用例？是否有忽略的情況？

• 錯誤資訊是否可被理解？

• 不清晰的程式碼是否被文件、註釋或容易理解的測試用例所覆蓋？具體可以根據團隊自身的喜好決定使用哪種方式。

關於功能

• 程式碼是否真的達到了預期的目標？如果有自動化測試來確保程式碼的正確性，測試的程式碼是否真的可以驗證程式碼達到了協定的需求？

• 程式碼看上去是否包含不明顯的bug，比如使用錯誤的變數進行檢查，或誤把and寫成or？

你是否考慮過……？

• 是否需要滿足相關監管需求？

• 作者是否需要建立公共文件或修改現存的幫助文件？

• 是否檢查了面向使用者的資訊的正確性？

• 是否有會在生產環境中導致應用停止執行的明顯錯誤？程式碼是否會錯誤地指向測試資料庫，是否存在應在真實服務中移除的硬編碼的stub程式碼？

• 你對效能的需求是什麼，你是否考慮了安全問題？這些是需要覆蓋到的重大區域也是至關重要的話題，下面讓我們仔細看下這兩點。

關於效能

讓我們深入探討下效能，這是一個真正能從程式碼審查中獲益的方面。

系統對效能方面的非功能性需求應當同所有架構、設計的領域一樣被置於重要位置。無論你是開發只容許納秒級延時的低延遲交易系統，還是管理“待辦事項”的手機應用，你都應該瞭解使用者所認為的“太慢”。

在考慮我們是否需要就程式碼效能進行程式碼審查之前，我們應該問自己幾個關於具體需求是什麼的問題。雖然一些應用確實不需要考慮每毫秒都花費在哪裡，對於大部分應用，花費幾個小時的折騰進行優化來獲得的些許CPU下降的價值也是有限的，但有些地方還是審查者可以檢查一下的，進而確保程式碼不會有一些常見可避免的效能缺陷。

這段程式碼是否有硬性的效能需求？

接受審查的程式碼是否涉及之前釋出的服務等級協議（SLA）？或這個需求本身有特別的效能需求？

如果程式碼導致“登入頁面載入太慢”，那原始的開發者需要找出合適的載入時間是多久，不然審查者或作者本人如何確保改進後的速度足夠快？

如果有硬性的需求，是否有測試能證明滿足了該需求？任何注重效能的系統應該就效能提供自動化測試，這能確保釋出的SLA達到預期（如所有訂單請求要在10毫秒內處理）。沒有這些，你只能依靠你的使用者來告訴你沒有達到對應的SLA。這不僅是一種糟糕的使用者體驗，還會帶來原本可避免的罰金和支出。

這個修復或新增的功能是否會反向影響到任何現存的效能測試結果

如果你定期執行效能測試或有測試套件可以按需執行它們，那你就需要檢查新的程式碼是否使得效能關鍵區域的系統性能有所下降。這可以是一個自動化的流程，但由於在持續整合環境中更常執行單元測試而不是效能測試，所以值得特別指出可以在程式碼審查中檢查這項。

呼叫外部的服務或應用的代價是昂貴的

任何通過網路來使用外部系統的方式通常會比沒有很好優化的方法有更差的效能。考慮以下幾點：

• 呼叫資料庫：最壞的情況是問題隱藏在系統抽象中，如關係物件對映（ORM）中。但是在程式碼審查中你應該可以找到常見的導致效能問題的問題，如在迴圈中逐個呼叫資料庫，一種情況就是載入ID列表之後，再在資料庫中逐個查詢ID對應的每條資料。

• 不必要的網路呼叫：就像資料庫一樣，遠端服務有時也會被過度使用，原來只要一個遠端呼叫就可實現的，或者可以使用批量或快取防止昂貴網路呼叫的，卻使用多個遠端呼叫來實現。再次強調，像資料庫一樣，有時抽象類會隱藏呼叫遠端API的方法。

• 移動或可穿戴應用過於頻繁地呼叫後端程式：這基本上和“不必要的網路呼叫”相同，但是在移動裝置上會產生其他問題，這不僅會產生不必要的呼叫後端使得效能變差，還會更快地消耗電量甚至導致使用者的金錢支出。

有效且高效地使用資源

程式碼是否用鎖來控制共享資源的訪問？這是否會導致效能降低或死鎖？

鎖是一個性能開銷大戶，並在多執行緒環境中很難理清。考慮使用以下模式：單執行緒寫或修改值，其餘執行緒只讀，或使用無鎖演算法。

• 是否存在記憶體洩露？Java中一些常見的原因會是：可變的靜態欄位，使用ThreadLocal變數和使用類載入器。

• 是否存在記憶體無限增長？這個和記憶體洩露不同，記憶體洩漏是指無用的物件不能被垃圾回收器回收。但對於任何語言，就算是沒有垃圾回收的語言，也能建立無限變大的資料結構。作為審查者，如果你看見新的變數不斷被加到list或map中，你就要問下，這個list或map什麼時候失效或清除無用資料。

• 程式碼是否關閉了連線或資料流？關閉連線或檔案、網路資料流很容易會被忘記。當你審查別人程式碼時，如果使用到檔案、網路或資料庫連線，就要確保它們被正確地關閉了。

• 資源池是否配置正確？針對一個環境的最佳配置取決於很多因素，所以作為審查者你很難馬上知道像資料庫連線池大小是否正確等這些問題。但是有一些是你一眼就可以看出來的，像資源池是否太小（比如大小設定為1）或太大（如數百萬執行緒）。如果無法確定，就從預設值開始。沒有使用預設值的就需要提供一定的測試或計算來證明這麼配置的合理性。

審查者可以輕鬆找出的警告訊號

一些程式碼一眼就能看出存在潛在效能問題。這依賴於所使用的語言和類庫。

• 反射：Java的反射比正常呼叫要慢。如果你在審查含有反射的程式碼，你就要問下是否必須使用它。

• 超時：當你審查程式碼時，你可能不知道一個操作合適的超時時間，但是你要想一下“如果超時了，會對系統其他部分造成什麼影響？”。作為審查者你應該考慮最壞的情況：當發生5分鐘的延時，應用是否會阻塞？如果超時時間設定成1秒鐘最壞的情況會是怎麼樣的？如果程式碼作者不能確定超時長度，你作為審查者也不知道一個選定的時間的好壞，那麼是時候找一個理解這其中影響的人蔘與程式碼審查了。

• 並行：程式碼是否使用多執行緒來執行一個簡單的操作？這樣是否花費了更多的時間以及複雜度而並沒有提升效能？如果使用現代化的Java，那其中潛在的問題相較於顯示建立執行緒中的問題更不容易被發現：程式碼是否使用Java 8新的並行流計算但並沒有從並行中獲益？比如，在少量元素上使用並行流計算，或者只是執行非常簡單的操作，這可能比在順序流上運算還要慢。

正確性

這些不一定影響系統的效能，但是它們與多執行緒環境執行關係密切，所以和這個主題有關：

• 程式碼是否使用了正確的適合多執行緒的資料結構。

• 程式碼是否存在競態條件（race conditions）？多執行緒環境中程式碼非常容易造成不明顯的競態條件。作為審查者，可以檢視不是原子操作的get和set。

• 程式碼是否正確使用鎖？和競態條件相關，作為審查者你應該檢查被審程式碼是否允許多個執行緒修改變數導致程式崩潰。程式碼可能需要同步、鎖、原子變數來對程式碼塊進行控制。

• 程式碼的效能測試是否有價值？很容易將小型的效能測試程式碼寫得很糟糕，或者使用不能代表生產環境資料的測試資料，這樣只會得到錯誤的結果。

• 快取：雖然快取是一種能防止過多高消耗請求的方式，但其本身也存在一些挑戰。如果審查的程式碼使用了快取，你應該關注一些常見的問題，如，不正確的快取失效方式。

程式碼級優化

對大部分並不是要構建低延時應用的機構來說，程式碼級優化往往是過早優化，所以首先要知道程式碼級優化是否必要。

• 程式碼是否在不需要的地方使用同步或鎖操作？如果程式碼始終執行在單執行緒中，鎖往往是不必要的。

• 程式碼是否可以使用原子變數替代鎖或同步操作？

• 程式碼是否使用了不必要的執行緒安全的資料結構？比如是否可以使用ArrayList替代Vector？

• 程式碼是否在通用的操作中使用了低效能的資料結構？如在經常需要查詢某個特定元素的地方使用連結串列。

• 程式碼是否可以使用懶載入並從中獲得性能提升？

• 條件判斷語句或其他邏輯是否可以將最高效的求值語句放在前面來使其他語句短路？

• 程式碼是否存在許多字串格式化？是否有方法可以使之更高效？

• 日誌語句是否使用了字串格式化？是否先使用條件判斷語句校驗了日誌等級，或使用延遲求值？

簡單的程式碼即高效的程式碼

Java程式碼中有一些簡單的東西可以供審查者尋找，這些會使JVM很好地替你優化你的程式碼：

• 短小的方法和類。

• 簡單的邏輯，即消除巢狀的條件或迴圈語句。

越是人類可讀的程式碼，JIT編譯器越有可能理解你的程式碼並進行優化。這應該在程式碼審查中很容易定位，如果程式碼看上去易理解且整潔，它執行時效率也會越好。

關於安全

你在構建一個安全、穩固的系統所花費的精力，和花在其他特性上的一樣，取決於專案本身，專案執行的地方、它使用的使用者、需要訪問的資料等。我們現在著重看一些你可能在程式碼審查時關注的地方。

儘可能使用自動化

有驚人數量的安全檢查可以被自動化，而不需要人工干預。安全測試不一定要啟動所有系統進行完整的滲透測試，一些問題可以在程式碼級就能被發現。

常見問題如SQL注入或跨站指令碼可以在持續整合環境通過相應工具檢查出。你也能通過OWASP依賴檢測工具自動化檢查你依賴中已知的漏洞。

有時需要“看情況”

對有的校驗你可以簡單回答“是”或“否”，有時你需要一個工具指出潛在的問題，之後再由人工來決定這個是否需要解決。這也正是Upsource真正的閃光點。Upsource顯示程式碼檢查結果，審查者可以利用這些來決定程式碼是否需要改動或還可以接受目前的情況。

理解你用到的依賴

第三方類庫是侵蝕系統安全並引起漏洞的一個途徑。當審查程式碼時至少你要檢查是否引入了新的依賴（如第三方類庫）。如果你還沒有自動化檢查漏洞，你應該檢查新引入的類庫中已知的問題。

你也應該嘗試著最小化每個類庫的版本，當然如果其他依賴有一個額外的間接依賴，這點可能達不到。但最簡單的最小化自己程式碼暴露在他人程式碼的（通過類庫或服務）安全問題中的方法有：

• 儘可能使用原始碼並理解它的可信度。

• 使用你所能得到的質量最高的類庫。

• 追蹤你在何處使用了什麼，當新的漏洞出現，你可以檢視你受影響的程度。

檢查是否新的路徑和服務需要認證

無論你開發web應用、提供web服務或一些其他需要認證的API，當你增加一個新的URI或服務時，你應該確保它不能在沒有認證的情況下被訪問（假設認證是你係統的需求）。你只要簡單地檢查程式碼的開發者寫了合適的測試用例來展示進行了認證。

你應該不只針對使用使用者名稱和密碼的人類使用者來考慮認證。其他系統或自動化流程來訪問你的應用或服務也會需要認證。這可能影響你們系統中對“使用者”的定義。

資料是否需要加密

當你儲存一些資料到磁碟或通過線纜傳輸，你需要了解資料是否應該被加密。顯然密碼永遠不應該是簡單文字，但是有諸多其他情況資料需要加密。如果被審查的程式碼通過線纜來傳送資料或儲存在某地或以其他方式離開你的系統，且你不知道它是否應該被加密，嘗試詢問下你組織中可以回答這個問題的人。

密碼是否被很好地控制？

這裡的密碼包含密碼（如使用者密碼、資料庫密碼或其他系統的密碼）、祕鑰、令牌等等。這些永遠不應該存放在會提交到原始碼控制系統的程式碼或配置檔案中，有其他方式管理這些密碼，例如通過密碼伺服器（secret server）。當審查程式碼時，要確保這些密碼不會悄悄進入你的版本控制系統中。

程式碼的執行是否應該被日誌記錄或監控？是否正確地使用？

日誌和監控需求因各個專案而不同，一些需要合規，一些擁有比別人嚴格的行為、事件日誌規範。如果你有規章規定哪些需要記錄日誌，何時、如何記錄，那麼作為程式碼審查者你應該檢查提交的程式碼是否滿足要求。如果你沒有固定的規章，那麼就考慮：

• 程式碼是否改變了資料（如增刪改操作）？是否應該記錄由誰何時改變了什麼？

• 程式碼是否涉及關鍵效能的部分？是否應該在效能監控系統中記錄開始時間和結束時間？

• 每條日誌的日誌等級是否恰當？一個好的經驗法則是“ERROR”觸發一個提示傳送到某處，如果你不想這些訊息在凌晨3點叫醒誰，那麼就將之降級為“INFO”或“DEBUG”。當在迴圈中或一條資料可能產生多條輸出的情況下，一般不需要將它們記錄到生產日誌檔案中，它們更應該被放在“DEBUG”級別。

記得叫上專家

安全是個很大的話題，大到足以讓你的公司聘請技術安全專家。我們有安全專家就可以獲得他們的幫助，如，邀請他們參加程式碼審查，或邀請他們在審查程式碼時和我們結對。如果這個無法實現，我們可以充分學習我們系統的環境，來理解我們有哪種安全需求（面向內部的企業級應用和麵向客戶的網頁應用有不同的標準），所以我們可以更好地理解我們應該在程式碼審查中看什麼。

寫在最後

程式碼審查是一個很好的方式，不僅確保了程式碼質量和一致性，也在團隊中或團隊間分享了專案知識。即使你已經自動化了基礎的校驗，還有許多不同程式碼、設計的方面需要考慮。程式碼審查工具，如Upsource，通過在每個程式碼提交的檢查中高亮可疑的程式碼並分析哪些問題已經被修復，新引入哪些問題，可以幫你定位一些潛在的問題。工具也可以簡化流程，因為它提供了一個平臺來討論設計和程式碼實現，也可以邀請審查者、作者和其他相關人員參加討論直到達成共識。

最後，團隊需要花時間決定程式碼質量的哪些因素對他們是重要的，也需要專家人工決定哪些規則應用到各個程式碼審查中，參與到審查中的每個人都應該具備並使用人際交往的技巧，如積極的反饋、談判妥協以達到最終的共識，即程式碼應該怎麼樣才“足夠好”可以通過審查。

原文地址 :  https://www.infoq.com/articles/effective-code-reviews