引言

作者：方基成(潤甫)

作為卓越工程文化的一部分，Code Review 其實一直在進行中，只是各團隊根據(jù)自身情況張馳有度，松緊可能也不一，這里簡單梳理一下 CR 的方法和團隊實踐。

為什么要CR

• 提前發(fā)現(xiàn)缺陷在 CodeReview 階段發(fā)現(xiàn)的邏輯錯誤、業(yè)務理解偏差、性能隱患等時有發(fā)生， CR 可以提前發(fā)現(xiàn)問題。
• 提高代碼質量主要體現(xiàn)在代碼健壯性、設計合理性、代碼優(yōu)雅性等方面，持續(xù) CodeReview 可以提升團隊整體代碼質量。
• 統(tǒng)一規(guī)范和風格集團編碼規(guī)范自不必說，對于代碼風格要不要統(tǒng)一，可能會有不同的看法，個人觀點對于風格也不強求。但代碼其實不是寫給自己看的，是寫給下一任看的，就像經(jīng)常被調(diào)侃的“程序員不喜歡寫注釋，更不喜歡別人不寫注釋”，代碼風格的統(tǒng)一更有助于代碼的可讀性及繼任者的快速上手。
• 防止架構腐爛架構的維護者是誰？僅靠架構師或應用 Owner 是遠遠不夠的，需要所有成員的努力，所謂人人都是架構師。架構防腐最好前置在設計階段，但 CodeReview 作為對最終產(chǎn)出代碼的檢查，也算是最后一道關鍵工序。
• 知識分享每一次 CodeReview，都是一次知識的分享，磨合一定時間后，團隊成員間會你中有我、我中有你，集百家之所長，融百家之所思。同時，業(yè)務邏輯都在代碼中，團隊 CodeReview 也是一種新人業(yè)務細節(jié)學習的途徑。
• 團隊共識通過多次討論與交流，逐步達成團隊共識，特別是對架構理解和設計原則的認知，在共識的基礎上團隊也會更有凝聚力，特別是在較多新人加入時尤為重要。

他山之石

2.1 某大廠A

非常重視 Code Review，基本上代碼需要至少有兩位以上 Reviewer 審核通過后，才會讓你 Check In。

2.1.1 代碼評審準則

• 如果變更達到可以提升系統(tǒng)整體代碼質量的程度，就可以讓它們通過，即使它們可能還不完美。這是所有代碼評審準則的最高原則。
• 世界上沒有“完美”的代碼，只有更好的代碼。評審者不應該要求代碼提交者在每個細節(jié)都寫得很完美。評審者應該做好修改時間與修改重要性之間的權衡。

2.1.2 代碼評審原則

• 以客觀的技術因素與數(shù)據(jù)為準，而非個人偏好。
• 在代碼樣式上，遵從代碼樣式指南，所有代碼都應與其保持一致，任何與代碼樣式指南不一致的觀點都是個人偏好。但如果某項代碼樣式在指南中未提及，那就接受作者的樣式。
• 任務涉及軟件設計的問題，都應取決于基本設計原則，而不應由個人喜好來決定。當同時有多種可行方案時，如果作者能證明（以數(shù)據(jù)或公認的軟件工程原理為依據(jù)）這些方案基本差不多，那就接受作者的選項；否則，應由標準的軟件設計原則為準。
• 如果沒有可用的規(guī)則，那么審核者應該讓作者與當前代碼庫保持一致，至少不會惡化代碼系統(tǒng)的質量。（一旦惡化代碼質量，就會帶來破窗效應，導致系統(tǒng)的代碼質量逐漸下降）

2.1.3 代碼審核者應該看什么

設計：代碼是否設計良好？這種設計是否適合當前系統(tǒng)？

功能：代碼實現(xiàn)的行為與作者的期望是否相符？代碼實現(xiàn)的交互界面是否對用戶友好？

復雜性：代碼可以更簡單嗎？如果將來有其他開發(fā)者使用這段代碼，他能很快理解嗎？

測試：這段代碼是否有正確的、設計良好的自動化測試？

命名：在為變量、類名、方法等命名時，開發(fā)者使用的名稱是否清晰易懂？

注釋：所有的注釋是否都一目了然？

