CodeReview實踐與總結

如果一次CodeReview能夠檢測出代碼中的錯誤或設計的缺陷(即使是低級錯誤)，阻止上線後導緻BUG甚至故障，那麼就可以說此次CodeReview 是有效的、成功的。CodeReview的使命就是阻止有負面影響力的 BUG 或故障上線。本文詳細探讨了各種代碼問題：常見的代碼問題、可維護性問題、複雜并發問題、輕微問題，讨論CodeReview技巧，提出了代碼送出建議标準，并附錄了具體的檢查點清單。

CodeReview 是大型軟體工程中公認的必不可少的保證工程品質的重要手段之一。但凡正規軟體作戰軍團都是非常重視 CodeReview 的作用和意義的。那麼，如何做好 CodeReview 呢？這裡總結下自己的學習筆記和實踐心得。

CodeReview有效性

對于 CodeReview 該做什麼和不該做什麼，尚存在争議。個人認為，與其從理論層次去思辨，不妨從現實角度思考：怎樣的CodeReview是有效的？

如果一次CodeReview能夠檢測出代碼中的錯誤或設計的缺陷(即使是低級錯誤)，阻止上線後導緻BUG甚至故障，那麼就可以說此次CodeReview 是有效的、成功的。

現實才不管你是怎麼區分的，出了導緻負面影響的BUG或故障就是鐵的事實，沒有進階低級之分。

CodeReview的使命就是阻止有負面影響力的 BUG 或故障上線。

CodeReview的兩個基本問題

兩個基本問題：

如何 Review 出代碼錯誤和設計缺陷?
如何更高效地Review出代碼錯誤和設計缺陷？

問題二是問題一的遞進，先解決問題一。

CodeReview顯然不可能面面俱到，在一次送出中程式猿媛們潛在犯錯誤可能就那麼一兩個地方，而且很可能隐蔽。這就要更有針對性：程式猿媛們更容易犯怎樣的錯誤呢？一方面，可以通過常見問題來了解，另一方面，通過建立 BUG 追蹤庫，可以得到更為精準甚至出乎意料的結論。

CodeReview代碼問題

CR常見代碼問題

高效CodeReview

高效CodeReview 一方面需要深入熟悉各種代碼問題，另一方面需要一定的技巧和工具來快速識别問題。

改動點和改動描述

代碼送出者最好說明改動點和準确的改動描述，讓CodeReview的同學有心理預期，明白該 Review 哪些内容和關鍵點；

問題分類與優先級

從上一節可知，代碼問題的出現種類實在太多，而一次送出中所犯的錯誤很少很隐蔽。要想更高效的實作有效的CodeReview, 那麼必須采用一些技巧。

對代碼問題進行分類，确定優先級：常見代碼問題 > 可維護性問題 > 更難發現的問題 > 較輕微問題。首先保證代碼是正确的，然後確定代碼的可維護性。
較輕微問題最好由工具使用和代碼送出者的自律來保證；

熟悉相關業務

CodeReview第一要保證業務的正确性，需要Review者去熟悉相關業務，而不僅僅是從代碼層面來推敲。如果改動比較大，還需要推敲設計是否合理，是否劃厘清晰了系統邊界，有哪些遺漏的地方。

影響面評估

影響面評估非常重要！！！

有時，隻改動一行代碼，卻會影響百萬次調用；有時，改動幾十幾百行，也隻影響一個局部功能。确定代碼變更的影響面是CR的首要職責。如果影響面非常大，需要考慮對所有相關業務的影響，多推敲，保證嚴格的代碼健壯性、性能與逾時合理、避免資損等，進行回歸測試，確定影響可控。

有針對性

CodeReview 的同學需要對代碼有一定敏感度，熟悉各種代碼改動最可能導緻的問題，然後确定是否有問題。比如調用 API 接口，則要考慮依賴服務的可靠性，做好捕獲異常的操作和日志記錄，避免因局部影響整體功能；拉取大量資料，則要考慮采用批量或并發的方式，或者構造合适的資料結構和算法，來避免性能問題；有跨語言互動，要考慮接口之間的邊界處理是否正常，在出錯的時候是否也銜接良好；複雜的資料結構，要考慮解析是否正常以及異常捕獲；通路資料庫，要檢測是否擷取到相應的對象。

