1. 引言
Code Review(下文簡稱CR),即代碼審查,是一種通過評審代碼以發現并修正錯誤的實踐。它不是一個新概念,但在軟件開發中,它的重要性毋庸置疑。首先,它可以顯著降低軟件中的缺陷比例;其次,它促進了知識共享,通過評審的過程,團隊成員可以相互學習,增強對系統的整體理解;最后,CR是一種預防措施,它有助于維護代碼的清晰和統一,減輕技術債務,提升系統的穩定性。
盡管CR有諸多好處,實際操作中卻面臨不少挑戰。例如,交付壓力可能導致CR被忽視或流于形式;另一方面,缺乏有效技巧和工具支持,可能會使CR變得低效,甚至引發團隊內的沖突;此外,一些團隊可能會遇到參與度不足的問題,團隊成員不愿意投入必要的時間和精力。
在接下來的內容中,我們將探討如何克服這些挑戰,優化流程,并分享一些實戰經驗,以幫助讀者在自己團隊中實施有效的CR。
?
在此特別感謝JDL平臺技術部王鑫、劉建設、劉風、楊宏強、鞠萬奎等對本文撰寫的幫助。
?
2. Code Review的核心目標和基本原則
2.1 核心目標
首先,CR并不是走馬觀花,也并不需要面面俱到,我們先要明確以下幾個核心目標。
2.1.1 提高代碼質量
CR的首要目標是提高代碼質量。這包括識別缺陷、識別性能問題、確保代碼遵循一致的設計原則、提高代碼的可讀性和可維護性。
2.1.2 風險管理
CR的次要目標是發現潛在風險。通過CR盡早發現并解決潛在的代碼問題,以降低未來的修復成本,降低大型項目返工及上線失敗的風險。
2.1.3 促進知識共享
最后,通過CR促進團隊知識共享。CR過程鼓勵團隊成員之間的交流和協作,讓團隊成員相互學習對方的代碼和設計思路。這種交流有助于提高團隊的整體技能水平,同時減少代碼庫中知識的單點問題。
?
2.2 基本原則
對應CR的核心目標,遵循以下幾個基本原則有助于做好CR。
2.2.1 專注于代碼質量
CR的核心目的是提升代碼質量。這包括但不限于代碼的清晰性、可維護性、性能、安全性和可測試性等,在評審過程中應時刻專注于這些方面。
2.2.2 保持一致性的標準
遵循團隊或項目的編碼標準、風格指南和最佳實踐。CR應該確保代碼更改都符合這些標準,以便于團隊成員理解和維護代碼,保持一致性還有助于減少錯誤和提高代碼質量。
2.2.3 保持尊重/建設性溝通
溝通是CR過程中的核心元素。所有的反饋都應該是建設性的,目的是改進代碼而不是批評個人。作為評審者應針對代碼給出具體、有用的反饋,并在表達時考慮代碼作者的感受。
?
3. Code Review的實踐步驟與技巧
3.1 實踐步驟
CR的實踐步驟總體分為三步:準備、評審、修改及完成。
3.1.1 準備
在提交CR之前,應該先自行檢查代碼,以確保基本的代碼質量且遵循代碼規范。可以通過單元測試、靜態分析插件(例如SonarLint、JD EOS)、借助AI分析插件(例如Copilot、JD JoyCoder)等來完成。
如果更改較大,考慮將其分割成幾個小的、邏輯上獨立的commit。這樣不僅能使每次評審過程更高效,也便于追蹤和管理更改。
提交評審的時機,越早進行CR則修改的代價越小,至少應保證在提測前提交CR及完成修改。
最后,確定適合的評審者,建議選擇具有業務經驗及較為資深的研發人員。
3.1.2 執行評審
在評審過程中,聚焦在代碼質量方面(可參考下文提供的checklist)。控制好每次的時長,如果一次評審時間過長,則考慮是否應在準備階段就拆分成多次commit,進行多次評審,而不是在提測前進行一次大型評審。
3.1.3 修改及完成
開發者根據收到的反饋進行代碼調整,改動較大時可能會進行多次反復評審,當修改完成后,由具有權限的負責人將代碼合并至相應分支。
3.2 CR的最佳實踐技巧
遵循以下的最佳實踐技巧,有助于解決CR中遇到的各種問題,并保持高效。
3.2.1 有一份明確的checklist
每次評審時,評審者應該檢查哪些內容?對照一份明確的checklist,有助于我們專注于代碼質量,并保持一致性的標準。以下是一份可供參考的checklist。
?設計:主要評審整體設計,例如,API設計簡單清晰,代碼交互、系統交互恰當,技術組件、中間件使用得當等。
?功能性/非功能性:評審代碼的行為是否符合預期?大多數時候,僅靠評審并不能發現每一行代碼是否如期運行,我們應特別關注一些異常的極端情況,例如,邊界處理、異常死循環、非法的輸入輸出、大報文處理、兼容性問題、線程安全/并發問題、Exception處理等。
?性能/穩定性:對于一些高吞吐量的系統,響應性能尤其重要。例如,確保依賴服務SLA符合預期,超時和重試配置得當,避免產生慢SQL、大量鎖等待、線程阻塞/耗盡等。
?可觀測性:是否在上線后可觀測代碼運行的行為,發生異常時可及時感知?例如,確保方法添加了必要的監控埋點、有正確的日志級別及日志內容。
?復雜度:代碼實現足夠簡單嗎?是否有過度設計?作為評審者應讓代碼盡量保持簡潔,以便讓其他的開發者可以快速理解,降低未來修改時引入新錯誤的風險。
?命名:是否為變量、類、方法等選擇了清晰的名稱?命名應遵守代碼規范,且能夠準確表達代碼的意圖,而又不至于過長難以閱讀。
?注釋:注釋清晰無歧義,應解釋代碼“為什么”,而不是“是什么”。注釋更應解釋一些代碼外的隱含信息,例如,設計的取舍、業務背景、某些看起來很tricky的實現,以及解釋正則表達式、特定算法等內容。
?測試:是否有適當的單元測試?需要修改已有的單元測試?
?風格:是否遵循一致的代碼風格?風格無所謂好壞,但保持一致性的風格,會讓其他團隊成員更容易理解。
?文檔:是否需要更新相關API說明、Readme等文檔?
3.2.2 避免完美主義
在評審中發現問題固然重要,但也應結合實際約束及現狀進行權衡,并非所有代碼均要達到理論上的最優解及最佳實踐。只要這次修改讓代碼有所改善,或是向著正確的方向前進,那么代碼就是可以接受的。(調研報告顯示61%的CR沒有發現缺陷)
3.2.3 拆分為小型MR/PR/Commit
小型的changelist,擁有降低評審難度、縮短評審時間、減少引入錯誤的可能性、易于合并等諸多好處。通常認為將changelist控制在只解決一件事(可以只是feature的一部分),視作合適的大小。我們可以按層進行水平拆分、按功能進行垂直拆分,亦或是結合兩者,有興趣的讀者可以閱讀文章最后引用的google關于CR工程實踐文章。
3.2.4 一次不要評審過多的代碼
建議將每次評審的代碼控制在100~300行,最多不超過500行,每次評審時間不超過1.5小時(調研報告顯示超過這些閾值會導致CR質量及效率大幅降低)。不過根據實際場景不同,讀者可以根據代碼實際的復雜度進行調整。
3.2.5 盡早進行小而頻繁的評審
盡早評審有助于提前發現問題,減少后期修正的成本。編碼階段,在IDE環境安裝靜態代碼檢查工具,提前預先檢查代碼風格、格式等基本錯誤,可減少人工評審的工作量。面對大型代碼變更,將代碼分為更小而獨立的多次commit,盡早進行多次評審,也可提升評審質量,減少返工成本。
3.2.6 保持尊重
保持開放的心態,拋開自負,不要將個人偏好帶入到CR中。作為代碼審查者,應意識到代碼作者更了解其編寫的代碼,并不是每次評審都需要進行代碼調整。基于事實及代碼規范來提出改進建議,會使代碼作者更容易接受。作為代碼提交者,提交高質量的代碼,是對評審者和團隊最基本的尊重。保持開放的心態,將評審當做自我學習和提升的過程。
3.2.7 度量和改進
設定一些度量指標,并持續追蹤趨勢,有助于我們持續不斷改進CR過程。以下是一些可以用作度量的指標,例如,審查時長、缺陷密度、CR率等。
?
4. 案例分享
以下是身邊真實發生的一些CR案例,與3.2.1章節中的checklist都有相應的對照,供大家參考。為了便于閱讀,部分代碼進行了刪除簡化。
4.1 案例1-異常及并發情況處理不周
問題:靜態緩存先clear,再進行加載,如果解析過程異常會導致配置丟失、在高并發訪問時讀取到錯誤的配置。
改善:應使用覆蓋更新的方式。
public class ReverseSwitch { private static Map multiConfigAddress = new HashMap?>(); public void setMultiConfigAddress(String multiConfigAddress){ ReverseSwitch.multiConfigAddress.clear(); // 以下是解析字符串配置映射到Map配置中,省略具體過程 for (/*.....*/) { ReverseSwitch.multiConfigAddress.put(/*.....*/); } } public static boolean isMultiConfigSwitch() { // ..... } }
CR修改后:
public void setMultiConfigAddress(String multiConfigAddress){ log.info("ReverseSwitch.setMultiConfigAddress {}", multiConfigAddress); Map newAddress = new HashMap?>(); // 省略解析過程 for () { newAddress.put(); } // 使用覆蓋更新的方式 ReverseSwitch.multiConfigAddress = newAddress; }
4.2 案例2-設計問題、可觀測性不足
問題:1. 本地緩存每小時失效一次,會集中產生大量RPC請求加載數據(容器數量*外部請求數),當依賴的RPC服務抖動時有可能導致雪崩;2. do while語句在遠程數據異常時,可能循環次數超出預期或產生死循環,導致tp99超時、阻塞或OOM;3. 缺少必要的日志及監控埋點。
改善:1. 使用redis緩存并預加載;2. while內設置最大分頁次數進行break;3. 上層調用增加監控埋點及日志。(由于修改不止一處文件,未一一列出修改后的代碼)
@CacheMethod(key = "vrs.SpareQueryProxyCache.getAllSpareInfo", cacheBean = "localGuavaCacheBean60m", timeout = Constants.REDIS_KEY_TIMEOUT_MINUTES_60) public List getAllSpareInfo() { int pageNum = 0; PageDto page; List returnList = new LinkedList?>(); do { page = basicPrimaryWS.getBaseStoreInfoByPage(++pageNum); if (page != null && CollectionUtils.isNotEmpty(page.getData())) { // 省略對page內容進行篩選等邏輯處理代碼 // ...... returnList.addAll(page.getData()); } } while (page != null && page.getCurPage() < page.getTotalPage()); return returnList; }
4.3 案例3-代碼復雜度
問題:代碼不夠內聚,可讀性不好,開發追加需求時將多個校驗的邏輯寫到了校驗方法外。
改善:將校驗邏輯放到對應的校驗方法內,保持代碼整潔,降低理解難度。
public void buildWaybillCodeList(AfterSaleOrderReceiveContext afterSaleOrderContext) { boolean useServiceCode = true; // 條件1 if (condition_1) { useServiceCode = false; } // 其他條件 if (!canUseServiceCode(afterSaleOrderContext)) { useServiceCode = false; } // 條件2 if (condition_2) { useServiceCode = false; } List waybillCodeList = new ArrayList?>(); if (useServiceCode) { // 場景1:單號規則 waybillCodeList.add(WAYBILLCODE_PREFIX + afterSaleOrderContext.getAfterSaleOrderReceiveDTO().getServiceCode()); } else { // 場景2:單號規則 waybillCodeList.add(this.preDeliveryId(afterSaleOrderContext)); } // ...... } private boolean canUseServiceCode(AfterSaleOrderReceiveContext afterSaleOrderContext) { List productDetailDTOList = buildMainGiftProductList(afterSaleOrderContext); // 只針對一單一品一個數量的返回true return productDetailDTOList.size() == 1 && Objects.equals(productDetailDTOList.get(0).getProductCount(), 1); }
CR修改后:
public void buildWaybillCodeList(AfterSaleOrderReceiveContext afterSaleOrderContext) { List waybillCodeList = new ArrayList?>(); // 將多次需求變更的邏輯點聚合到職責明確的方法內 if (canUseServiceCode(afterSaleOrderContext)) { // 場景1:單號規則 waybillCodeList.add(WAYBILLCODE_PREFIX + afterSaleOrderContext.getAfterSaleOrderReceiveDTO().getServiceCode()); } else { // 場景2:單號規則 waybillCodeList.add(this.preDeliveryId(afterSaleOrderContext)); } // ...... } private boolean canUseServiceCode(AfterSaleOrderReceiveContext afterSaleOrderContext) { // 條件1 if (condition_1) { return false; } // 條件2 if (condition_2) { return false; } // 條件3 List productDetailDTOList = buildMainGiftProductList(afterSaleOrderContext); // 只針對一單一品一個數量的返回true return productDetailDTOList.size() == 1 && Objects.equals(productDetailDTOList.get(0).getProductCount(), 1); }
4.4 案例4-增加灰度策略控制
問題:CR過程中發現無法評估改動影響的業務范圍,如有問題可能會影響100%的流量。
改善:增加灰度策略開關。
public void setConsigneeAddress(WaybillAddress targetAddress) { // 100%修改至新邏輯:A // ...... }
CR修改后:
public void setConsigneeAddress(WaybillAddress targetAddress) { // 選取可控的特征屬性,例如按流量比例或商家切量 if (ThreadLocalRandom.current().nextInt(1000) < ducc.getAddressMontageSwitchRate()) { // 新邏輯:A,使用灰度策略,控制本次改動影響范圍 // ...... } else { // 原邏輯:B // ...... } }
4.5 案例5-善用工具
在提交人工CR前,善用工具進行代碼掃描,可能有意想不到的收獲。
對如下代碼使用JD JoyCoder進行自動評審建議。
if (response != null && response.getCode() != 0 && String.valueOf(response.getCode()).length() > 2 && (KK_PARAM_PREFIX_CODE.equals(String.valueOf(response.getCode()).substring(0, 2))) || KK_BIZ_PREFIX_CODE.equals(String.valueOf(response.getCode()).substring(0, 2))) { throw new BusinessException(StringUtils.isNotBlank(response.getSubMsg()) ? response.getSubMsg() : response.getMsg()); }
JD JoyCoder給出的評審意見:“代碼中存在一個潛在的邏輯錯誤,這是由于條件判斷中的邏輯運算符&&和||的優先級沒有被明確區分。在Java中,&&操作符的優先級高于||,這意味著在沒有適當的括號的情況下,&&綁定的條件會先被評估,然后才是||綁定的條件。”
修改后:
if (response != null && response.getCode() != 0 && String.valueOf(response.getCode()).length() > 2 && ((KK_PARAM_PREFIX_CODE.equals(String.valueOf(response.getCode()).substring(0, 2)) || KK_BIZ_PREFIX_CODE.equals(String.valueOf(response.getCode()).substring(0, 2)))) { throw new BusinessException(StringUtils.isNotBlank(response.getSubMsg()) ? response.getSubMsg() : response.getMsg()); } // 此段代碼可以進一步優化,將if里面的條件提前抽取到有明確業務語義的變量中,提升可讀性
除目前流行的基于LLM實現的AI掃描工具外,使用傳統代碼掃描也可以發現潛在問題。
以下代碼通過靜態掃描工具發現問題:直接使用“==”進行包裝類型Integer的比較,當遇到[-128, 127]范圍外時比較結果會不符合預期。
if (!(request.getSkuList().stream().allMatch( sku -> sku.getPreProduce() != null && sku.getPreProduce() == request.getSkuList().get(0).getPreProduce() ))) { throw new DOSException(ResultEnum.PRE_PRODUCE_UN_SAME.getCode(), ResultEnum.PRE_PRODUCE_UN_SAME.getMessage()); }
修改后:
if (!(request.getSkuList().stream().allMatch( sku -> sku.getPreProduce() != null && sku.getPreProduce().equals(request.getSkuList().get(0).getPreProduce()) ))) { throw new DOSException(ResultEnum.PRE_PRODUCE_UN_SAME.getCode(), ResultEnum.PRE_PRODUCE_UN_SAME.getMessage()); }
?
5. Code Review的成果收益
筆者所在團隊沒有單獨統計數據來佐證CR與線上缺陷的直接關聯。線上質量與CR、單元測試、質量測試、SRE等各方面息息相關,CR并非銀彈,但是做好CR非常有助于降低缺陷數量。
通過搜索公開數據顯示,行業中使用CR的項目,潛在缺陷發現率約在50%~60%之間,大部分的測試,潛在缺陷發現率約在30%左右。同時,數據顯示約75%的CR評審意見影響著軟件的可維護性/可演化性,這表明CR利于軟件系統的長期演化。
?
6. 總結與展望
本文探討了CR的重要性,它可以提前發現缺陷,有助于知識共享及團隊能力提升,同時分享了CR實踐步驟、技巧、案例等內容。當然,本文僅是一份參考指南,每個團隊根據其所處現狀的不同,可以根據本文調整優化各自的實踐流程。
如今,軟件開發的格局在不斷變化,圍繞CR的實踐也在不斷發展。隨著技術的進步,更智能的工具和 AI 輔助平臺在不斷涌現,這些工具能夠提供更高級的靜態分析、模式識別,甚至預測分析,在潛在問題出現之前識別它們。這種AI上下文感知的能力,將能夠根據項目特定的編碼風格、功能模塊以及依賴關系,提供針對性的CR反饋,甚至不再需要人工評審的介入。
未來,CR將繼續發揮其關鍵作用,我們期待AI+CR成為一個更加強大和智能的伙伴,使團隊將能夠保持競爭力,持續提升軟件質量和交付速度。
?
7. 參考資料
《Google Engineering Practices Documentation》:https://google.github.io/eng-practices/review/?
《Code Review at Cisco Systems》:https://static1.smartbear.co/support/media/resources/cc/book/code-review-cisco-case-study.pdf?
Wikipeida:https://en.wikipedia.org/wiki/Code_review?
?審核編輯 黃宇
-
Code
+關注
關注
0文章
68瀏覽量
15371 -
代碼
+關注
關注
30文章
4748瀏覽量
68356
發布評論請先 登錄
相關推薦
評論