代碼樣式：所有的代碼是否都遵循代碼樣式？

文檔：開發(fā)者是否同時更新了相關文檔？

2.2 某大廠B

• 在開發(fā)流程上專門有這個環(huán)節(jié)，排期會明確排進日程，比如5天開發(fā)會排2天來做代碼審核，分為代碼自審、交叉審核、集中審核。

• 有明確的量化指標，如8人時審核/每千行代碼，8個以上非提示性有效問題/每千行代碼。

2.3 某大廠C

• 推行 Code Owner 機制，每個代碼變更必須有 Code Owner 審核通過才可以提交。

• 所有的一線工程師，無論職級高低，最重要的工程輸出原則是“show me the code”，而 Code Review 是最能夠反應這個客觀輸出的。

• 盡量讓每個人的 Code Review 參與狀況都公開透明，每個變更發(fā)送給項目合作者，及轉發(fā)到小組內(nèi)成員，小組內(nèi)任何人都可以去 Review 其他人的代碼。

• 明確每個人的考評和 Code Review 表現(xiàn)相關，包括 Code Review 輸出狀況及提交代碼的質量等。

我們怎么做 CR

3.1 作為代碼提交者

• 發(fā)起時機：發(fā)起 Code Review 盡量提前，開發(fā)過程小步快跑

代碼行數(shù)：提交 Code Review 的代碼行數(shù)最好在400行以下。根據(jù)數(shù)據(jù)分析發(fā)現(xiàn)，從代碼行數(shù)來看，超過400行的 CR，缺陷發(fā)現(xiàn)率會急劇下降；從 CR 速度來看，超過500行/小時后，Review 質量也會大大降低，一個高質量的 CR 最好控制在一個小時以內(nèi)。

明確意圖：編寫語義明確的標題（必填）和描述（選填，可以包括背景、思路、改造點和影響面、風險等）

善用工具：IDEA 打開編碼規(guī)約實時檢測，減少代碼樣式、編碼規(guī)約等基礎性問題

阿里編碼規(guī)約插件： github.com/alibaba/p3c…

3.2 作為代碼評審者

3.2.1 評審范圍

主要從兩方面來評審：

代碼邏輯

• 功能完整：代碼實現(xiàn)是否滿足功能需求，實現(xiàn)上有沒有需求的理解偏差，對用戶是否友好；
• 邏輯設計：是否考慮了全局設計和兼容現(xiàn)有業(yè)務細節(jié)，是否考慮邊界條件和并發(fā)控制；
• 安全隱患：是否存在數(shù)據(jù)安全隱患及敏感信息泄漏，如越權、SQL注入、CSRF、敏感信息未脫敏等；
• 性能隱患：是否存在損害性能的隱患，如死鎖、死循環(huán)、FullGC、慢SQL、緩存數(shù)據(jù)熱點等；
• 測試用例：單元測試用例的驗證邏輯是否有效，測試用例的代碼行覆蓋率和分支覆蓋率；

代碼質量

• 編碼規(guī)范：命名、注釋、領域術語、架構分層、日志打印、代碼樣式等是否符合規(guī)范
• 可讀性：是否邏輯清晰、易理解，避免使用奇巧技，避免過度拆分
• 簡潔性：是否有重復可簡化的復雜邏輯，代碼復雜度是否過高，符合KISS和DRY原則
• 可維護性：在可讀性和簡潔性基礎上，是否分層清晰、模塊化合理、高內(nèi)聚低耦合、遵從基本設計原則
• 可擴展性：是否僅僅是滿足一次性需求的代碼，是否有必要的前瞻性擴展設計
• 可測試性：代碼是否方便寫單元測試及分支覆蓋，是否便于自動化測試

3.2.2 評審注意事項

• 盡快完成評審
• 避免過度追求完美
• 明確評論是否要解決
• 避免使用反問句來評價

我們主要是通過交叉 CR、集中 CR 相結合的方式，由應用 Owner+SM+架構師+TL完成。

CR 怎么避免流于形式

CR 流于形式的因素很多，大概如下：

不認同 CodeReview

