成天說Code Review重要，你知道應該關注哪些關鍵點嗎？

衆所周知，在團隊中進行代碼審查（Code Review）能夠提高代碼質量，分享項目知識、明確責任，最終達到構建更好的軟件、更好的團隊。若是你花幾秒鐘搜索代碼審查的相關信息，你會看到許多關於代碼審查帶來的價值的文章。也有許多方法來進行代碼審查：在GitHub中提pull request，或使用像JetBrains的Upsource之類的工具。然而即便擁有清晰的流程和正確的工具，還遺留了一個大問題須要解決——咱們須要找尋哪些問題。

可能沒有明確關於「咱們須要找尋哪些問題」的文章，是由於有許多不一樣的要點須要考慮。正如任何其餘的需求，各個團隊對各個方面都有不一樣的優先級。

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

在咱們繼續以前，讓咱們考慮一下你們在代碼審查時會討論到的問題。對於代碼的格式、樣式和命名以及缺乏測試這些問題是很常見的幾點。若是你想擁有可持續的、可維護的代碼，這些是有用的檢查點。然而，在代碼審查時討論這些就有些浪費時間，由於不少這樣的檢查能夠（也應該）被自動化。

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

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

設計

 

• 如何讓新代碼與全局的架構保持一致？

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

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

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

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

• 新代碼是否重用了現存的代碼？新代碼是否能夠被現有代碼重用？新代碼是否有重複代碼？若是是的話，是否應該重構成一個更可被重用的模式，仍是當前還能夠接受？

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

可讀性和可維護性

 

• 字段、變量、參數、方法、類的命名是否真實反映它們所表明的事物。

• 我是否能夠經過讀代碼理解它作了什麼？

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

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

• 錯誤信息是否可被理解？

• 不清晰的代碼是否被文檔、註釋或容易理解的測試用例所覆蓋？具體能夠根據團隊自身的喜愛決定使用哪一種方式。

功能

 

• 代碼是否真的達到了預期的目標？若是有自動化測試來確保代碼的正確性，測試的代碼是否真的能夠驗證代碼達到了協定的需求？

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

你是否考慮過……？

 

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

• 做者是否須要建立公共文檔或修改現存的幫助文檔？

• 是否檢查了面向用戶的信息的正確性？

• 是否有會在生產環境中致使應用中止運行的明顯錯誤？代碼是否會錯誤地指向測試數據庫，是否存在應在真實服務中移除的硬編碼的stub代碼？

• 你對性能的需求是什麼，你是否考慮了安全問題？這些是須要覆蓋到的重大區域也是相當重要的話題。

讓咱們深刻探討下性能，這是一個真正能從代碼審查中獲益的方面。

系統對性能方面的非功能性需求應當同全部架構、設計的領域同樣被置於重要位置。不管你是開發只允許納秒級延時的低延遲交易系統，仍是管理「待辦事項」的手機應用，你都應該瞭解用戶所認爲的「太慢」。

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

這段代碼是否有硬性的性能需求？

接受審查的代碼是否涉及以前發佈的服務等級協議（SLA）？或這個需求自己有特別的性能需求？

若是代碼致使「登陸頁面加載太慢」，那原始的開發者須要找出合適的加載時間是多久，否則審查者或做者本人如何確保改進後的速度足夠快？

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

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

若是你按期運行性能測試或有測試套件能夠按需運行它們，那你就須要檢查新的代碼是否使得性能關鍵區域的系統性能有所降低。這能夠是一個自動化的流程，但因爲在持續集成環境中更常運行單元測試而不是性能測試，因此值得特別指出能夠在代碼審查中檢查這項。

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

任何經過網路來使用外部系統的方式一般會比沒有很好優化的方法有更差的性能。考慮如下幾點：

1. 調用數據庫：最壞的狀況是問題隱藏在系統抽象中，如關係對象映射（ORM）中。可是在代碼審查中你應該能夠找到常見的致使性能問題的問題，如在循環中逐個調用數據庫，一種狀況就是加載ID列表以後，再在數據庫中逐個查詢ID對應的每條數據。 

