關于 Code Review，你「必須」了解的一些關鍵點……

代碼審查的重要性，對碼農來說自是不言而喻，但整天說Code重要，你知道都應該關注哪些關鍵點嗎？

眾所周知，在團隊中進行代碼審查（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，通過在每個代碼提交的檢查中高亮可疑的代碼并分析哪些問題已經被修復，新引入哪些問題，可以幫你定位一些潛在的問題。工具也可以簡化流程，因為它提供了一個平臺來討論設計和代碼實現，也可以邀請審查者、作者和其他相關人員參加討論直到達成共識。

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

 

 

 

來自：http://mp.weixin.qq.com/s?__biz=MjM5MDE0Mjc4MA==&mid=2650994555&idx=1&sn=b196e2dfb293ec7829523011316a7e06&chksm=bdbf0f288ac8863e014eae215469f4bbc0b8e88fa286c3cd38b467348466c663415ffed0e4ca&scene=0#wechat_redirect