• 評審者的姿態(tài)？有沒有帶來好處？有沒有從中收獲？這些都會直觀影響團隊成員的認可度
• 每個 Review 建議的提出都是一次思想交流，評論要友好、中肯、具體，避免教條式及負面詞匯，在遵守評審原則下，同時尊重個性展現(xiàn)
• 團隊集中 CodeReview 盡量不要太正式和嚴肅，輕松的氣氛下更有助于互相理解，來點水果，聊聊業(yè)務聊聊代碼
• 在 Review 過程有時候會陷入誰對誰錯的爭論，只要是為了尋求真理辯證的去看問題，哪怕是討論再激烈也是有收獲的，注意只對事不對人。

CodeReview 后改動太大

發(fā)布前發(fā)現(xiàn)問題多，改動太大，影響項目計劃

大項目要求編碼前設計評審，小需求可以事先Review設計思路，避免最后的驚喜

每次 Review 的代碼行數(shù)最好控制在數(shù)百行以內(nèi)

評審者沒有足夠時間

評審者在任務安排上盡量預留好時間

盡快評審，代碼在百行以內(nèi)及時響應，在千行以內(nèi)當日完結

評審者不了解業(yè)務和代碼

代碼提交人編寫清晰的標題和描述

有必要的情況下評審者需要了解PRD

評審者需要提前了解系統(tǒng)和代碼

Review 建議未修改

這一點極為重要，需要對修改后的代碼再次 Review，確保理解一致，以及預防帶問題上線

應用可以設置 Review 建議需全部解決的卡點，同時對于非必需修改的建議可以進行打標或說明

CR 實踐中發(fā)現(xiàn)的幾個常見代碼問題

筆者對個人 CR 評論問題做了個大概統(tǒng)計，Bug 發(fā)現(xiàn)數(shù)占比約4%（直接或潛在Bug），重復代碼數(shù)占比約5%，其他還有規(guī)范、安全、性能、設計等問題。在CR代碼質量時，可以參考《重構：改善既有代碼的設計》，書中所列的22種壞味道在CR中基本都會遇到。而此處我們主要聚焦以下幾個常見問題：

5.1 DRY

DRY 是 Don't Repeat Yourself 的縮寫，DRY 是 Andy Hunt 和 Dave Thomas's 在《 The Pragmatic Programmer 》一書中提出的核心原則。DRY 原則描述的重復是知識和意圖的重復，包含代碼重復、文檔重復、數(shù)據(jù)重復、表征重復，我們這里重點講講代碼重復。

5.1.1 代碼重復

《重構》中對“Duplicated Code（重復代碼）”的描述：壞味道行列中首當其沖的就是Duplicated Code。如果你在一個以上的地點看到相同的程序結構，那么可以肯定：設法將它們合而為一，程序會變得更好。

最單純的Duplicated Code就是“同一個類的兩個函數(shù)含有相同的表達式”。這時候你需要做的就是采用Extract Method (110)提煉出重復的代碼，然后讓這兩個地點都調(diào)用被提煉出來的那一段代碼。

另一種常見情況就是“兩個互為兄弟的子類內(nèi)含相同表達式”。要避免這種情況，只需對兩個類都使用Extract Method (110)，然后再對被提煉出來的代碼使用Pull Up Method (332)，將它推入超類內(nèi)。如果代碼之間只是類似，并非完全相同，那么就得運用Extract Method (110)將相似部分和差異部分割開，構成單獨一個函數(shù)。然后你可能發(fā)現(xiàn)可以運用Form Template Method (345)獲得一個Template Method設計模式。如果有些函數(shù)以不同的算法做相同的事，你可以選擇其中較清晰的一個，并使用Substitute Algorithm (139)將其他函數(shù)的算法替換掉。

如果兩個毫不相關的類出現(xiàn)Duplicated Code，你應該考慮對其中一個使用Extract Class (149)，將重復代碼提煉到一個獨立類中，然后在另一個類內(nèi)使用這個新類。但是，重復代碼所在的函數(shù)也可能的確只應該屬于某個類，另一個類只能調(diào)用它，抑或這個函數(shù)可能屬于第三個類，而另兩個類應該引用這第三個類。你必須決定這個函數(shù)放在哪兒最合適，并確保它被安置后就不會再在其他任何地方出現(xiàn)。

