一文梳理Code Review方法論與實踐總結-有解無憂

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

一、為什么要CR

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

二、他山之石

2.1 某大廠A

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

2.1.1 代碼評審準則

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

2.1.2 代碼評審原則

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

2.1.3 代碼審核者應該看什么

設計：代碼是否設計良好？這種設計是否適合當前系統？
功能：代碼實作的行為與作者的期望是否相符？代碼實作的互動界面是否對用戶友好？
復雜性：代碼可以更簡單嗎？如果將來有其他開發者使用這段代碼，他能很快理解嗎？
測驗：這段代碼是否有正確的、設計良好的自動化測驗？
命名：在為變數、類名、方法等命名時，開發者使用的名稱是否清晰易懂？
注釋：所有的注釋是否都一目了然？
代碼樣式：所有的代碼是否都遵循代碼樣式？
檔案：開發者是否同時更新了相關檔案？

2.2 某大廠B

在開發流程上專門有這個環節，排期會明確排進日程，比如5天開發會排2天來做代碼審核，分為代碼自審、交叉審核、集中審核，
有明確的量化指標，如8人時審核/每千行代碼，8個以上非提示性有效問題/每千行代碼，

2.3 某大廠C

推行Code Owner機制，每個代碼變更必須有Code Owner審核通過才可以提交，
所有的一線工程師，無論職級高低，最重要的工程輸出原則是“show me the code”，而Code Review是最能夠反應這個客觀輸出的，
盡量讓每個人的Code Review參與狀況都公開透明，每個變更發送給專案合作者，及轉發到小組內成員，小組內任何人都可以去Review其他人的代碼，
明確每個人的考評和Code Review表現相關，包括Code Review輸出狀況及提交代碼的質量等，

三、我們怎么做CR

3.1 作為代碼提交者

發起時機：發起Code Review盡量提前，開發程序小步快跑

代碼行數：提交Code Review的代碼行數最好在400行以下，根據資料分析發現，從代碼行數來看，超過400行的CR，缺陷發現率會急劇下降；從CR速度來看，超過500行/小時后，Review質量也會大大降低，一個高質量的CR最好控制在一個小時以內，
明確意圖：撰寫語意明確的標題（必填）和描述（選填，可以包括背景、思路、改造點和影響面、風險等）
善用工具：IDEA打開編碼規約實時檢測，減少代碼樣式、編碼規約等基礎性問題
（阿里編碼規約插件：https://github.com/alibaba/p3c/tree/master/idea-plugin）

3.2 作為代碼評審者

3.2.1 評審范圍

主要從兩方面來評審：

代碼邏輯

功能完整：代碼實作是否滿足功能需求，實作上有沒有需求的理解偏差，對用戶是否友好；
邏輯設計：是否考慮了全域設計和兼容現有業務細節，是否考慮邊界條件和并發控制；
安全隱患：是否存在資料安全隱患及敏感資訊泄漏，如越權、SQL注入、CSRF、敏感資訊未脫敏等；
性能隱患：是否存在損害性能的隱患，如死鎖、死回圈、FullGC、慢SQL、快取資料熱點等；
測驗用例：單元測驗用例的驗證邏輯是否有效，測驗用例的代碼行覆寫率和分支覆寫率；

代碼質量

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

3.2.2 評審注意事項

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

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

四、CR怎么避免流于形式

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

不認同CodeReview

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

CodeReview后改動太大

發布前發現問題多，改動太大，影響專案計劃
大專案要求編碼前設計評審，小需求可以事先Review設計思路，避免最后的驚喜
每次Review的代碼行數最好控制在數百行以內

評審者沒有足夠時間

評審者在任務安排上盡量預留好時間
盡快評審，代碼在百行以內及時回應，在千行以內當日完結

評審者不了解業務和代碼

代碼提交人撰寫清晰的標題和描述
有必要的情況下評審者需要了解PRD
評審者需要提前了解系統和代碼

Review建議未修改

