代碼審查「查」什麼？

讓咱們來談談代碼審查（Code Review）。若是花幾秒鐘去搜索有關內容，你會發現許多論述代碼審查好處的文章（例如，Jeff Atwood的這篇文章）。你還會發現許多介紹如何使用代碼審查工具的文檔，好比咱們經常使用的Upsource。但可以在你審查他人代碼時指導查什麼的內容卻不多見。

或許沒有明確審查條目的緣由是：有太多不一樣的因素須要考慮。就像對任何（功能性或非功能性）需求，個體組織對各個方面的優先級都有不一樣的考慮。

因爲文章主題覆蓋面廣，本文旨在概述了代碼審查者在代碼審查時能夠關注的一些方面。各方面優先級的分配和持續檢查是一個很是複雜課題，具備研究價值。

審查他人代碼時，查什麼？

不管是使用 Upsource 相似工具審查代碼，仍是在同事查他們代碼期間，無論哪一種狀況，相比較而言評審有些內容要更容易。例如：

格式：在什麼地方放置空格和斷行符？使用製表符仍是空格？大括號如何放置？

風格：變量、參數是否聲明爲final？函數變量的定義是否與調用代碼或函數開始代碼太類似？

命名：域、常量、變量、參數、類的名稱是否符合標準？命名是否過短？

覆蓋測試：這段代碼是否進行了測試？

這些檢查都是有意義的——你但願儘量減小不一樣代碼間的上下文切換並減小認知負荷，那麼代碼看上去越一致越好。（譯者注：認知負荷（Cognitive Load）是指人在完成任務過程當中進行信息加工所須要的認知資源的總量。）

可是，人工審查這些內容可能沒有充分利用團隊的時間和資源，由於其中大部分檢查能夠自動化。許多工具都能確保代碼格式一致，包括命名和 final 關鍵字的使用規範，並且能發現簡單編程錯誤形成的常見bug。例如，你在命令行中運行IntelliJ IDEA的檢查，那麼就不須要全部團隊成員在他們的集成開發環境中進行相同的檢查。

你應該審查什麼？

哪些事情適合代碼審查者作？在代碼審查時，哪些事情是工具不能代勞的？

事實上，代碼審查者能夠作的事情不少。固然，下面給出的不是一份詳盡的列表，這裏也不會深刻探討其中的任何一項。相反，這應該是團隊交流的起點，如今大家在代碼審查中關注什麼，也許正是大家須要審查的內容。

設計

新代碼是否與總體架構匹配？

代碼是否遵循SOLID原則、領域驅動設計以及團隊喜好的其它設計模式。

新代碼採用哪些設計模式？這些模式合適嗎？

若是代碼庫採用混合標準或設計風格，新代碼是否符合當前風格？代碼遷移是按正確的方向進行，仍是效仿即將被淘汰的陳舊代碼？

代碼是否處於正確的位置？例如，若是代碼執行與順序有關，它是否能按順序執行？

新代碼可否複用部分已存在代碼？新代碼可否給已存在代碼提供複用部分？

新代碼是否包含冗餘代碼？若是包含，是應該重構成更加可複用部分，仍是在這個階段能接受這種冗餘？

代碼是否超出設計標準？這種複用構造如今是否是不須要？團隊如何根據YAGNI權衡複用？

可讀性與可維護性

命名（字段、變量、參數、函數以及類）可否反映它們表明的事物？

經過閱讀代碼，我可否理解代碼的目的？

我可否理解測試的目的？

測試是否覆蓋了絕大部分狀況？是否覆蓋常見狀況和異常狀況？是否存在沒考慮到的狀況？

異常錯誤消息是否易於理解？

難以理解的代碼段是否進行了備註、評論或者使用易懂的測試案例進行覆蓋（符合團隊的偏好）？

功能

代碼的實際工做是否符合預期？若是有自動化測試來確保代碼的正確，測試可否測出代碼知足約定要求？