做好普通的CodeReview

幸運的是，絕大多數CodeReview 并不涉及複雜的并發場景和資料一緻性問題，隻是普通的業務操作。針對這種，可以劃分為兩種類型的Review: (1) 子子產品的CodeReview ; (2) 接口邊界的 CodeReview 。

子子產品的CodeReview尤其要注意健壯性。能夠推斷出各種可能的主要錯誤場景，并在錯誤或異常的情況下正确捕獲異常、列印日志并傳回可讀的錯誤資訊，通常就做好了一大半工作。

接口邊界的CodeReview通常要注意傳回結構是否銜接良好。正常和異常情況的資料結構傳回都要測試一下。

CodeReview工具

工具始終是提高效率的利器。

提高CodeReview效率的工具分兩類： (1) 靜态代碼檢測工具，比如 PMD, FindBugs, etc. (2) CodeReview 工具：比如 Upsource 。

CodeReview的一個實際困難是，在缺失整體代碼氛圍的情況下審視代碼片段很難保證CodeReview的有效性。也就是說，代碼片段可以“看上去是毫無問題的”，然而與現有的某個設計或細節關聯起來就變成了BUG。此外，在浏覽器中做CodeReview要求暫時離開IDE，會破壞開發者的工作狀态。Upsource 可以與 IntellJ 很好的內建，下載下傳 IntellJ 的 upsource 插件，就可以在 IntellJ 裡面 Review 相關改動，添加 Review 意見，回複 Review 意見，這些都會自動同步給 Upsource ，然後發送通知給代碼送出者。

CodeReview的另一個實際困難是，很難在短時間内對代碼可能出現的各種問題都判斷一遍，隻能專注于最可能出現的問題，而部分可維護性問題、輕微問題、程式設計風格、單元測試、正常功能測試則必須由代碼送出者保證。靜态代碼檢測工具一般在代碼送出前使用，保證在CodeReview前就消除大部分低級錯誤；這可以形成團隊代碼規範，提高CodeReview效率。

CodeReview小貼士

一些小的技巧和習慣可以讓 CodeReview 更加容易和輕松。

Review時機：對于普通bugfix或優化，CodeReview最遲要在釋出前一天或釋出當天早上；對于項目，CodeReview 最遲應該在QA提測前一天；
持續增強原則：每次送出變更盡可能小一點，但是建議每次的變更都是正确和完整的，合并到代碼庫中持續增強系統，盡量保證随時可傳遞。
去除無用改動：仔細斟酌每一行改動，去掉無用的注釋，reset 掉沒有邏輯改動（空行改動）的檔案；改動越少越好；合并送出：如果僅有一個分支，且全部是自己的改動，合并成一次送出，避免漏掉某些重要Review；
分支與送出：使用 teamname_改動簡述_年月日作為分支名，認真編寫含義明顯的 commit 注釋。
相關性：組内業務的相關改動放在單獨分支裡友善Review ; 避免在一個大而全的項目分支裡Review 少許改動造成遺漏；
改動最小化：改動檔案數代碼數盡量越小越好，減少調用方相容代碼更改；不改“陌生”代碼：不要自己更改前端代碼(涉及不熟悉的打包流程)；不要更改自己不熟悉的代碼；

代碼送出建議标準

不涉及複雜并發或一緻性的業務變更，要求能夠達到第十二級；複雜的業務，需要達到第十六級。

