代碼審查者應(yīng)該關(guān)注哪些方面？

代碼審查時應(yīng)該關(guān)注以下方面：

• 設(shè)計：設(shè)計是否合理？
• 功能：是否滿足prd需求？是否滿足用戶流程交互合理性？
• 復雜度：代碼能更簡單嗎？將來其他開發(fā)人員能輕松理解并使用此代碼嗎？
• 測試：代碼是否具有正確且設(shè)計良好的單元測試？
• 命名：開發(fā)人員是否為變量、類、方法、包等選擇了明確的名稱？
• 注釋：注釋是否清晰有用？
• 風格：代碼是否遵守了代碼規(guī)范？
• 文檔：開發(fā)人員是否同時更新了相關(guān)文檔？

設(shè)計

審查中最重要的是 CL 的整體設(shè)計。CL 中各種代碼的交互是否有意義？現(xiàn)在是添加此功能的好時機嗎？

單一職責原則 (Single Responsibility Principle)

• 類中的代碼行數(shù)、函數(shù)或?qū)傩赃^多，會影響代碼的可讀性和可維護性，我們就需要考慮對類進行拆分；
• 類依賴的其他類過多，或者依賴類的其他類過多，不符合高內(nèi)聚、低耦合的設(shè)計思想，我們就需要考慮對類進行拆分；
• 私有方法過多，我們就要考慮能否將私有方法獨立到新的類中，設(shè)置為 public 方法，供更多的類使用，從而提高代碼的復用性；
• 比較難給類起一個合適名字，很難用一個業(yè)務(wù)名詞概括，或者只能用一些籠統(tǒng)的 Manager、Context 之類的詞語來命名，這就說明類的職責定義得可能不夠清晰；
• 類中大量的方法都是集中操作類中的某幾個屬性，那就可以考慮將這幾個屬性和對應(yīng)的方法拆分出來。

違背SRP原則的案例

1.CreditSchemaAbilityImpl中函數(shù)、依賴過多，充斥著類轉(zhuǎn)換邏輯、訪問facade邏輯、工廠邏輯、私有方法過多、大量私有方法最終只是得到一個字段值等。

2.User類中大量方法操作的都是UserAddress成員變量對象，而這些方法散落在User類而不是委托UserAddress類。

開閉原則 (Open-Closed Principle)

• 添加一個新的功能，應(yīng)該是通過在已有代碼基礎(chǔ)上擴展代碼（新增模塊、類、方法、屬性等），而非修改已有代碼（修改模塊、類、方法、屬性等）的方式來完成。關(guān)于定義，我們有兩點要注意。第一點是，開閉原則并不是說完全杜絕修改，而是以最小的修改代碼的代價來完成新功能的開發(fā)。第二點是，同樣的代碼改動，在粗代碼粒度下，可能被認定為“修改”；在細代碼粒度下，可能又被認定為“擴展”。

• 我們要時刻具備擴展意識、抽象意識、封裝意識。在寫代碼的時候，我們要多花點時間思考一下，這段代碼未來可能有哪些需求變更，如何設(shè)計代碼結(jié)構(gòu)，事先留好擴展點，以便在未來需求變更的時候，在不改動代碼整體結(jié)構(gòu)、做到最小代碼改動的情況下，將新的代碼靈活地插入到擴展點上。

最常用來提高代碼擴展性的方法有：多態(tài)、依賴注入、基于接口而非實現(xiàn)編程，以及大部分的設(shè)計模式（比如，裝飾、策略、模板、職責鏈、狀態(tài)）。

違背OCP原則的案例

public class Alert {
  private AlertRule rule;
  private Notification notification;

  public Alert(AlertRule rule, Notification notification) {
    this.rule = rule;
    this.notification = notification;
  }

  public void check(String api, long requestCount, long errorCount, long durationOfSeconds) {
    long tps = requestCount / durationOfSeconds;
    if (tps > rule.getMatchedRule(api).getMaxTps()) {
      notification.notify(NotificationEmergencyLevel.URGENCY, "...");
    }
    if (errorCount > rule.getMatchedRule(api).getMaxErrorCount()) {
      notification.notify(NotificationEmergencyLevel.SEVERE, "...");
    }
  }
}

上面這段代碼非常簡單，業(yè)務(wù)邏輯主要集中在 check() 函數(shù)中。當接口的 TPS 超過某個預先設(shè)置的最大值時，以及當接口請求出錯數(shù)大于某個最大允許值時，就會觸發(fā)告警，通知接口的相關(guān)負責人或者團隊?，F(xiàn)在，如果我們需要添加一個功能，當每秒鐘接口超時請求個數(shù)，超過某個預先設(shè)置的最大閾值時，我們也要觸發(fā)告警發(fā)送通知。這個時候，我們該如何改動代碼呢？主要的改動有兩處：第一處是修改 check() 函數(shù)的入?yún)ⅲ砑右粋€新的統(tǒng)計數(shù)據(jù) timeoutCount，表示超時接口請求數(shù)；第二處是在 check() 函數(shù)中添加新的告警邏輯。具體的代碼改動如下所示：