代碼看上去是否含有細微錯誤，好比使用錯誤變量進行檢查，或者把 or 誤用爲 and？

你是否考慮過……？

代碼中是否存在潛在的安全問題？

是否須要知足規範要求？

對於沒有覆蓋自動化性能測試的代碼段，新代碼是否引入了不可避免的性能問題，好比沒必要要的數據調用或遠程服務？

做者是否須要建立公共文檔，或者修改現有的幫助文檔。

展現給用戶的消息是否檢查無誤？

是否存在致使產品崩潰的明顯錯誤？代碼是否會意外指向測試數據庫，或者是否有應該替換成真正服務的硬編碼存根代碼？

請關注本博客中詳細探究這些主題的文章。

如何編寫「良好」代碼是每一個開發者都能各抒己見的主題。若是你有一些能夠加入咱們列表的內容，但願能在評論中收到你的回覆。

在上一篇咱們談了不少能夠在代碼審查時留意的事情。如今咱們會聚焦這個關注點：在測試代碼中找什麼。

本文假定：

你的團隊已經爲代碼寫過自動化測試。

測試在持續集成（CI）環境下按期運行。

審查的代碼已經通過自動編譯和測試流程，結果也經過了。

這篇中咱們會將介紹審查者在查看測試時須要考慮的事情。

新的或修改的代碼有測試嗎？

不管是修改 bug 仍是新功能開發，幾乎沒有新代碼不須要一個新的或更新過的測試去覆蓋它。甚至相似性能這樣的非功能性修改,也被證實每每須要通過測試驗證。若是在代碼審查中沒有測試，做爲審查者你應該問的第一個問題是「爲何沒有測試？」。

測試有覆蓋到代碼中使人困惑或者複雜的部分嗎？

比提出「有測試嗎？」這個問題更進一步是要回答這個問題，「重要的代碼是否被至少一個測試覆蓋？」。

檢查測試覆蓋率無疑是應該自動化的。但咱們不只僅能夠檢查覆蓋率的明確百分比，還可使用覆蓋率檢測工具來確保被覆蓋到的代碼位於正確的位置。

考慮下面這個例子：

你能夠檢查新代碼（應該容易識別，特別是當你使用相似 Upsource工具）的測試覆蓋率報告以確保充分覆蓋。

上面這個例子，審查者可讓做者增長一個測試去覆蓋 303 行 if 判斷爲真的狀況，由於測試工具標記304-306 行爲紅色，說明他們沒有被測試到。

對於任何團隊而言，100 % 的測試覆蓋都是不現實的目標，深刻了解哪些代碼須要被覆蓋到，比工具得出的數字更有用。

你特別想檢查全部的邏輯分支和複雜的代碼都有被覆蓋到。

我能理解這些測試麼？

進行測試，提供滿意的測試覆蓋率是一件事，但若是我這我的沒法理解這些測試，那它們的使用就被限制了。當它們出了問題該怎麼辦？很難知道要如何修復它們。

考慮下面這個測試：

這是一個很簡單的測試，可是我並不徹底肯定它測試什麼。它是測試 save 方法？仍是 getMyLongID ？並且爲何一樣的事須要作兩遍？

下面測試的意圖可能更清楚：

闡述測試目的所採起的特定步驟，取決於你使用的語言、庫、所在團隊和我的喜愛。這個例子證實經過選擇清楚的名字、內聯常量和增長註釋，做者可讓一個測試對於開發者而言更容易閱讀，而不只是他（她）本身。

這些測試須要知足什麼要求？

這是一個須要專門知識的領域。不管這些審覈的代碼須要知足的需求是在正式文檔裏，或許在一張用戶故事卡中，仍是在用戶提交的 bug裏，被審查的代碼都須要知足一些基本要求。（譯者注：在scrum開發流程中，用戶故事 User-Story 是從用戶的角度來描述用戶渴望獲得的功能，一般寫在一張卡片上）

