【如何有效做Code Review】8行代碼提出的21個問題

8行代碼的21問題

1. 如何有效的做CR?

很多同學都有這個疑問，如何結構化體系化的做CR？如何綜合應用各種手段盡快及早的發現代碼問題和缺陷？

下面圍繞這個執行個體，抛磚引玉，大家可以一起探讨；

1.1 CR執行個體：8行代碼21問題

執行個體如下 ，短短8行代碼，通過CR可以發現多少問題呢？21處；這段代碼誰寫的不重要，探讨的重點是如何全面發現其中的問題和隐患；

1.2 CR執行個體：如何結構化的CR?

對這段代碼，從背景了解，邏輯分析，異常分析，編碼規範，非功能點，可測性來分析代碼問題：

1.2.1. 背景了解

思考問題：代碼塊是要幹什麼？從上下文，方法簽名和代碼結構上快速看下代碼：

• Q1: 注釋中，Line94，嗲嗎出現音近拼寫錯誤
• Q2: 注釋中，Line93（非dev），Line94（生産），Line98（非dev，非test）不是很統一，應該是生産環境要保障是https
• Q3: 注釋中，Line94，為啥生産中拿不到正确的https, 原因是什麼，什麼情況觸發，多大機率

生産環境可能拿不到正确的schema, 是個異常的CASE處理

其中注釋方法命名，出入參數沒清晰表達場景

1.2.2. 邏輯分析

思考問題：代碼究竟幹了什麼？選自己熟悉或則感興趣的點開始：

• Q4: Line98，dbmode {String} dbmode： [dev test, stable, prod,...]為啥隻考慮dev和test?
• Q5: Line98，代碼中是逆向判斷，不符合條件（線上）會有很多，為啥不直接過濾PROD， if(PROD) 比較好了解，簡單；同時case數量也減少很多
• Q6: Line100，http替換成https，但是中間端口沒有替換，是否存在此端口未開通https的情況

代碼做了什麼清楚多了

1.2.3. 異常分析

思考問題：代碼幹不了什麼？真實的環境複雜，資料多樣，看看會那些情況：

• Q7: Line98，dev，test都是小寫，是否大寫就不能比對， 要是有枚舉的可以統一處理
• Q8: Line98，dbMode沒做判空，如果dbMode為空，則會認為是生産環境
• Q9: Line99，httpGetRequest.getUri().xxx()可能報空指針異常
• Q10: Line99，uri.startsWith("http://") 需要考慮前面有空格的情況
• Q11: Line99，大寫的 HTTP:// 是否也需要可也要比對處理？

現實中會出現這麼多異常麼，代碼執行環境，輸入最終是什麼樣的？

1.2.4. 程式設計規範分析

思考問題：代碼怎麼幹最标準？各種情況邏輯情況有了，再看下能否最佳姿勢表達：

• Q12: Line98，記得判斷環境，有直接的函數的，不用自己寫？ 之前主站有對這塊治理過，是否可重用
• Q13: Line98, equals方法比對對象寫在左邊
• Q14: Line98，if (!(StringUtil.equals(dbMode, "dev") || StringUtil.equals(dbMode, "test"))) if 中包含比較複雜判斷，應該用變量替換例如:isSitEnv
• Q15: Line99, httpGetRequest.getUri()調用了三次，存儲到變量中可以少調用兩次

集團和螞蟻都有程式設計規範，代碼送出前可以自行掃描一下，部分代碼門禁也有接入

代碼中多處字面量（dev, test, http, https)，需要常量化，dbmode适合枚舉化

1.2.5. 非功能點分析

思考問題：代碼有什麼影響？跳出來再思考，代碼雖小，也需要看下運作中鍊路，檢查性能，穩定性等點：

• Q16: Line100, replaceFirst("^http", "https"); 使用正則，比對比簡單字元操作要耗時
• Q17: Line100, replaceFirst中，每次都會new*complie Pattern，大量命中存在不必要的重複compile pattern時間
• Q18: Line100, 從comment中看出這個一個異常情況，線上如果觸發了，需要一種方式能感覺，例如增加列印日志并需要配置監控，目前沒有日志無法感覺，也無法調查到root cause

發現功能之外情況，同時需要考慮三闆斧

1.2.6. 可測試行分析

思考問題：代碼怎麼覆寫又快又好？

• Q19: Line100, 由于限定了環境，線上下環境沒發功能測試進行覆寫，最好單測覆寫最簡單
• Q20: Line100, 線上觸發時也無日志列印，不具備可觀察性；
• Q21: Line100, 如果不加日志，在驗收或則開量時進行double check 出入參的正确性（或驗收用例）