public class Alert {
  // ...省略AlertRule/Notification屬性和構(gòu)造函數(shù)...
  
  // 改動一：添加參數(shù)timeoutCount
  public void check(String api, long requestCount, long errorCount, long timeoutCount, long durationOfSeconds) {
    long tps = requestCount / durationOfSeconds;
    if (tps > rule.getMatchedRule(api).getMaxTps()) {
      notification.notify(NotificationEmergencyLevel.URGENCY, "...");
    }
    if (errorCount > rule.getMatchedRule(api).getMaxErrorCount()) {
      notification.notify(NotificationEmergencyLevel.SEVERE, "...");
    }
    // 改動二：添加接口超時處理邏輯
    long timeoutTps = timeoutCount / durationOfSeconds;
    if (timeoutTps > rule.getMatchedRule(api).getMaxTimeoutTps()) {
      notification.notify(NotificationEmergencyLevel.URGENCY, "...");
    }
  }
}

這樣的代碼修改實際上存在挺多問題的。一方面，我們對接口進行了修改，這就意味著調(diào)用這個接口的代碼都要做相應(yīng)的修改。另一方面，修改了 check() 函數(shù)，相應(yīng)的單元測試都需要修改（關(guān)于單元測試的內(nèi)容我們在重構(gòu)那部分會詳細介紹）。

里氏替換原則 (Liskov Substitution Principle)

• 子類對象（object of subtype/derived class）能夠替換程序（program）中父類對象（object of base/parent class）出現(xiàn)的任何地方，并且保證原來程序的邏輯行為（behavior）不變及正確性不被破壞。

• 判斷子類的設(shè)計實現(xiàn)是否違背里式替換原則，一個小竅門，那就是拿父類的單元測試去驗證子類的代碼。如果某些單元測試運行失敗，就有可能說明，子類的設(shè)計實現(xiàn)沒有完全地遵守父類的約定，子類有可能違背了里式替換原則。

違背LSP原則的案例

1. 子類違背父類聲明要實現(xiàn)的功能父類中提供的 sortOrdersByAmount() 訂單排序函數(shù)，是按照金額從小到大來給訂單排序的，而子類重寫這個 sortOrdersByAmount() 訂單排序函數(shù)之后，是按照創(chuàng)建日期來給訂單排序的。那子類的設(shè)計就違背里式替換原則。

2. 子類違背父類對輸入、輸出、異常的約定在父類中，某個函數(shù)約定：運行出錯的時候返回 null；獲取數(shù)據(jù)為空的時候返回空集合（empty collection）。而子類重載函數(shù)之后，實現(xiàn)變了，運行出錯返回異常（exception），獲取不到數(shù)據(jù)返回 null。那子類的設(shè)計就違背里式替換原則。在父類中，某個函數(shù)約定，輸入數(shù)據(jù)可以是任意整數(shù)，但子類實現(xiàn)的時候，只允許輸入數(shù)據(jù)是正整數(shù)，負數(shù)就拋出，也就是說，子類對輸入的數(shù)據(jù)的校驗比父類更加嚴格，那子類的設(shè)計就違背了里式替換原則。在父類中，某個函數(shù)約定，只會拋出 ArgumentNullException 異常，那子類的設(shè)計實現(xiàn)中只允許拋出 ArgumentNullException 異常，任何其他異常的拋出，都會導致子類違背里式替換原則。

3. 子類違背父類注釋中所羅列的任何特殊說明父類中定義的 withdraw() 提現(xiàn)函數(shù)的注釋是這么寫的：“用戶的提現(xiàn)金額不得超過賬戶余額……”，而子類重寫 withdraw() 函數(shù)之后，針對 VIP 賬號實現(xiàn)了透支提現(xiàn)的功能，也就是提現(xiàn)金額可以大于賬戶余額，那這個子類的設(shè)計也是不符合里式替換原則的。

依賴倒轉(zhuǎn)原則 (Dependence Inversion Principle)

• 高層模塊（high-level modules）不要依賴低層模塊（low-level）。高層模塊和低層模塊應(yīng)該通過抽象（abstractions）來互相依賴。除此之外，抽象（abstractions）不要依賴具體實現(xiàn)細節(jié)（details），具體實現(xiàn)細節(jié)（details）依賴抽象（abstractions）。

違背DIP原則的案例

1.在sop流程中提供通用功能，拓展點卻依賴了具體實現(xiàn)環(huán)節(jié)中的類，關(guān)心了具體實現(xiàn)。

2.在某個類中依賴了某個接口具體實現(xiàn)類，而不是依賴接口。

3.沒有按照DI方式分層，單測困難。

4.沒有通過spring ioc容器控制反轉(zhuǎn)，直接在類中new成員變量，單測困難 ， 依賴細節(jié)。

接口隔離原則 (Interface Segregation Principle)

客戶端不應(yīng)該被強迫依賴它不需要的接口。其中的“客戶端”，可以理解為接口的調(diào)用者或者使用者。