審查者應該找到最初的需求來確認：

不管是單元測試、端對端測試、仍是其它的測試，這些測試是否知足這些要求。好比，若是要求是「容許特殊字符‘#’，‘!’ 和 ‘&’ 出如今密碼字段」，就應該有一個在密碼字段使用這些值的測試。若是測試使用其它的特殊字符，則不能證實代碼符合要求。

測試須要覆蓋全部的要求。在咱們特殊字符的例子中，要求可能繼續「……若是輸入了其它特殊字符，給用戶提示一個錯誤信息」。審查者在這裏應該檢查是否有一個測試來確認當使用一個錯誤字符會發生什麼。

我能夠考慮沒有被現有測試覆蓋到的用例嗎？

一般咱們的要求不是清晰明確。在這種情形下，審查者應該考慮最初的 bug、問題、用例中沒有考慮到的邊界條件。

例如，咱們有一個新功能，即「讓用戶能夠登陸系統」，審查者應該想：「若是用戶輸入的用戶名爲空會發生什麼？」。或者「若是系統中不存在該用戶，會發生什麼錯誤？」。若是被審查代碼有這些測試，審查者就比較有信心代碼自己會處理這種狀況。反之，若是這些異經常使用例不存在，審查者就不得不仔細檢查代碼，看它們是否有正確處理。

若是代碼裏有但沒有相應的測試，要由團隊決定大家的策略 —— 是否須要讓做者加上這些測試？或者你以爲經過代碼審查保證邊界條件被覆蓋到就行了？

這些測試是否有說明代碼的限制條件？

審查者常常會看到審查代碼中的限制條件。這些限制條件有時是故意設計的 —— 例如，一個批處理最多隻能處理 1000 個條目。

一種說明這些刻意限制條件的方法就是明確地測試他們。在上個例子，咱們能夠用一個測試來證實當批處理大小超過 1000，會有某種異常發生。

自動化測試中，沒必要代表這些限制條件，但若是做者寫了一個測試代表他們實現中的這個限制，這就意味着這些限制是有意的（有文檔說明的）而不是疏忽形成的。

審查代碼中的測試類型、測試級別正確嗎？

例如，若是單元測試就足夠了，做者還須要作昂貴的集成測試麼？他們寫的性能微基準能不能在 CI 環境下以一致方式有效地運行呢？

理想狀況下自動化測試會盡量快地運行，這樣就不須要用昂貴的端對端測試去檢查全部類型的功能。用一個數學運算或者布爾邏輯檢查的函數，能夠很好地替代方法級單元測試。

有沒有針對安全性的測試？

代碼安全性能夠從代碼審查中受益。咱們後續將有一篇安全方面的文章，但就測試而言，咱們能夠編寫測試覆蓋一些常見問題。例如，咱們上面提到的登陸代碼，咱們能夠寫一個測試，看若是沒有受權咱們是否沒法進入網站中被保護的區域（或稱爲保護 API 方法）。

性能測試

在上篇中我談到性能是審查者須要檢查的部分。自動化性能測試是另一類測試，我本能夠在本篇探討，但後面會寫一篇在代碼審查中具體看性能方面的文章，咱們在那裏再討論這種類型的測試。

審查者也能夠寫測試

不一樣組織有不一樣的代碼審查方法 —— 有時很是明確地要求做者負責全部的代碼改動，有時會和審查者協做由審查者本身提交代碼的評審建議。

不管採起哪一種方法，做爲審查者你會發現，寫一些額外的測試去玩一下審查的代碼，對於理解代碼是頗有幫助的。相似地，你也能夠啓動UI界面嘗試一下新功能，一樣也是頗有意義的。有些方法和工具能夠很容易被用來試驗代碼。從團隊的利益出發，在代碼審查中要讓代碼和玩代碼變得儘量容易。

提交額外的測試做爲審查的一部分固然很好，但也不是必須的，例如這個實驗已經給我這個審查者滿意的答案。

