怎樣做一個合格的 CodeReview

CodeReview 是一件很需要耐心和技術功底的活動，也是一項需要全局觀的活動。怎樣做好一個合格的 CodeReview？

引子

CodeReview 是一件很需要耐心和技術功底的活動。

• 不了解業務，很容易淪為代碼風格檢查；
• 代碼量比較大，很容易走馬觀花，放過業務細節上的纰漏；
• 技術功底不夠，很容易放過性能或安全上有漏洞的代碼；
• 代碼細節不嚴格追究，似乎達不到 CR 的力度；嚴格追究，來回溝通好幾趟。
• 需要設定标準和底線。底線達不到，堅決不通過，不因任何問題而妥協。

最難的是，CodeReview 是一項需要全局觀的活動。哪怕僅有一行代碼改動，也需要在更大的業務語境裡去審查其是否恰當；如果有多行改動，恐怕要結合該業務的所有代碼去分析。因為，改動的代碼有可能單看上去沒有問題，但結合已有代碼及邏輯，就可能潛藏錯誤或漏洞。

BUG 無處不在。以人類現有的了解力和審查力，基本不可能避免大型軟體裡的 BUG 的産生（你無法看到看不到的 BUG）。能夠做到的是： 限制 BUG 産生的影響使之微乎其微。

必要前提

• 閱讀 CodeReview 相關的技術文檔及說明，熟悉相關業務邏輯及改動意圖；
• 代碼送出者保證遵循必要的代碼規範和風格；有極少量違反可以提醒，大量違反直接打回；
• 代碼送出者保證送出之前編譯通過，單測全部通過；
• 代碼送出者保證功能的自測通過；
• 代碼送出者自己先行 review 一遍，糾正低級錯誤；
• CodeReview 重點檢查必要規範、關鍵邏輯、品質層面。

兩個敏感素質

無論技術水準如何，業務熟悉度如何，CodeReview 的人應當具備兩個基本敏感素質：

• 影響面評估敏感。 對代碼涉及的影響面敏感，即使隻有一行代碼；
• 對代碼潛在問題敏感。看到一行代碼，能敏銳意識到可能存在的問題：NPE、健壯性、性能、可維護性、安全等。

常用檢查項

規範與風格

• 命名内容： 是否清晰直覺表達意圖；是否有粗略不貼切的問題；是否與項目約定一緻；
• 命名風格： 變量/方法名 - 駝峰式；常量 - 大寫+下劃線；（新手容易犯）
• 注釋： DO 屬性是否有加業務含義注釋；（建立團隊約定）
• 魔數： 使用常量定義；（情不自禁都會犯，要堅持零容忍）
• 拼寫錯誤： 要杜絕拼寫錯誤，尤其是兩個字元對調的情形（零容忍）；

邏輯層面

• 參數校驗： 參數格式、參數内容、參數合法性校驗（必須）；
• 異常：異常是否被捕獲并适當處理；（必須）
• 關鍵代碼： 要細讀，反複推敲；（必須）
• 模闆代碼： 是否有拼寫錯誤；（必須）
• 算法：是否有詳細說明、引用出處；（推薦）
• 流程：複雜流程是否有相應文檔說明；（推薦）
• 注釋：不明顯的邏輯、workaround 方案、臨時方案、很難用簡單單詞表達的複雜業務含義等要加注釋說明（建議）；

表達層面

• 擁擠的代碼：多行擠在一行最好能分拆為多行；（推薦）
• 嵌套 if-else: 是否能夠解開打平；是否可使用政策模式來分離；（推薦）
• 費解的代碼：了解起來有點繞彎，不夠直覺清晰地表達意圖；（建議）
• 建議的方式：可以給出更好的代碼實作建議；（建議）

品質層面

• NPE： NPE 無處不在，要嚴加防範；API 調用、資料庫查詢、反射擷取的對象、多級嵌套對象等傳回的對象務必要做判空檢查（必須）；
• 健壯性： 主要檢查越界、字元串解析異常；split, indexOf 等方法要注意越界異常；Json 解析要注意非法格式及非法資料校驗（必須）；
• 重複代碼： 重複代碼，是否可以抽離子函數或模闆處理（重複代碼敏感）；
• 可維護： 散落在流程裡的領域對象行為；過長的參數；過長的方法；方法内部修改入參等行為；
• 統一性： 同一個概念，是否建立了多個不同類來重複表達；
• 單測：是否有必要的單測；單測是否覆寫主要邏輯；
• 性能：是否有循環調用 API 或通路資料庫的做法； 是否面臨大資料量場景（是否有加索引措施、SQL 編寫是否适當等）；是否需要加緩存（本地還是分布式）；
• 并發： 是否有多線程通路的可能性；是否有不受控的線程或線程池建立；是否有加同步通路；同步手段是否恰當；并發性能是否能夠接；是否存在死鎖風險；
• 安全： 越權通路；敏感資訊明文；SQL 注入； 動态代碼執行是否有惡意參數校驗；
• 日志： 是否有必要的日志資訊（info, warn, error）；是否簡潔得當；異常是否捕獲并仔細列印了業務ID 相關的日志；

方法

• 可以先初步過一遍，檢查下是否明顯有以上問題，及時回報；
• 進一步再深入業務核心，去審查業務邏輯是否有問題；
• 将模闆代碼與關鍵代碼區分開；
• 關鍵代碼要仔細研讀；
• 可以在開發的停頓間隔之間進行；
• 建議至少在 IDE 裡審查一下改動，在 IDE 裡看更有感覺，在頁面上看由于缺失上下文語境，容易遺漏；
• 把程式運作起來，親自試一試，或許你會有一些和他們測試時不同的操作，發現一些他們遺漏的問題。

建議

• 對于一些比較緊急的改動會留下改進建議，但可以通過，通過後續代碼送出解決遺留的問題；
• 可以對Review的評論進行分級，不同級别的結果可以打上不同的Tag，比如[required]必須修改，[recommend] 推薦用法，[suggestion] 可選建議，[question]有疑問需要澄清；
• 可以考慮團隊統一安裝某個代碼檢查插件，送出之前先解決插件指出的問題，減少 CR 成本；
• 比較大的改動，可以分批送出，比如分成多個分支；
• 評論宜正面積極，給予建議而不是指責；
• 對于新手，更多是給予代碼規範上的建議；對于進階工程師，更多給出業務和品質層面的提醒；
• 不要吝啬你的贊揚；好的代碼也有必要拿出來讓大家一起學習。

參考文章

• 代碼問題及對策
• CodeReview實踐與總結
• 談談 Code Review
• Understanding Code Review
• 如何做好 CodeReview
• how-to-make-good-code-reviews-better
• 知乎問答：Code Review 都是怎麼做的？
• 7個 code review 的技巧
• PR的含義