2. 抽象1步，如何複制和複用？

這裡共21個問題，其實每個問題都能對應到快速check list中

可以看出：異常情形，程式設計規範，功能邏輯和注釋這塊問題比較多，非功能性和可測性問題也存在

2.1 問題分類

2.1.1 需求和上下文

每個Reviewer都會關注的地方，好的注釋，代碼結構很容易人了解：

• Q3: Line94，為啥生産中拿不到正确的https, 原因是什麼，什麼情況，多大機率
• 這個是需求，是提前，為啥要做這個事情。在CR時需要需要了解，特别是對口的開發測試同學，否則發現不了基本的問題

2.1.2 代碼邏輯

不管是否熟悉業務，都能發現邏輯問題：

• 這個是異常資料，是否真的存在，存在了有什麼影響

2.1.3 代碼規範

這種問題可能太多了，經驗和方法也有很多。Follow最佳實踐和慣例，可以提高可讀性和避免采坑:

• 這個是程式設計規範問題，字面值常量化（枚舉化），複雜判斷，代碼可讀性,

2.1.4 業務邏輯

業務背景知識對實作的完整性至關重要:

• Q4: Line98，dbmode {String} dbmode： [dev, sit, stable, gray, prod,...]為啥隻考慮dev和test?
• 這個是常見錯誤，忽視了客觀資料；很多異常case都是由此産生的

2.1.5 設計合理

合理的設計，要結合上線文來看，會有對比和取舍：

• 這個是設計問題（性能考量），string的replaceFirst調用正則，一次compile沒問題，也邏輯很清楚
• 如果大量請求，正則重複compile，則有不必要重複耗時計算，這裡主要識别replaceFirst的實作機制，根據上下文來選擇

2.1.6 非功能

需要跳出功能點來看，從靜态的代碼，看到品質的更多面：

• 大家能看到有很多防禦性代碼，避免了線上的報錯，是大家普遍接受的。過多防禦代碼，讓系統逐漸減少對異常感覺能力，進而隐藏了真正的問題。
• 另外防禦性代碼對今後的維護性造成了隐患，不清楚是否能改造。

2.1.7 可測性

可測性是測試同學，也是開發需要考慮的點：

• 這個是可測性的問題，測試同學在CR時需要考慮這塊，考慮通過什麼手段來覆寫；很多測試不一定要從內建後進行功能測試；CR和單測是很好的方法。

2.2 結構化CR

上述發現的問題和我們CR的方式很有關系，每個人進入CR的時機不一樣，關注點也不一樣。下面是從CR的簡單流程，每個人CR時都能從中找一個切入點，根據經驗和已有規則，能提出自己問題。

2.3 CR常見規則

下圖是一個Expert Code Reviewer的想法，正常大家在Code Review 可能不會考慮這麼多和全面，哪些可以幫助快速有效CR發現問題。根據之前出現的問題，下面列出最常見check點，幫助大家有效CR。

2.3.1 可讀性

• 評論是否清晰有用？變量、類、方法的命名是否明确，可辨識
• 代碼可讀性，将來其他同學能輕松了解并使用此代碼
• 阿裡巴巴集團JAVA代碼規範-基礎規約可讀性部分

這些規範可由門禁代碼掃描[靜态掃描規則庫]發現部分，可适當關注。

• 注釋規約
• 常量定義
• 格式規約
• 命名規約

2.3.2 代碼邏輯

• 代碼是否實作DEV的意圖
• 邊界問題，如數組越界，擷取object key，業務邏輯的邊界（沒有的情況、全等、全null、全undefined等）
• 資源洩露（記憶體洩露，檔案句柄未釋放，網絡端口占用等）
• 算術計算溢出（指派截斷，精度提升，移位，類型轉化等）
• 線程&并發處理（ThreadLocal退出清理，一鎖二判三更新四釋放等）
• 異常資料處理(資料約定，長度，類型，預設值等）
• 工具使用姿勢是否正确（了解工具背後邏輯)
• 工具常見問題，如map.put key=null報錯，switch(param=null)報錯

2.3.2 代碼規範

• 阿裡巴巴集團JAVA代碼規範-基礎規約
  • 控制語句
  • OOP規約
  • 集合處理
  • 并發處理
• 其他平台語言開發參考：
  • 集團iOS開發規約
  • 集團Android開發規約
  • 集團前端開發規約
  • 集團C++規約
• 是否follow業界Best Practice Oracel Java Language Best Practices

2.3.3 業務邏輯

• 功能實作是否完整性
• 是否遺漏關聯場景
• 異常情形處理
• 對依賴方實作，架構和規劃有了解

2.3.4 設計合理性