總結

不管你在組織中如何執行，作代碼審查有很大好處。代碼審查能夠在代碼集成到主線以前，就發現代碼中可能出現的問題。這個階段，開發者還記得前因後果，修正問題的成本也不高。

做爲一個代碼審查者，你應該要檢查最初的開發者放在他（她）代碼中的想法，哪些狀況下會變壞，處理邊界條件，用自動化測試「記錄」預期的行爲（正常條件和異常狀況）。

若是審查者查找已有的測試，檢查測試的正確性，那麼大家團隊對代碼會頗有信心。並且，若是這些測試在 CI 環境下被按期執行，你能夠看到這些代碼一直能夠工做 —— 他們提供了自動化迴歸檢查。若是代碼審查者很看重他們審查的代碼，要求高質量的測試，那麼這個代碼審查的價值在審查者按下「接受」按鈕後，還會延續下去。

在代碼審查系列的第三篇，咱們將涉及就性能而言審查代碼時須要留意什麼。

和全部的架構或設計領域同樣，一個系統性能的非功能性要求應該在前期就肯定下來。不管你是工做在一個響應要求在納秒級別的低延遲交易系統上，仍是在建立一個須要響應客戶的在線商店，或者正在編寫一個電話應用去管理 「待辦事項」列表，對「太慢」都要有相應的認識（譯者注：環境不一樣，含義不一樣）。

讓咱們來討論一些影響性能的事情，審查者能夠在代碼審查時留意。

首先

這部分功能有硬性的性能需求嗎？

這部分審查的代碼是否屬於一個之前發佈的服務層級協議（Service Level Agreement SLA）？或者在需求中有規定必需的性能指標嗎？

若是最初的需求來自於一個bug —— 「登陸界面加載太慢」，最初的開發者應該搞清楚一個合理的加載時間是多少 —— 不然審查者或者做者如何相信這個優化後的速度知足需求？

若是有，有測試去檢查嗎？

任何對性能敏感的系統都應該有自動化測試，來保證能夠知足公佈的 SLA（在 10 ms 內爲全部訂單需求提供服務）。若是沒有這些，你就只有等到客戶投訴時才知道你沒有知足 SLA。 這不只讓用戶感受很糟，並且還會致使了沒必要要的罰款和費用。在這系列的上一篇裏，已經詳細討論了代碼審查的測試部分。

功能修改或新功能對已有性能測試結果有不良影響嗎？

若是你會按期執行性能測試（或者你有一套能夠按須要執行的測試集），應該檢查對性能有嚴格要求部分的新代碼，確保它不會形成系統性能降低。 這能夠是一個自動化的流程，但性能測試一般不像單元測試那麼廣泛地被集成到 CI 環境中，因此值得強調這一審查步驟。

代碼審查中沒有硬性的性能需求要怎麼作？

若是通過數小時的痛苦，只節省了一點點 CPU 週期，這樣的優化實在划不來。但有些事情若是審查者檢查了，就能夠保證代碼不會存在常見的性能缺陷 （徹底能夠避免）。檢查列表的其餘部分，看是否是有簡單的方法能夠防止將來可能出現的性能問題。

服務或應用外的調用開銷很大

在本身的應用以外使用須要跨網絡躍點的系統和使用一個優化不好的 equals() 方法相比，前者開銷更大。請考慮這些：（譯者注：網絡躍點 network hop 即路由。一個路由爲一個躍點。傳輸過程當中須要通過多個網絡，每一個被通過的網絡設備點（有能力路由的）叫作一個躍點，地址就是它的 IP）

數據庫調用

最大的麻煩可能躲在抽象後面，好比 ORM（Object Relational Mapping，對象關係映射）。但在代碼審查時，你應該可以找到性能問題的常見緣由，就像一個循環裏的屢次數據庫調用 —— 例如 ，加載一個 ID 列表，而後基於每一個 ID 查詢數據庫裏的對應項目。