第一級：無文法錯誤，消除無用的代碼、注釋；解決了代碼檢測工具的警告與錯誤。
第二級：簡潔清爽的排版，合理的命名，遵循代碼規範，适宜必要的注釋。
第三級：适當的方法 Javadoc 及注釋，注明用途及作者、日期; 文檔化邊界點、例外情況、極端情況以及特殊處理。
第四級：編譯通過，成功啟動應用。
第五級：能處理正常情況下的功能實作，保證正常場景下的準确。
第六級：能處理不合法輸入，給出友好錯誤提示；能處理可能出現的錯誤和異常，輸出合理級别和合适detail容易排錯的日志。
第七級：使用互相協作良好的短小方法；消除了重複代碼；沒有寫死；沒有未實作的 TODO 樁。
第八級：編寫的代碼有比較完善的單元測試用例；對錯誤和異常進行了測試；變更有相應單元測試且通過；全量單元測試本地通過。
第九級：避免常見的錯誤，比如 +-1、越界、空指針、傳值或引用錯誤、變量未初始化為合适值、多重條件下的邏輯與或非錯誤、死循環、修改全局變量。
第十級：考慮基本的安全問題，比如SQL注入攻擊、通路或操作權限限制、屏蔽敏感資訊。
第十一級：如果涉及到性能，比如大量資料處理，選擇合适的容器及算法，使性能保持在 O(nlogn) 或 O(n)。
第十二級：使用合理的設計模式組織代碼結構，消除重複，實作可擴充性。
第十三級：確定打開的資源（http, db, file）被正确釋放。
第十四級：對于關鍵的資料一緻性業務，使用合适的資料庫事務和鎖同步。
第十五級：考慮性能、安全、線程安全及死鎖；對共享可變的狀态使用同步通路，使用并發類、線程池而不是原始的線程對象。
第十六級：開發時考慮後期維護和可運維性，使錯誤情況的排查更加高效，從錯誤中恢複更加容易。

階段性小結

CodeReview的檢查事項可以涵蓋非常廣泛的範圍，從不起眼的“變量未初始化”到“并發事務”，乃至設計性問題。這要求代碼送出者有足夠高的自律和程式設計能力，能夠盡量自行Hold住這些Review事項。而當一個人能夠做到這些時，他就具備了Review其他夥伴代碼的能力。

參考内容

CodeReview重點檢查什麼

業務邏輯的實作是否未考慮到全局的設計或現有的某些業務細節（對業務不夠熟悉的同學往往因為沒有考慮到更大的業務範圍或細節而犯錯）
是否有隐藏的細微錯誤或潛在的隐患（經驗判斷）
代碼的品質屬性，性能、可維護性、可擴充性等，對需求和設計的代碼實作方式
CodeReview 不應該檢測程式設計風格和低級錯誤；代碼送出者應當足夠自律保證這兩點
CodeReview 不應該承擔發現編譯與運作錯誤的職責；代碼中的編譯與運作錯誤應該由單元測試，接口測試，回歸測試來消除

CodeReview檢查類型及優先級

Functionality 功能問題 = High
General Unit Testing 單元測試 = High
Error Handling 錯誤處理 = High
Resource Leaks 資源洩露 = High
Performance 性能問題 = High
Thread Safety 并發安全 = High
Security 安全問題 = High
Redundant Code 重複或無用代碼 = Medium or High
Control Structures and Logical issues 控制結構和邏輯錯誤 Medium or High
Comment and Coding Conventions 注釋與代碼風格 = Low

一些老外建議做以上事項的CodeReview。事實上，對于 90% 的CodeReview 來說，通常涉及到的是 Functionality 、General Unit Testing 和 Error Handling。其他大部分很少會觸及到。是以說，與其沉迷于思辨，不如從實際角度來探讨和行動。

檢查點詳細清單

【以下内容來自于《Java Code Review清單》】

資訊安全

注釋出安全相關的資訊文檔化
系統的輸入必須檢查是否有效和在允許範圍内拒絕服務(Denial of Service)
檢驗輸入是否含有非法或惡意字元，防止注入性攻擊拒絕服務(Denial of Service)
避免對于一些不尋常行為的過分日志拒絕服務(Denial of Service)
把從不可信對象得到的輸出作為輸入來檢驗輸入檢驗(Input Validation)
避免伺服器暴露應用系統的目錄結構的配置通路限制
定義合理的角色權限分級，并授予合适的人員通路限制
從異常中清除敏感資訊(暴露檔案路徑，系統内部相關，配置) 私密資訊(Confidential Information)
不要把高度敏感的資訊寫到日志私密資訊(Confidential Information)
考慮把高度敏感的資訊在使用後從記憶體中清除私密資訊(Confidential Information)
重要資訊如密碼的儲存是否選用難以破解的不可逆加密算法私密資訊(Confidential Information)
通訊時考慮是否使用了安全的通訊方式私密資訊(Confidential Information)
避免暴露敏感類的構造函數對象構造
避免安全敏感類的序列化序列化反序列化(Serialization Deserialization)
通過序列化來保護敏感資料序列化反序列化(Serialization Deserialization)
小心地緩存潛在的特權操作結果序列化反序列化(Serialization Deserialization)
隻有在需要的時候才使用JNI 通路限制