• 明确真實的業務需求和場景，真實環境中怎麼使用的
• 是否基于未确定的假設
• 是否重複造輪子，有沒有漏洞和留坑，有限制
• 是否和架構，系分比對，能否滿足業務需求
• 是否follow常見設計規約，如阿裡巴巴設計規約,  螞蟻金服DB設計規範
  • 異常日志
  • MySQL規約
  • OceanBase
  • 螞蟻中間件
  • 螞蟻二方庫
  • 螞蟻安全
  • 伺服器規約
• 國際化設計follow國際際化規約，其實個人認為所有應用預設就應follow
• 錯誤碼設計
• 常見性能（是否有更優操作方法，是否有重複耗時任務， 在大量請求下是否觸發性能問題）
• 防錯設計

2.3.5 非功能

常見的點，但不限于

1. 穩定性，上下遊鍊路穩定性

• 相容性（代碼相容性， 資料相容性，協定相容性）
• 系統依賴（高等級穩定依賴低等級系統）
• 資料庫和緩存（SQL性能，是否能走到索引，緩存多入口重新整理，失敗補償，緩存&持久化不一緻）
• 流量和性能（服務重試+長耗時，下遊服務負載過重，自身限流）
• 資金安全（前端不進行金額計算，幂等字段，并發流水更新）
• 服務隔離（是否一個失敗是否會觸發整體失敗）

2. 安全性

• 垂直權限和水準權限
• 資料脫敏和防洩漏（不限于日志，前端傳回）
• 使用者請求有效校驗
• 用戶端安全請參考：
  • 螞蟻金服h5安全開發規範
  • 螞蟻金服android應用安全開發規範
  • 螞蟻金服IOS應用安全開發規範    

3. 可診斷性

• 關鍵路徑是否有日志
• 日志資訊是否可排查，能否各端串聯起來，如：線程加上名稱等，異常時列印資訊是否可用于複原場景

4. 可監控，可灰階，可應急

• 關鍵業務有日志埋點用于監控，并發現問題
• 該功能可灰階方式和政策
• 資料變更是否可灰階，怎麼灰階的
• 是否需要和有應急能力

5. 可測性

• 是否可測, 是否滿足可觀察性，可操作性，可自動化等
• 什麼方式覆寫和難點，CR覆寫，單測覆寫，功能，還是專項，是否需要額外可測性改造
• 單元測試規約，很多點對功能自動化也使用

3 再抽象1步，如何系統性預防和發現暴露出的問題？

3.1 問題背後的問題

為什麼會有這麼問題呢？問題多屬于注釋，異常情形，程式設計規範，功能邏輯；下面具體的原因分析： 

這裡有人的經驗，有方法，複雜環境和基礎設施原因，都可以落到這個九宮格中。

正确的結果	不正确結果
我知道我知道 已知正确前提	這個是期望 經驗總結 - 複制經驗規則方法 - 經驗沉澱到技術	原因：人的意識，人的失誤 怎麼解決？ - 自省和練習 - 他查 - 技術發現
我知道我不知道 已知錯誤前提	僥幸	原因：缺乏專業精神 - 自身嘗試和經驗積累 - 他人經驗規則方法 - 技術發現問題 - 資料度量
我不知道我不知道 未知前提	運氣不錯	- 發現未知（AI和資料）

究竟怎麼防範和發現問題呢？人員經驗可以積累，方法可以沉澱和傳授和基礎設施（技術，資料，AI)可以逐漸建設，下面說下自己的想法：

3.2 人員

• 提升品質意識，全面認識品質；
  • 如代碼複雜度高，可讀性，測試成本也高；發現治理根因比問題防範更重要等
  • 主動通過持續技術營運，技術分享等來彌補技術不足和積累經驗
  • 在持續 Code Review中學習，類似的問題就會越少
  • 意識

建議參考 The Clean Coder: A Code of Conduct for Professional Programmers (Robert C. Martin Series)

3.3 方法

• 養成良好CR方式，送出CR 時double check一次加強自檢，對Author和Reviewer來說同樣重要：
  • 具體可參考 Google的CR最佳實踐 （ 英文版 ）可以組織CR學習
• 設計方法
  • 不少假設和前提需要确定，同時需要資料和工具來支援，在正确前提下做設計，防範
  • 設計和實作方式，很多問題都有共性，或有存在的解，開發者和Reviewer都需要持續積累
  • 參考标準集團開發規約和業界最佳實踐
• 持續品質營運
  • 持續補充代碼規範和Best Practice到掃描工具中并分享
  • 經典或常見問題了解，如國際daily quiz 和daily bug

3.4 基礎設施（技術解法）