例如 ，19 行看起來沒有什麼問題（無辜的），但它在一個 for 循環裏 —— 你不知道這行代碼會致使多少次數據庫調用。

沒必要要的網絡調用

如同數據庫，有時遠程服務也被過分使用。就像有些地方使用了屢次遠程調用，其實一次就足夠了，或者能夠用批處理或緩存避免開銷巨大的網絡調用。另外一個和數據庫相似的是，一個抽象有時候會隱藏一個實際在呼叫遠程 API的方法調用。

移動程序、可穿戴程序過於頻繁地調用後端

這基本上和「沒必要要的網絡調用」相似，但在移動設備上還增長了一個問題，沒必要要的調用不只下降性能，並且還很耗電。

高效且有效果地使用資源

除了注意如何使用網絡資源外，審查者還能夠查看其餘的資源使用，來肯定可能的性能問題。

代碼經過鎖來訪問共享代碼嗎？這樣會致使性能降低和死鎖嗎？

鎖是性能的殺手，特別是在多線程環境下。考慮這樣的方式：僅有一個線程寫入或改變數值，而其餘線程能夠任意讀取；或者使用無鎖算法。

代碼會形成內存泄漏嗎？

一些 Java 中的常見緣由是：可變靜態字段，使用 ThreadLocal 和 ClassLoader。

應用程序的內存會無限增長嗎？

這和內存泄漏不一樣 —— 內存泄漏是因爲垃圾收集器不能回收再也不使用的對象。但任何語言，包含那些沒有垃圾收集機制的語言，均可以建立無限增加的數據結構。做爲審查者，若是你看到新的值不斷被加入一個列表或者映射表（Map） ，問一下這個列表或者映射表是否或者什麼時候被釋放或者減小。

上面的代碼中咱們能夠看到，全部來自 Twitter 的消息都被加入一個映射表。若是咱們仔細檢查這個類，咱們發現 allTwitterUsers 映射表歷來沒有減小過，TwitterUser中的 tweets 列表也沒有。這個映射表可能很快就變得很大，這取決於咱們監控用戶的數量和咱們添加 tweets 的頻率。

代碼有關閉鏈接或流嗎？

一不當心就會忘記關閉鏈接、文件或網絡流。當你審查其餘人（不管什麼語言）的代碼時，確保每一個使用的文件、網絡或數據庫鏈接都有被正確地關閉。

上面代碼很幸運地編譯經過，最初的做者很容易忽視這個問題。做爲審查者你應該注意 Connection、Statement 和 ResultSet 這些對象在方法退出前都要關閉。在 Java 7 中經過 try-with-resources 能夠很容易作到這一點。下面的截屏顯示了做者利用 try-with-resources 修改代碼後代碼審查的結果。

資源池配置正確嗎？

一個環境的最優配置受不少因素的影響，做爲審查者你不可能立刻知道，就好像一個數據庫鏈接池的大小是否合適。可是有些事情你能夠很容易判斷，好比這個池子過小（大小爲1）或者太大（數以百萬計的線程）。優化這些值須要通過測試，測試環境越接近真實環境越好，但若是池子（例如線程池或者鏈接池）真的太大，在代碼審查時能夠抓出這個常見問題。邏輯代表越大越好，但每一個對象都會消耗資源，對象太多一般會形成管理的開銷比帶來的好處大不少。若是有疑問，最好先使用默認設定。沒有使用默認設定的代碼須要經過某些測試或計算，來驗證數值是合理的 。

審查者容易發現警告標誌

某些類型的代碼能夠當即顯示一個可能的性能問題。這取決於使用的語言和函數庫（請讓咱們知道，在大家環境下對「代碼異味」的評價）。

（譯者注：代碼異味 code smells，代碼中的任何可能致使深層次問題的症狀均可以叫作代碼異味。代碼異味包括重複代碼、巨型類、方法過長等。）

反射

在Java中使用反射比不使用要慢不少。若是你檢查包含反射的代碼，問一下反射是否是必定須要。