違背ISP原則的案例

1.為了程序分層方便api將對數(shù)據(jù)庫的邏輯DELETE方法也暴露對外了，造成風險。

2.由于接口設(shè)計不佳，授信領(lǐng)域間接被強迫依賴了支用領(lǐng)域接口。

3.一個接口方法功能太多，只能兼容一些入?yún)着芰撕芏喽嘤嗟倪壿嫛?/p>

迪米特法則（Law Of Demeter）

每個模塊（unit）只應(yīng)該了解那些與它關(guān)系密切的模塊（units: only units “closely” related to the current unit）的有限知識（knowledge）。或者說，每個模塊只和自己的朋友“說話”（talk），不和陌生人“說話”（talk）。

不該有直接依賴關(guān)系的類之間，不要有依賴；有依賴關(guān)系的類之間，盡量只依賴必要的接口（也就是定義中的“有限知識”）。

違背LOD原則的案例

1.文件上傳接口FileUploadInterface依賴了OSS的Bucket對象，而不是定義抽象的文件上傳參數(shù)，文件上傳不一定要用OSS實現(xiàn)的。

2.還款實體為了得到某個字段，依賴了征信實體，只因為征信實體里面有這個字段，獲取方便，但是征信不屬于還款領(lǐng)域概念。

功能

背景了解

Code Reviewer 要知道改動的背景，本次改動代碼是為了做什么？不能做什么？會影響什么？

邏輯分析

Code Reviewer 根據(jù)需求背景，對代碼邏輯進行分析，是否滿足需求，是否有業(yè)務(wù)邏輯漏洞。

邊界檢查

Code Reviewer 檢查改動影響的功能模塊，是否有邊界問題，如果是refactor change，應(yīng)當檢查兼容性，如果是feature change，需要檢查領(lǐng)域邊界是否合理。

異常監(jiān)控

Code Reviewer 檢查日志處理、異常處理、監(jiān)控埋點方式等代碼，是否存在缺陷、丟失。

非業(yè)務(wù)功能性檢查

Code Reviewer 檢查多線程、并發(fā)、代碼安全、性能等角度，是否存在漏洞，是否可以優(yōu)化。

復雜度

圈復雜度

Code Reviewer 檢查代碼圈復雜度情況（if while分支過多、方法分支太多），圈復雜度在1-10為合理范圍。

改進方法

• 簡化、合并條件表達式
• 將條件判定提煉出獨立函數(shù)
• 將大函數(shù)拆成小函數(shù)
• 以明確函數(shù)取代參數(shù)
• 替換算法、策略模式

方法長度

代碼長短可以直接說明它復不復雜、閱讀成本高不高，長度度量往往是復雜度分析里最簡單直接的手段。此處，有效代碼長度 = 代碼行 - 空白行 - 注釋行。當前，集團95%的方法的有效代碼行數(shù)在42以內(nèi)。

方法參數(shù)長度

方法參數(shù)過多會降低容錯性、模糊表達意圖，參數(shù)個數(shù)應(yīng)當在3個以內(nèi)，可以通過靜態(tài)創(chuàng)建、參數(shù)合并為Model等方式降低長度。

最高結(jié)構(gòu)控制層數(shù)

花括號 { } 的最大嵌套層數(shù)。深度嵌套的代碼總是像俄羅斯套娃一樣晦澀難懂，可讀性和可維護性大打折扣。當前，集團95%的方法的最大嵌套層數(shù)在3以內(nèi)。

設(shè)計復雜度

是否有over設(shè)計的情況，簡單的兩個分支，拓展性低的case，做成了策略模式之類的。

測試

case覆蓋率

Code Reviewer 檢查單測case覆蓋情況，是否覆蓋了主要業(yè)務(wù)流程，嚴格來說每個邏輯分支都需要覆蓋。

可測性

Code Reviewer 檢查代碼中是否有環(huán)境、外部、靜態(tài)類等依賴，如有，會影響單測隔離性，單測應(yīng)該可以通過打樁隔絕任何依賴。

規(guī)范

單測代碼也需要規(guī)范，清晰易讀，重復性低為準。

命名、注釋、風格

代碼規(guī)范

文檔

文檔更新

Code Reviewer 提醒 developer 在二分庫、應(yīng)用等升級之后更新文檔。

如何撰寫 Code Review 評論

總結(jié)

• 保持友善。
• 解釋你的推理。
• 在給出明確的指示與只指出問題并讓開發(fā)人員自己決定間做好平衡。
• 鼓勵開發(fā)人員簡化代碼或添加代碼注釋，而不僅僅是向你解釋復雜性。

糟糕的示例：“為什么這里你使用了線程，顯然并發(fā)并沒有帶來什么好處？”

好的示例：“這里的并發(fā)模型增加了系統(tǒng)的復雜性，但沒有任何實際的性能優(yōu)勢，因為沒有性能優(yōu)勢，最好是將這些代碼作為單線程處理而不是使用多線程。”

引用

• <設(shè)計模式之美> 王爭
• <Code Review Developer Guide> Google