代碼重復的幾種場景：

• 一個類中重復代碼抽象為一個方法
• 兩個子類間重復代碼抽象到父類
• 兩個不相關類間重復代碼抽象到第三個類

CASE：

反例

private BillVO convertBillDTO2BillVO(BillDTO billDTO) {
    if (billDTO == null) {
        return null;
    }
    BillVO billVO = new BillVO();
    Money cost = billDTO.getCost();
    if (cost != null && cost.getAmount() != null) {
        billVO.setCostDisplayText(String.format("%s %s", cost.getCurrency(), cost.getAmount()));
    }
    Money sale = billDTO.getSale();
    if (sale != null && sale.getAmount() != null) {
        billVO.setSaleDisplayText(String.format("%s %s", sale.getCurrency(), sale.getAmount()));
    }
    Money grossProfit = billDTO.getGrossProfit();
    if (grossProfit != null && grossProfit.getAmount() != null) {
        billVO.setGrossProfitDisplayText(String.format("%s %s", grossProfit.getCurrency(), grossProfit.getAmount()));
    }
    return billVO;
}

正例

private static final String MONEY_DISPLAY_TEXT_PATTERN = "%s %s";
private BillVO convertBillDTO2BillVO(BillDTO billDTO) {
    if (billDTO == null) {
        return null;
    }
    BillVO billVO = new BillVO();
    billVO.setCostDisplayText(buildMoneyDisplayText(billDTO.getCost()));
    billVO.setSaleDisplayText(buildMoneyDisplayText(billDTO.getSale()));
    billVO.setGrossProfitDisplayText(buildMoneyDisplayText(billDTO.getGrossProfit()));
    return billVO;
}
private String buildMoneyDisplayText(Money money) {
    if (money == null || money.getAmount() == null) {
        return StringUtils.EMPTY;
    }
    return String.format(MONEY_DISPLAY_TEXT_PATTERN, money.getCurrency(), money.getAmount().toPlainString());
}

5.1.2 DYR 實踐忠告：

• 不要借用 DRY 之名，過度提前抽象，請遵循 Rule of three 原則。
• 不要過度追求 DRY，破壞了內(nèi)聚性，實踐中需要平衡復用與內(nèi)聚。

5.2 Primitive Obsession

《重構》中對“Primitive Obsession（基本類型偏執(zhí)）”的描述：大多數(shù)編程環(huán)境都有兩種數(shù)據(jù)：結構類型允許你將數(shù)據(jù)組織成有意義的形式；基本類型則是構成結構類型的積木塊。結構總是會帶來一定的額外開銷。它們可能代表著數(shù)據(jù)庫中的表，如果只為做一兩件事而創(chuàng)建結構類型也可能顯得太麻煩。

對象的一個極大的價值在于：它們模糊（甚至打破）了橫亙于基本數(shù)據(jù)和體積較大的類之間的界限。你可以輕松編寫出一些與語言內(nèi)置（基本）類型無異的小型類。例如，Java 就以基本類型表示數(shù)值，而以類表示字符串和日期——這兩個類型在其他許多編程環(huán)境中都以基本類型表現(xiàn)。

對象技術的新手通常不愿意在小任務上運用小對象——像是結合數(shù)值和幣種的 money 類、由一個起始值和一個結束值組成的 range 類、電話號碼或郵政編碼（ZIP)等的特殊字符串。你可以運用 Replace Data Valuewith Object (175)將原本單獨存在的數(shù)據(jù)值替換為對象，從而走出傳統(tǒng)的洞窟，進入炙手可熱的對象世界。如果想要替換的數(shù)據(jù)值是類型碼，而它并不影響行為，則可以運用 Replace Type Code with Class (218)將它換掉。如果你有與類型碼相關的條件表達式，可運用Replace Type Codewith Subclass (213)或Replace Type Code with State/Strategy (227)加以處理。

如果你有一組應該總是被放在一起的字段，可運用 Extract Class(149)。如果你在參數(shù)列中看到基本型數(shù)據(jù)，不妨試試 IntroduceParameter Object (295)。如果你發(fā)現(xiàn)自己正從數(shù)組中挑選數(shù)據(jù)，可運用 Replace Array with Object (186)。