上面的截屏顯示了一個審查者在 Upsource 裏點擊一個方法確認它來自哪裏，能夠看到這個方法來自 java.lang.reflect 包，這就應該是一個警告標誌。

超時

當你審查代碼時，你可能不知道一個操做的正常超時應該是多少，但應該這樣考慮「若是超時，系統中的其餘部分會受到什麼影響？」。做爲審查者你應該考慮最壞的狀況 —— 若是 5 分鐘超時到了，應用程序會卡住嗎？若是超時被設置爲1秒，最壞會發生什麼？若是代碼的做者沒法判斷超時的大小，而你做爲審查者也不知道如何選擇一個值，這時最好讓懂行的人蔘與進來。不要等到你的用戶向你抱怨性能問題。

並行性

代碼中有使用多個線程來執行一個簡單的操做嗎？除了增長時間和複雜性外，卻沒有帶來性能的提升？現代Java中，並行性比直接建立線程更加微妙：代碼有使用Java 8 炫酷的並行流機制，可是卻沒有從並行性中受益嗎？例如在並行流中處理很是簡單的操做，可能會比在一個順序流上執行慢不少。

上面代碼中增長的並行性沒有給咱們帶來任何好處 —— 這個流做用在 Tweet 上，上面一個字符串不會超過140個字符。就把那麼幾句話的操做並行化，也許不會改善性能，反而拆分紅並行線程的開銷會更大。

正確性

這些事情不必定會影響系統的性能，但它們就會影響到正確性，由於它們很大程度上和運行在多線程環境有關。

多線程環境下的代碼使用了正確的數據結構嗎？

上面的代碼中，做者在第 12 行用一個 ArrayList 存儲全部的會話（session）。但這代碼特別是 onOpen 方法會被多個線程調用，sessions 字段就必須是一個線程安全的數據結構。這種狀況下有多種選擇：可使用Vector，也能夠用 Collections.synchronizedList()建立一個線程安全列表（List），但最好的選擇是用CopyOnWriteArrayList，由於對這個列表的修改一般遠低於對它的讀取。

代碼容易出現競態條件嗎？

多線程環境下，編寫代碼很容易形成微妙的競爭問題。例如：

雖然遞增代碼只有一行（第16 行），但從代碼讀取值到設定新值的這段時間裏，另外一個線程極可能已經增長了 orders 的值。做爲審查者，要注意 get 和 set 組合都不是原子操做。

代碼中對鎖的使用正確嗎？

對於競態條件，做爲審查者你應該要檢查代碼，不容許多個線程用可能會引起衝突的方式修改數值。應該要使用同步、鎖或者原子變量來控制修改的代碼塊。

代碼的性能測試有價值嗎？

例如，一不當心就會寫出糟糕的微基準測試。或者若是測試使用的數據和量產數據不同，也會獲得一個錯誤的結果。

緩存

雖然緩存能夠避免過多的外部請求，它也會面臨本身的挑戰。若是審查的代碼使用了緩存，你應該注意一些常見的問題，例如不正確的緩存項失效。

代碼級優化

若是你在審查代碼，同時你也是一名開發者，下面的內容中可能會有你想建議的優化方法。做爲團隊，大家應該很清楚性能對大家有多重要，這些優化方法對大家的代碼是否有幫助。

大部分公司不會開發低延遲的應用程序，這些優化極可能都屬於過早的優化。

代碼中是否使用了沒必要要的同步、鎖？若是代碼老是執行在單線程下，加鎖只會帶來額外開銷。

代碼中是否使用了沒必要要的線程安全數據結構？例如，能夠用ArrayList 替換 Vector 嗎？

代碼中是否使用了在常見操做上性能不好的數據結構？例如，使用了鏈表結構，卻常常搜索其中的一項。

代碼是否使用了鎖或者同步機制，而實際上能夠用原子變量替代？