2. 沒必要要的網絡調用：就像數據庫同樣，遠程服務有時也會被過分使用，原來只要一個遠程調用就可實現的，或者可使用批量或緩存防止昂貴網絡調用的，卻使用多個遠程調用來實現。再次強調，像數據庫同樣，有時抽象類會隱藏調用遠程API的方法。 

3. 移動或可穿戴應用過於頻繁地調用後端程序：這基本上和“沒必要要的網絡調用”相同，可是在移動設備上會產生其餘問題，這不只會產生沒必要要的調用後端使得性能變差，還會更快地消耗電量甚至致使用戶的金錢支出。

有效且高效地使用資源

代碼是否用鎖來控制共享資源的訪問？這是否會致使性能下降或死鎖？

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

1. 是否存在內存泄露？Java中一些常見的緣由會是：可變的靜態字段，使用ThreadLocal變量和使用類加載器。 

2. 是否存在內存無限增加？這個和內存泄露不一樣，內存泄漏是指無用的對象不能被垃圾回收器回收。但對於任何語言，就算是沒有垃圾回收的語言，也能建立無限變大的數據結構。做爲審查者，若是你看見新的變量不斷被加到list或map中，你就要問下，這個list或map何時失效或清除無用數據。

3. 代碼是否關閉了鏈接或數據流？關閉鏈接或文件、網絡數據流很容易會被忘記。當你審查別人代碼時，若是使用到文件、網絡或數據庫鏈接，就要確保它們被正確地關閉了。

4. 資源池是否配置正確？針對一個環境的最佳配置取決於不少因素，因此做爲審查者你很難立刻知道像數據庫鏈接池大小是否正確等這些問題。可是有一些是你一眼就能夠看出來的，像資源池是否過小（好比大小設置爲1）或太大（如數百萬線程）。若是沒法肯定，就從默認值開始。沒有使用默認值的就須要提供必定的測試或計算來證實這麼配置的合理性。

審查者能夠輕鬆找出的警告信號

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

 

• 反射：Java的反射比正常調用要慢。若是你在審查含有反射的代碼，你就要問下是否必須使用它。

• 超時：當你審查代碼時，你可能不知道一個操做合適的超時時間，可是你要想一下「若是超時了，會對系統其餘部分形成什麼影響？」。做爲審查者你應該考慮最壞的狀況：當發生5分鐘的延時，應用是否會阻塞？若是超時時間設置成1秒鐘最壞的狀況會是怎麼樣的？若是代碼做者不能肯定超時長度，你做爲審查者也不知道一個選定的時間的好壞，那麼是時候找一個理解這其中影響的人蔘與代碼審查了。

• 並行：代碼是否使用多線程來運行一個簡單的操做？這樣是否花費了更多的時間以及複雜度而並無提高性能？若是使用現代化的Java，那其中潛在的問題相較於顯示建立線程中的問題更不容易被發現：代碼是否使用Java 8新的並行流計算但並無從並行中獲益？好比，在少許元素上使用並行流計算，或者只是運行很是簡單的操做，這可能比在順序流上運算還要慢。

正確性

這些不必定影響系統的性能，可是它們與多線程環境運行關係密切，因此和這個主題有關：

• 代碼是否使用了正確的適合多線程的數據結構。

• 代碼是否存在競態條件（race conditions）？多線程環境中代碼很是容易形成不明顯的競態條件。做爲審查者，能夠查看不是原子操做的get和set。

• 代碼是否正確使用鎖？和競態條件相關，做爲審查者你應該檢查被審代碼是否容許多個線程修改變量致使程序崩潰。代碼可能須要同步、鎖、原子變量來對代碼塊進行控制。

• 代碼的性能測試是否有價值？很容易將小型的性能測試代碼寫得很糟糕，或者使用不能表明生產環境數據的測試數據，這樣只會獲得錯誤的結果。

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

代碼級優化

對大部分並非要構建低延時應用的機構來講，代碼級優化每每是過早優化，因此首先要知道代碼級優化是否必要。

• 代碼是否在不須要的地方使用同步或鎖操做？若是代碼始終運行在單線程中，鎖每每是沒必要要的。

