阿里是如何做Code Review的?
一、為什么要CR
-
提前發(fā)現(xiàn)缺陷
在CodeReview階段發(fā)現(xiàn)的邏輯錯誤、業(yè)務理解偏差、性能隱患等時有發(fā)生，CR可以提前發(fā)現(xiàn)問題。 -
提高代碼質(zhì)量
主要體現(xiàn)在代碼健壯性、設(shè)計合理性、代碼優(yōu)雅性等方面，持續(xù)CodeReview可以提升團隊整體代碼質(zhì)量。 -
統(tǒng)一規(guī)范和風格
集團編碼規(guī)范自不必說，對于代碼風格要不要統(tǒng)一，可能會有不同的看法，個人觀點對于風格也不強求。但代碼其實不是寫給自己看的，是寫給下一任看的，就像經(jīng)常被調(diào)侃的“程序員不喜歡寫注釋，更不喜歡別人不寫注釋”，代碼風格的統(tǒng)一更有助于代碼的可讀性及繼任者的快速上手。 -
防止架構(gòu)腐爛
架構(gòu)的維護者是誰？僅靠架構(gòu)師或應用Owner是遠遠不夠的，需要所有成員的努力，所謂人人都是架構(gòu)師。架構(gòu)防腐最好前置在設(shè)計階段，但CodeReview作為對最終產(chǎn)出代碼的檢查，也算是最后一道關(guān)鍵工序。 -
知識分享
每一次CodeReview，都是一次知識的分享，磨合一定時間后，團隊成員間會你中有我、我中有你，集百家之所長，融百家之所思。同時，業(yè)務邏輯都在代碼中，團隊CodeReview也是一種新人業(yè)務細節(jié)學習的途徑。 -
團隊共識
通過多次討論與交流，逐步達成團隊共識，特別是對架構(gòu)理解和設(shè)計原則的認知，在共識的基礎(chǔ)上團隊也會更有凝聚力，特別是在較多新人加入時尤為重要。
二、他山之石
2.1 某大廠A
非常重視Code Review，基本上代碼需要至少有兩位以上Reviewer審核通過后，才會讓你Check In。
2.1.1 代碼評審準則
-
如果變更達到可以提升系統(tǒng)整體代碼質(zhì)量的程度，就可以讓它們通過，即使它們可能還不完美。這是所有代碼評審準則的最高原則。
-
世界上沒有“完美”的代碼，只有更好的代碼。評審者不應該要求代碼提交者在每個細節(jié)都寫得很完美。評審者應該做好修改時間與修改重要性之間的權(quán)衡。
2.1.2 代碼評審原則
-
以客觀的技術(shù)因素與數(shù)據(jù)為準，而非個人偏好。
-
在代碼樣式上，遵從代碼樣式指南，所有代碼都應與其保持一致，任何與代碼樣式指南不一致的觀點都是個人偏好。但如果某項代碼樣式在指南中未提及，那就接受作者的樣式。
-
任務涉及軟件設(shè)計的問題，都應取決于基本設(shè)計原則，而不應由個人喜好來決定。當同時有多種可行方案時，如果作者能證明（以數(shù)據(jù)或公認的軟件工程原理為依據(jù)）這些方案基本差不多，那就接受作者的選項；否則，應由標準的軟件設(shè)計原則為準。
-
如果沒有可用的規(guī)則，那么審核者應該讓作者與當前代碼庫保持一致，至少不會惡化代碼系統(tǒng)的質(zhì)量。（一旦惡化代碼質(zhì)量，就會帶來破窗效應，導致系統(tǒng)的代碼質(zhì)量逐漸下降）
2.1.3 代碼審核者應該看什么
-
設(shè)計 ：代碼是否設(shè)計良好？這種設(shè)計是否適合當前系統(tǒng)？
-
功能 ：代碼實現(xiàn)的行為與作者的期望是否相符？代碼實現(xiàn)的交互界面是否對用戶友好？
-
復雜性 ：代碼可以更簡單嗎？如果將來有其他開發(fā)者使用這段代碼，他能很快理解嗎？
-
測試 ：這段代碼是否有正確的、設(shè)計良好的自動化測試？
-
命名 ：在為變量、類名、方法等命名時，開發(fā)者使用的名稱是否清晰易懂？
-
注釋 ：所有的注釋是否都一目了然？
-
代碼樣式 ：所有的代碼是否都遵循代碼樣式？
-
文檔 ：開發(fā)者是否同時更新了相關(guān)文檔？
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ā)送給項目合作者，及轉(zhuǎn)發(fā)到小組內(nèi)成員，小組內(nèi)任何人都可以去Review其他人的代碼。
- 明確每個人的考評和Code Review表現(xiàn)相關(guān)，包括Code Review輸出狀況及提交代碼的質(zhì)量等。
三、我們怎么做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質(zhì)量也會大大降低，一個高質(zhì)量的CR最好控制在一個小時以內(nèi)。
-
明確意圖 ：編寫語義明確的標題（必填）和描述（選填，可以包括背景、思路、改造點和影響面、風險等）
-
善用工具
：IDEA打開編碼規(guī)約實時檢測，減少代碼樣式、編碼規(guī)約等基礎(chǔ)性問題
（阿里編碼規(guī)約插件： https://github.com/alibaba/p3c/tree/master/idea-plugin ）
3.2 作為代碼評審者
3.2.1 評審范圍
主要從兩方面來評審：
-
代碼邏輯
-
功能完整 ：代碼實現(xiàn)是否滿足功能需求，實現(xiàn)上有沒有需求的理解偏差，對用戶是否友好；
-
邏輯設(shè)計 ：是否考慮了全局設(shè)計和兼容現(xiàn)有業(yè)務細節(jié)，是否考慮邊界條件和并發(fā)控制；
-
安全隱患 ：是否存在數(shù)據(jù)安全隱患及敏感信息泄漏，如越權(quán)、SQL注入、CSRF、敏感信息未脫敏等；
-
性能隱患 ：是否存在損害性能的隱患，如死鎖、死循環(huán)、FullGC、慢SQL、緩存數(shù)據(jù)熱點等；
- 測試用例 ：單元測試用例的驗證邏輯是否有效，測試用例的代碼行覆蓋率和分支覆蓋率；
-
-
代碼質(zhì)量
-
編碼規(guī)范 ：命名、注釋、領(lǐng)域術(shù)語、架構(gòu)分層、日志打印、代碼樣式等是否符合規(guī)范
-
可讀性 ：是否邏輯清晰、易理解，避免使用奇淫巧技，避免過度拆分
-
簡潔性 ：是否有重復可簡化的復雜邏輯，代碼復雜度是否過高，符合KISS和DRY原則
-
可維護性 ：在可讀性和簡潔性基礎(chǔ)上，是否分層清晰、模塊化合理、高內(nèi)聚低耦合、遵從基本設(shè)計原則
-
可擴展性 ：是否僅僅是滿足一次性需求的代碼，是否有必要的前瞻性擴展設(shè)計
- 可測試性 ：代碼是否方便寫單元測試及分支覆蓋，是否便于自動化測試
-
3.2.2 評審注意事項
-
盡快完成評審
-
避免過度追求完美
-
明確評論是否要解決
- 避免使用反問句來評價
四、CR怎么避免流于形式
CR流于形式的因素很多，大概如下：
-
不認同CodeReview
-
評審者的姿態(tài)？有沒有帶來好處？有沒有從中收獲？這些都會直觀影響團隊成員的認可度
-
每個Review建議的提出都是一次思想交流，評論要友好、中肯、具體，避免教條式及負面詞匯，在遵守評審原則下，同時尊重個性展現(xiàn)
-
團隊集中CodeReview盡量不要太正式和嚴肅，輕松的氣氛下更有助于互相理解，來點水果，聊聊業(yè)務聊聊代碼
- 在Review過程有時候會陷入誰對誰錯的爭論，只要是為了尋求真理辯證的去看問題，哪怕是討論再激烈也是有收獲的，注意只對事不對人。
-
-
CodeReview后改動太大
-
發(fā)布前發(fā)現(xiàn)問題多，改動太大，影響項目計劃
-
大項目要求編碼前設(shè)計評審，小需求可以事先Review設(shè)計思路，避免最后的驚喜
- 每次Review的代碼行數(shù)最好控制在數(shù)百行以內(nèi)
-
-
評審者沒有足夠時間
-
評審者在任務安排上盡量預留好時間
- 盡快評審，代碼在百行以內(nèi)及時響應，在千行以內(nèi)當日完結(jié)
-
-
評審者不了解業(yè)務和代碼
-
代碼提交人編寫清晰的標題和描述
-
有必要的情況下評審者需要了解PRD
- 評審者需要提前了解系統(tǒng)和代碼
-
-
Review建議未修改
-
這一點極為重要，需要對修改后的代碼再次Review，確保理解一致，以及預防帶問題上線
- 應用可以設(shè)置Review建議需全部解決的卡點，同時對于非必需修改的建議可以進行打標或說明
-
五、CR實踐中發(fā)現(xiàn)的幾個常見代碼問題
筆者對個人CR評論問題做了個大概統(tǒng)計，Bug發(fā)現(xiàn)數(shù)占比約4%（直接或潛在Bug），重復代碼數(shù)占比約5%，其他還有規(guī)范、安全、性能、設(shè)計等問題。在CR代碼質(zhì)量時，可以參考《重構(gòu)：改善既有代碼的設(shè)計》，書中所列的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 代碼重復
《重構(gòu)》中對“Duplicated Code（重復代碼）”的描述： 壞味道行列中首當其沖的就是Duplicated Code。如果你在一個以上的地點看到相同的程序結(jié)構(gòu)，那么可以肯定：設(shè)法將它們合而為一，程序會變得更好。代碼重復的幾種場景：
最單純的Duplicated Code就是“同一個類的兩個函數(shù)含有相同的表達式”。這時候你需要做的就是采用Extract Method (110)提煉出重復的代碼，然后讓這兩個地點都調(diào)用被提煉出來的那一段代碼。
另一種常見情況就是“兩個互為兄弟的子類內(nèi)含相同表達式”。要避免這種情況，只需對兩個類都使用Extract Method (110)，然后再對被提煉出來的代碼使用Pull Up Method (332)，將它推入超類內(nèi)。如果代碼之間只是類似，并非完全相同，那么就得運用Extract Method (110)將相似部分和差異部分割開，構(gòu)成單獨一個函數(shù)。然后你可能發(fā)現(xiàn)可以運用Form Template Method (345)獲得一個Template Method設(shè)計模式。如果有些函數(shù)以不同的算法做相同的事，你可以選擇其中較清晰的一個，并使用Substitute Algorithm (139)將其他函數(shù)的算法替換掉。
如果兩個毫不相關(guān)的類出現(xiàn)Duplicated Code，你應該考慮對其中一個使用Extract Class (149)，將重復代碼提煉到一個獨立類中，然后在另一個類內(nèi)使用這個新類。但是，重復代碼所在的函數(shù)也可能的確只應該屬于某個類，另一個類只能調(diào)用它，抑或這個函數(shù)可能屬于第三個類，而另兩個類應該引用這第三個類。你必須決定這個函數(shù)放在哪兒最合適，并確保它被安置后就不會再在其他任何地方出現(xiàn)。
-
一個類中重復代碼抽象為一個方法
-
兩個子類間重復代碼抽象到父類
- 兩個不相關(guān)類間重復代碼抽象到第三個類
反例
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
《重構(gòu)》中對“Primitive Obsession（基本類型偏執(zhí)）”的描述： 大多數(shù)編程環(huán)境都有兩種數(shù)據(jù)：結(jié)構(gòu)類型允許你將數(shù)據(jù)組織成有意義的形式；基本類型則是構(gòu)成結(jié)構(gòu)類型的積木塊。結(jié)構(gòu)總是會帶來一定的額外開銷。它們可能代表著數(shù)據(jù)庫中的表，如果只為做一兩件事而創(chuàng)建結(jié)構(gòu)類型也可能顯得太麻煩。
對象的一個極大的價值在于：它們模糊（甚至打破）了橫亙于基本數(shù)據(jù)和體積較大的類之間的界限。你可以輕松編寫出一些與語言內(nèi)置（基本）類型無異的小型類。例如，Java就以基本類型表示數(shù)值，而以類表示字符串和日期——這兩個類型在其他許多編程環(huán)境中都以基本類型表現(xiàn)。
對象技術(shù)的新手通常不愿意在小任務上運用小對象——像是結(jié)合數(shù)值和幣種的money類、由一個起始值和一個結(jié)束值組成的range類、電話號碼或郵政編碼（ZIP)等的特殊字符串。你可以運用Replace Data Valuewith Object (175)將原本單獨存在的數(shù)據(jù)值替換為對象，從而走出傳統(tǒng)的洞窟，進入炙手可熱的對象世界。如果想要替換的數(shù)據(jù)值是類型碼，而它并不影響行為，則可以運用Replace Type Code with Class (218)將它換掉。如果你有與類型碼相關(guān)的條件表達式，可運用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)境下，結(jié)構(gòu)化類型導致的開銷基本可以忽略
- 結(jié)構(gòu)化類型帶來更清晰的語義和復用
反例
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;
}
正例
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;
}
public class Company {
/**
* 工商稅務企業(yè)類型
*/
private String companyType;
/**
* 企業(yè)名稱
*/
private String companyName;
/**
* 企業(yè)納稅人識別號
*/
private String companyTaxNo;
}
public class Employee {
/**
* 員工工號
*/
private String empNo;
/**
* 員工姓名
*/
private String empName;
}
其實就是怎么去抽象，對于特定領(lǐng)域的對象可以參考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ù)一致性，鎖失敗時未處理直接返回，會帶來非預期結(jié)果的影響，除非明確失敗可放棄。
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ī)避方案，此處不展開。
最后，我們工程師的智慧結(jié)晶都盡在代碼之中，而Code Review可以促進結(jié)晶更加清瑩通透、純潔無瑕、精致完美，值得大家一起持續(xù)精進！