代碼能夠得益於延遲加載嗎？

if 語句或其餘邏輯語句能經過短路機制進行優化嗎？好比把最快的計算放在條件的開始？

有不少的字符串格式嗎？能夠更有效率嗎？

log 語句有使用字符串格式化嗎？有用檢查log級別的 if 語句包起來或者使用的日誌提供者能夠進行延遲操做？

上面代碼只會在 logger 被設定爲 FINSET 時纔會記錄消息。但不管這條消息是否真的被記錄下來，每次都會運行這個開銷很大的String.format 操做。

能夠像上面代碼那樣優化性能，即確保只在 log 級別等於某一數值時，才把消息被寫入在 log。

Java 8中，可使用 lambda 表達式來提升性能，而不是使用 if。因爲使用到 lambda，除非消息真的被記錄下來，String.format 不會執行。這應該是Java 8推薦的方法。

涉及到性能有不少要考慮的事情……

正如我在第一篇中列出代碼審查須要留意的事情，本文強調了其中的一些關注點，多是大家團隊在審查時會持續檢查的。這取決於大家項目的性能需求。

雖然本文是面向全部的開發人員，但大部分例子都是使用Java或 JVM。我最後介紹一些簡單的事情，是在審查 Java代碼時須要留意的，這樣可讓JVM 而不是你本身去優化代碼：

編寫小的方法和類。

保持邏輯簡單 —— 不要使用深刻嵌套的 if 或 循環。

代碼越容易讓人閱讀，JIT 編譯器越有可能理解你的代碼從而去優化它 。 若是代碼看上去容易理解和整潔，它的性能也可能很好 —— 這應該很容易在代碼審查時發現。

總結

談到性能，你要理解經過代碼審查，某些方面能夠快速獲得改善（好比，沒有必要的數據庫調用），有些方面也很誘人（就像代碼級優化）但極可能不會給大家的系統帶來很大的改善。

這是代碼審查「查」什麼系列第5篇。 查看該系列之前的文章。
在今天的文章中，咱們將更多地關注代碼自己的設計，專門檢查代碼是否遵循了良好的面向對象設計實踐。與全部咱們討論的其餘方面同樣，並非全部的團隊將它做爲最高準則來優先檢查，可是若是你嘗試遵循SOLID原則，或嘗試將你的代碼往這個方向上努力，這裏的一些建議可能會對你有所幫助。

什麼是SOLID？

SOLID原則 是面向對象程序設計和編程的五個核心原則。這篇文章的目的並非教會你這些原則是什麼或者深刻探討爲何要遵循它們，而是指出那些代碼評審中發現的代碼異味，它們多是沒有遵循這些原則的結果。

SOLID表明：

S – 單一職責原則

O – 開放封閉原則

L – 里氏替換原則

I – 接口分離原則

D – 依賴倒置原則

單一職責原則（SRP）

不該該有多種狀況須要修改某個類的對象。

僅僅經過代碼評審有時會很難發現上述狀況。根據定義，做者是（或應該是）由一個獨立的要求去改變代碼庫——修復一個bug，添加一個新功能，或一次專門的重構。

你應該看看在一個類中哪些方法在同一時間會改變，以及哪些方法幾乎是不可能受其餘方法變化影響而被改變。例如：

上面Upsource工具排展現的diff中，一個新功能被添加到TweetMonitor，可以基於user接口的某種排序畫出Tweeter的十大排行榜。雖然這彷佛是合理的， 由於它使用了onMessage方法收集的數據，但有跡象顯示這違反了SRP。onMessage和 getTweetMessageFromFullTweet方法都是和接收或解析一個Twitter消息相關，而draw用來組織數據並在UI上顯示。

代碼審查者應該標記這兩個職責，而後和做者研究出一個分離這些特性的更好方法：也許經過移動Twitter字符串解析方法到不一樣的類，或者建立一個不一樣的類負責來渲染排行榜。

開放封閉原則（OCP）