• 代碼是否可使用原子變量替代鎖或同步操做？

• 代碼是否使用了沒必要要的線程安全的數據結構？好比是否可使用ArrayList替代Vector？

• 代碼是否在通用的操做中使用了低性能的數據結構？如在常常須要查找某個特定元素的地方使用鏈表。

• 代碼是否可使用懶加載並從中得到性能提高？

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

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

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

簡單的代碼即高效的代碼

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

• 短小的方法和類。

• 簡單的邏輯，即消除嵌套的條件或循環語句。

安全

你在構建一個安全、穩固的系統所花費的精力，和花在其餘特性上的同樣，取決於項目自己，項目運行的地方、它使用的用戶、須要訪問的數據等。咱們如今着重看一些你可能在代碼審查時關注的地方。

儘量使用自動化

有驚人數量的安全檢查能夠被自動化，而不須要人工干預。安全測試不必定要啓動全部系統進行完整的滲透測試，一些問題能夠在代碼級就能被發現。

常見問題如SQL注入或跨站腳本能夠在持續集成環境經過相應工具檢查出。你也能經過OWASP依賴檢測工具自動化檢查你依賴中已知的漏洞。

有時須要「看狀況」

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

理解你用到的依賴

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

你也應該嘗試着最小化每一個類庫的版本，固然若是其餘依賴有一個額外的間接依賴，這點可能達不到。但最簡單的最小化本身代碼暴露在他人代碼的（經過類庫或服務）安全問題中的方法有：

• 儘量使用源碼並理解它的可信度。

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

• 追蹤你在何處使用了什麼，當新的漏洞出現，你能夠查看你受影響的程度。

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

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

你應該不僅針對使用用戶名和密碼的人類用戶來考慮認證。其餘系統或自動化流程來訪問你的應用或服務也會須要認證。這可能影響大家系統中對「用戶」的定義。

數據是否須要加密

當你保存一些數據到磁盤或經過線纜傳輸，你須要瞭解數據是否應該被加密。顯然密碼永遠不該該是簡單文本，可是有諸多其餘狀況數據須要加密。若是被審查的代碼經過線纜來傳送數據或保存在某地或以其餘方式離開你的系統，且你不知道它是否應該被加密，嘗試詢問下你組織中能夠回答這個問題的人。

密碼是否被很好地控制？

這裏的密碼包含密碼（如用戶密碼、數據庫密碼或其餘系統的密碼）、祕鑰、令牌等等。這些永遠不該該存放在會提交到源碼控制系統的代碼或配置文件中，有其餘方式管理這些密碼，例如經過密碼服務器（secret server）。當審查代碼時，要確保這些密碼不會悄悄進入你的版本控制系統中。

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

日誌和監控需求因各個項目而不一樣，一些須要合規，一些擁有比別人嚴格的行爲、事件日誌規範。若是你有規章規定哪些須要記錄日誌，什麼時候、如何記錄，那麼做爲代碼審查者你應該檢查提交的代碼是否知足要求。若是你沒有固定的規章，那麼就考慮：

• 代碼是否改變了數據（如增刪改操做）？是否應該記錄由誰什麼時候改變了什麼？

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

• 每條日誌的日誌等級是否恰當？一個好的經驗法則是「ERROR」觸發一個提示發送到某處，若是你不想這些消息在凌晨3點叫醒誰，那麼就將之降級爲「INFO」或「DEBUG」。當在循環中或一條數據可能產生多條輸出的狀況下，通常不須要將它們記錄到生產日誌文件中，它們更應該被放在「DEBUG」級別。

記得叫上專家

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

總結

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

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

關於做者

Trisha Gee爲一系列行業開發Java應用程序，包括金融、製造業、軟件和非盈利組織，包括各類規模的公司。她在開發Java高性能系統上有豐富經驗，並積極幫助開發者使他們擁有高生產率，她也涉足開源開發。Trisha是Sevilla Java用戶組和Java Champion的帶頭人，她信任社區並分享本身的觀點，幫助咱們從錯誤中學習，並在成功的基礎上繼續發展。她是JetBrains技術推廣人，也就是說她會不斷分享全部有趣的發現。