給我們的啟示主要有兩點：

• 大部分業(yè)務場景和語言環(huán)境下，結構化類型導致的開銷基本可以忽略
• 結構化類型帶來更清晰的語義和復用

CASE：

反例

@Data
public class XxxConfigDTO implements Serializable {
  private static final long serialVersionUID = 8018480763009740953L;
  /**
   * 租戶ID
   */
  private Long   tenantId;
  /**
   * 工商稅務企業(yè)類型
   */
  private String companyType;
  /**
   * 企業(yè)名稱
   */
  private String companyName;
  /**
   * 企業(yè)納稅人識別號
   */
  private String companyTaxNo;
  /**
   * 審單員工工號
   */
  private String auditEmpNo;
  /**
   * 審單員工姓名
   */
  private String auditEmpName;
  /**
   * 跟單員工工號
   */
  private String trackEmpNo;
  /**
   * 跟單員工姓名
   */
  private String trackEmpName;
}

正例

@Data
public class XxxConfigDTO2 implements Serializable {
  private static final long serialVersionUID = 8018480763009740953L;
  /**
   * 租戶ID
   */
  private Long   tenantId;
  /**
   * 企業(yè)信息
   */
  private Company company;
  /**
   * 審單員工信息
   */
  private Employee auditEmployee;
  /**
   * 跟單員工信息
   */
  private Employee trackEmployee;
}
@Data
public class Company {
  /**
   * 工商稅務企業(yè)類型
   */
  private String companyType;
  /**
   * 企業(yè)名稱
   */
  private String companyName;
  /**
   * 企業(yè)納稅人識別號
   */
  private String companyTaxNo;
}
@Data
public class Employee {
  /**
   * 員工工號
   */
  private String empNo;
  /**
   * 員工姓名
   */
  private String empName;
}

其實就是怎么去抽象，對于特定領域的對象可以參考 DDD 里面的 Domain Primitive（DP）。

5.3 分布式鎖

5.3.1 未處理鎖失敗

private void process(String orderId) {
    // do validate
    try {
        boolean lockSuccess = lockService.tryLock(LockBizType.ORDER, orderId);
        if (!lockSuccess) {
            // TODO 此處需要處理鎖失敗，重試或拋出異常
            return;
        }
        // do something
    } finally {
        lockService.unlock(LockBizType.ORDER, orderId);
    }
}

分布式鎖的目的是為了防止并發(fā)沖突和保證數(shù)據(jù)一致性，鎖失敗時未處理直接返回，會帶來非預期結果的影響，除非明確失敗可放棄。

5.3.2 手寫解鎖容易遺漏

上面的加鎖和解鎖都是手動編寫，而這兩個動作一般是成對出現(xiàn)的，在手動編寫時容易發(fā)生遺漏解鎖而導致線上問題，推薦封裝一個加解鎖的方法來實現(xiàn)，會更加安全和便利。

private void procoess(String orderId) {
    // do validate
    Boolean processSuccess = lockService.executeWithLock(LockBizType.ORDER, orderId, () -> doProcess(orderId));
    // do something
}
private Boolean doProcess(String orderId) {
    // do something
    return Boolean.TRUE;
}
// LockService
public <T> T executeWithLock(LockBizType bizType, String bizId, Supplier<T> supplier) {
    return executeWithLock(bizType, bizId, 60, 3, supplier);
}
public <T> T execteWithLock(LockBizType bizType, String bizId, int expireSeconds, int retryTimes, Supplier<T> supplier) {
    // 嘗試加鎖
    int lockTimes = 1;
    boolean lock = tryLock(bizType, bizId, expireSeconds);
    while(lockTimes < retryTimes && !lock) {
        try {
            Thread.sleep(10);
        } catch (Exception e) {
            // do something
        }
        lock = tryLock(bizType, bizId, expireSeconds);
        lockTimes++;
    }
    // 鎖失敗拋異常
    if (!lock) {
        throw new LockException("try lock fail");
    }
    // 解鎖
    try {
        return supplier.get();
    } finally {
        unlock(bizType, bizId);
    }
}

5.3.3 加鎖 KEY 無效