軟件實體應該對擴展開放，對修改封閉。
做爲一個代碼審查者，若是你看到一系列的if語句檢查對象的特殊類型，可能這就是違反該原則的跡象：

若是你審查上面的代碼應該很清楚，當一個新的Event類型添加到系統，新建立的Event類型可能要添加另外一個else到這個方法中來處理它。

最好使用多態來移除這個if：

像往常同樣，這個問題的解決方案不止一個，但關鍵是把複雜的if/else和instanceof判斷移除。

里氏替換原則（LSP）

函數使用基類的引用，必須可以在不知不覺的狀況下使用派生類的對象。

發現違反這一原則的一個簡單的方法是尋找顯示的類型轉換。若是你不得不把一個對象轉換爲某種類型，你就沒有作到不知不覺地使用繼承了基類的子類對象。

檢查如下兩條時能夠找到更細微的衝突：

先決條件不能在子類型增強。

後置條件不能在子類型減弱。

例如，想象咱們有一個抽象類Order和一些子類——BookOrder、ElectronicsOrder等等。placeOrder方法可能須要一個Warehouse，能夠用這個方法來改變倉庫中的實物產品的庫存水平：

如今想象一下，咱們引入電子禮品卡，只需簡單地爲錢包添加餘額，但不須要物理盤點。若是實現爲GiftCardOrder，placeOrder方法就沒必要使用warehouse參數：

這可能在邏輯上看起來像繼承，但事實上你能夠說使用GiftCardOrder應該指望這個類和其餘類有相似的行爲，例如，你應該指望這個參數傳遞給全部子類：

但這不會傳遞，做爲GiftCardOrders有不一樣類型的order行爲。若是你審查這樣的代碼，應該質疑這裏繼承的使用，也許order行爲能夠經過使用組合替代繼承的方式插入。

接口分離原則（ISP）

多個客戶端特定的接口比使用一個通用的接口要好。

能夠很容易識別某些代碼違反這一原則，由於接口上具備不少方法。這一原則與SRP呼應，正如你可能看到一個接口具備許多方法，實際上它是負責了多個功能。

但有時甚至一個接口只有兩個方法也能夠拆分紅兩個接口：

在這個例子中，有些時候可能不須要decode方法，而且一個編解碼器根據使用要求可能被視爲一個編碼器或解碼器，把SimpleCodec接口分割成一個編碼器和解碼器可能會更好。一些類能夠選擇兩個都實現，但沒必要去重載不須要的方法，或者某些類只須要編碼器，同時他們的編碼器實例也實現解碼。

依賴倒置原則（DIP）

依賴抽象。不要依賴於具體實現。

雖然想要在試圖尋找違反該原則的簡單例子時，相似處處使用new關鍵字（而不是使用依賴注入或工廠模式）和濫用集合類型（例如聲明ArrayList變量或參數而不是List），做爲一個代碼審查者你在代碼審查時，應該確保代碼做者已經使用或建立了正確的抽象。

例如，服務級的代碼直接鏈接到一個數據庫讀寫數據：

這段代碼依賴於大量的具體的實現細節：鏈接到一個（關係）數據庫的JDBC，基於具體數據庫的SQL，須要知道數據庫結構，等等。這段代碼來自你的系統的某處，可是不該該出如今這裏。這裏還有其餘方法不須要知道具體的數據庫。更好的方式是，提取出一個DAO或使用Repository模式，將DAO或repository注入到這個服務中。

總結

一些代碼異味可能意味着可能已經違反了一個或多個SOLID原則：

很長的if/else語句

強制轉換成一個子類型

不少公共方法

實現的方法拋出UnsupportedOperationException異常

與全部的設計問題同樣，在遵循這些原則和故意迴避這些原則間須要找到一個平衡，這個平衡取決於你的團隊的喜愛。可是若是你在代碼審查中看到複雜的代碼，會發現應用這些原則將提供一個更簡單、更容易理解的解決方案。