這一點極為重要，需要對修改后的代碼再次Review，確保理解一致，以及預防帶問題上線
應用可以設定Review建議需全部解決的卡點，同時對于非必需修改的建議可以進行打標或說明

五、CR實踐中發現的幾個常見代碼問題

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

5.1 DRY

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

5.1.1 代碼重復

《重構》中對“Duplicated Code（重復代碼）”的描述：

壞味道行列中首當其沖的就是Duplicated Code，如果你在一個以上的地點看到相同的程式結構，那么可以肯定：設法將它們合而為一，程式會變得更好，

最單純的Duplicated Code就是“同一個類的兩個函式含有相同的運算式”，這時候你需要做的就是采用Extract Method (110)提煉出重復的代碼，然后讓這兩個地點都呼叫被提煉出來的那一段代碼，

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

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

代碼重復的幾種場景：

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

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，破壞了內聚性，實踐中需要平衡復用與內聚，

5.2 Primitive Obsession

《重構》中對“Primitive Obsession（基本型別偏執）”的描述：

大多數編程環境都有兩種資料：結構型別允許你將資料組織成有意義的形式；基本型別則是構成結構型別的積木塊，結構總是會帶來一定的額外開銷，它們可能代表著資料庫中的表，如果只為做一兩件事而創建結構型別也可能顯得太麻煩，

物件的一個極大的價值在于：它們模糊（甚至打破）了橫亙于基本資料和體積較大的類之間的界限，你可以輕松撰寫出一些與語言內置（基本）型別無異的小型類，例如，Java就以基本型別表示數值，而以類表示字串和日期——這兩個型別在其他許多編程環境中都以基本型別表現，

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

如果你有一組應該總是被放在一起的欄位，可運用Extract Class(149)，如果你在引數列中看到基本型資料，不妨試試IntroduceParameter Object (295)，如果你發現自己正從陣列中挑選資料，可運用Replace Array with Object (186)，

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

大部分業務場景和語言環境下，結構化型別導致的開銷基本可以忽略
結構化型別帶來更清晰的語意和復用

CASE：

反例

@Data
public class XxxConfigDTO implements Serializable {

  private static final long serialVersionUID = 8018480763009740953L;

  /**
   * 租戶ID
   */
  private Long   tenantId;
  /**
   * 工商稅務企業型別
   */
  private String companyType;
  /**
   * 企業名稱
   */
  private String companyName;
  /**
   * 企業納稅人識別號
   */
  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;
  /**
   * 企業資訊
   */
  private Company company;
  /**
   * 審單員工資訊
   */
  private Employee auditEmployee;
  /**
   * 跟單員工資訊
   */
  private Employee trackEmployee;

}

@Data
public class Company {
  /**
   * 工商稅務企業型別
   */
  private String companyType;
  /**
   * 企業名稱
   */
  private String companyName;
  /**
   * 企業納稅人識別號
   */
  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);
    }
}

分布式鎖的目的是為了防止并發沖突和保證資料一致性，鎖失敗時未處理直接回傳，會帶來非預期結果的影響，除非明確失敗可放棄，

5.3.2 手寫解鎖容易遺漏

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

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性能影響非常大，特別是在專案初期，因為資料量非常小問題不明顯，而導致沒有及時發現，會給未來留坑，

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的大小并沒有一個固定的標準，取決于業務需求、資料量及資料庫等，但動輒幾千上萬的分頁size，會帶來性能瓶頸，而大量的慢SQL不但影響客戶體驗，對系統穩定性也是極大的隱患，

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的問題，在阿里巴巴編碼規范中也有明確的規避方案，此處不展開，

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

作者 | 方基成(潤甫)

本文來自博客園，作者：古道輕風，轉載請注明原文鏈接：https://www.cnblogs.com/88223100/p/A-summary-of-Code-Review-methodology-and-practice.html

轉載請註明出處，本文鏈接：https://www.uj5u.com/gongcheng/541527.html

標籤：其他

上一篇：一文梳理Code Review方法論與實踐總結

下一篇：辛苦加班了一年，為何年終獎比同級同事少？