private void process(String orderId) {
    // do validate
    try {
        // 此處加鎖類型與加鎖KEY不匹配
        boolean lockSuccess = lockService.tryLock(LockBizType.PRODUCT, orderId);
        if (!lockSuccess) {
            // TODO 重試或拋出異常
            return;
        }
        // do something
    } finally {
        lockService.unlock(LockBizType.PRODUCT, orderId);
    }
}

注意加鎖類型與加鎖 KEY 在同一個維度，否則加鎖會失效。

5.4 分頁查詢

5.4.1 完全沒有分頁

反例

private List<OrderDTO> queryOrderList(Long customerId) {
    if (customerId == null) {
        return Lists.newArrayList();
    }
    List<OrderDO> orderDOList = orderMapper.list(customerId);
    return orderConverter.doList2dtoList(orderDOList);
}

正例

private Page<OrderDTO> queryOrderList(OrderPageQuery query) {
    Preconditions.checkNotNull(query, "查詢條件不能為空");
    Preconditions.checkArgument(query.getPageSize() <= MAX_PAGE_SIZE, "分頁size不能大于" + MAX_PAGE_SIZE);
    // 分頁size一般由前端傳入
    // query.setPageSize(20);
    long cnt = orderMapper.count(query);
    if (cnt == 0) {
        return PageQueryUtil.buildPageData(query, null, cnt);
    }
    List<OrderDO> orderDOList = orderMapper.list(query);
    List<OrderDTO> orderDTOList = orderConverter.doList2dtoList(orderDOList);
    return PageQueryUtil.buildPageData(query, orderDTOList, cnt);
}

沒有分頁的列表查詢對 DB 性能影響非常大，特別是在項目初期，因為數(shù)據(jù)量非常小問題不明顯，而導致沒有及時發(fā)現(xiàn)，會給未來留坑。

5.4.2 分頁 size 太大

反例

private Page<OrderDTO> queryOrderList2(OrderPageQuery query) {
    Preconditions.checkNotNull(query, "查詢條件不能為空");
    query.setPageSize(10000);
    long cnt = orderMapper.count(query);
    if (cnt == 0) {
        return PageQueryUtil.buildPageData(query, null, cnt);
    }
    List<OrderDO> orderDOList = orderMapper.list(query);
    List<OrderDTO> orderDTOList = orderConverter.doList2dtoList(orderDOList);
    return PageQueryUtil.buildPageData(query, orderDTOList, cnt);
}

分頁 size 的大小并沒有一個固定的標準，取決于業(yè)務需求、數(shù)據(jù)量及數(shù)據(jù)庫等，但動輒幾千上萬的分頁 size，會帶來性能瓶頸，而大量的慢 SQL 不但影響客戶體驗，對系統(tǒng)穩(wěn)定性也是極大的隱患。

5.4.3 超多分頁慢 SQL

反例

<!-- 分頁查詢訂單列表 -->
<select id="list" parameterType="com.xxx.OrderPageQuery" resultType="com.xxx.OrderDO">
    SELECT
        <include refid="all_columns"/>
    FROM t_order
        <include refid="listConditions"/>
    ORDER BY id DESC
    LIMIT #{offset},#{pageSize}
</select>

正例

<!-- 分頁查詢訂單列表 -->
<select id="list" parameterType="com.xxx.OrderPageQuery" resultType="com.xxx.OrderDO">
    SELECT
        <include refid="all_columns"/>
    FROM t_order a
    INNER JOIN (
        SELECT id AS bid
        FROM t_order
            <include refid="listConditions"/>
        ORDER BY id DESC
        LIMIT #{offset},#{pageSize}
    ) b ON a.id = b.bid
</select>

以上 bad case 的 SQL 在超多頁分頁查詢時性能極其低下，存在多次回表甚至 Using Filesort 的問題，在阿里巴巴編碼規(guī)范中也有明確的規(guī)避方案，此處不展開。

最后，我們工程師的智慧結晶都盡在代碼之中，而 Code Review 可以促進結晶更加清瑩通透、純潔無瑕、精致完美，值得大家一起持續(xù)精進！

以上就是Code Review 方法論與實踐總結梳理的詳細內(nèi)容，更多關于Code Review 方法論與實踐總結的資料請關注腳本之家其它相關文章！

最新評論