性能

同步方法是否過度使用，同步區域是否過大并發
處理大量資料時，是否選取了合适的資料結構和高效的算法綜合程式設計
對hashtable,vector等集合類資料結構的選擇和設定是否合适，如正确設定capacity,load factor等參數，資料結構的是否是同步的綜合程式設計
是否采用通用的線程池、對象池、連接配接池等cache技術以提高性能綜合程式設計
是否采用記憶體或硬碟緩沖機制以提高效率綜合程式設計
I/O方面是否使用了合适的類或采用良好的方法以提高性能（如減少序列化，使用buffer類封裝流等）綜合程式設計
遞歸方法中的疊代次數是否合适，應該保證在合理的棧空間範圍内綜合程式設計
如果調用了阻塞方法，是否考慮了保證性能的措施綜合程式設計
避免過度優化，對性能要求高的代碼是否使用profile工具，如Jprobe等工具使用

資源洩露檢查

資源回收與洩露是否集合中的失效對象的reference 已經設定為 null 可以被回收綜合程式設計
是否所有的資源對象被正确釋放，如資料庫連接配接、Socket、檔案等綜合程式設計
資源是否被釋放多次綜合程式設計
避免使用finalizer 綜合程式設計

資料庫通路

資料庫設計或SQL語句是否便于移植（注意和性能方面會存在沖突）綜合程式設計
資料庫資源是否正常關閉和釋放綜合程式設計
資料庫通路子產品是否正确封裝，便于管理和提高性能綜合程式設計
是否采用合适的事務隔離級别綜合程式設計
是否采用存儲過程以提高性能綜合程式設計
是否采用PreparedStatement以提高性能綜合程式設計

網絡通信

socket通訊是否存在長期阻塞問題綜合程式設計
發送接收的資料流是否采用緩沖機制綜合程式設計
socket逾時處理，異常處理綜合程式設計
資料傳輸的流量控制問題綜合程式設計

錯誤處理

每次當方法傳回時是否正确處理了異常，記錄日志到日志檔案中日志
是否對資料的值和範圍的合法進行校驗輸入檢驗(Input Validation)
在出錯路徑上是否所有的資源和記憶體都已經釋放綜合程式設計
所有抛出的異常都得到正确的處理，特别是對子方法抛出的異常，在整個調用棧中必須能夠被捕捉并處理異常
當調用導緻錯誤發生時，方法的調用者應該得到一個通知異常
對可以恢複的情況使用已受檢異常(checked exceptions)，對于程式錯誤使用運作時異常(runtime exceptions) 異常
是否更多地使用标準異常異常
是否定義了具有合理名稱的自定義異常異常
是否“默默地吞掉了”異常異常

面向對象程式設計

通過接口而不是實作類來引用對象，是否符合面向接口程式設計的思想設計與重構
方法API是否被良好定義，便于維護和重構設計與重構
重寫對象的equals時，總是重寫hashCode 基礎
總是重寫對象的 toString 基礎
需要 update 的對象的值是否正确地設定基礎
是否大量或頻繁地建立臨時對象基礎
是否盡量使用局部對象(堆棧對象) 基礎
是否使用了全局可變對象且在某處代碼裡進行了修改基礎
是否修改了全局可見 final Reference 的内容基礎
在隻需要對象reference的地方是否建立了不必要的對象執行個體基礎
類的接口是否定義良好，如參數類型等，避免内部轉換基礎
是否有醜陋的強制類型轉換基礎
是否存在不必要的使用反射來擷取私密資訊基礎
傳回堆對象的reference，不要傳回棧對象的reference 基礎