在實際工作中，經常會遇到項目交接或者二次開發的情況，在這個過程中，我們經常會聽到“這是什麼垃圾代碼啊”。有時候我們翻看自己幾年前寫的代碼，也會忍不住鄙視自己。
在軟件開發過程中，代碼Review是一個可以提高代碼質量，統一代碼規範，分享技術知識，從而形成增長團隊的有效手段。
在代碼Review過程中，存在兩個角色：
- 提交者。提交者就是代碼的提交人，他發起了Review事件。同樣也可以稱作被審查者。
- 審查者。審查者是對代碼進行Review的人。
在本文中，主要涉及了以下內容：
- 為什麼要代碼Review
- 何時代碼Review
- 準備代碼Review
- 進行代碼Review
- 代碼Review示例
動機
通過代碼Review可以提供代碼質量，並且我們還可以通過代碼Review來提高自我的能力。
比如：
- 通過代碼Review，審查人員可以看到本次變更的內容：處理TODO，代碼優化等。提交者的代碼被認可，可以提升自我成就感。
- 可以分享知識：
- 代碼Review可以是提交內容更加明確，並且使團隊成員更進一步瞭解項目，為以後的開發做知識積累
- 團隊成員可以從提交者的代碼中學習新的技術、算法等等
- 通過代碼Review，提交者可以從審查人員的評審中獲得相關的技術知識
- 可以增加團隊交流，形成增長團隊
- 可以形成統一的代碼規範，方便閱讀和理解
- 審查者因為沒有完整的上下文，只看到代碼片段，更容易發現問題，提高代碼片段的可複用率
- 更容易檢查拼寫錯誤
- 可以避免常規的安全問題等
Review什麼
對於代碼Review什麼內容，可以有很多的方面，如：變量命名、代碼結構、算法、架構、安全等等。具體內容沒有一個統一的標準，但是在一個團隊中，是需要形成一個統一的標準的，這樣更有益於團隊的可持續發展。
什麼時候Review
代碼需要在測試、CI之後，在合併上線分支之前。測試、CI等確保了邏輯是正確的。因為需要保證線上的代碼是最優的，所以Review需要在合併分支之前。
準備Review
提交者需要提交一個便於Review的代碼，避免浪費審查者的精力和時間：
- 範圍和大小。一次提交Review的代碼不應過大，如果太大需要耗費一天的時間，那就說明提交Review的代碼不夠合理，應分解成多次Review提交。
- 只提交已完成的，並且自檢及自測過的代碼。提交Review的代碼，一定是已經開發完的，否則Review將沒有意義。它也一定是經過自測的代碼，對沒有通過自測的代碼進行Review，同樣沒有意義。
- 重構不應該改變代碼行為，同樣改變代碼行為的不應該包含重構內容。每次提交的變更目標應該是明確的，且是單一的，不能將重構和開發新功能合併到一起提交。
進行Review
代碼Review一定要及時，不能因為卡在沒有進行Review而影響項目進度。如果審查者時間不允許，應立即告知提交者，讓他找其他人對代碼進行Review。
作為審查者，有責任執行編碼標準並保持質量水準。 審查代碼更多是一門藝術，而不是一門科學。 學習它的唯一方法就是去做。 有經驗的審查者需要考慮讓經驗不足的審查者先Review，以此來提高他們的Review經驗。
假設提交者遵循上面的指南（尤其是關於自我檢查並確保代碼可以運行的準則），審查者在代碼Review過程中應注意的事項應注意以下事項：
- 目標
- 這段代碼是否達到了提交者的目的？ 每次更改都應有特定的原因（新功能，重構，錯誤修正等）。 提交的代碼是否真的達到了這個目的？
- 提問
- 函數和類應該存在是有原因的。 當原因對於審查者來說不清楚時，這可能表明該代碼需要重寫、添加註釋等等。
- 實現
- 考慮一下您將如何解決問題。 如果不同，那為什麼呢？ 您的代碼可以處理更多（邊緣）情況嗎？ 它更短、更容易、更清潔、更快、更安全，但在功能上等效嗎？ 您發現當前代碼未捕獲的異常了嗎？
- 您看到有用的抽象的潛力嗎？ 部分重複的代碼通常表示可以提取出更抽象或更通用的功能，然後在不同的上下文中重新使用。
- 像對手一樣思考，但要對此保持友善。 嘗試通過提出有問題的配置、輸入數據來破壞他們的代碼，從而找出程序裡面的漏洞。
- 考慮庫或現有產品代碼。 當某人重新實現現有功能時，通常是因為他們不知道該功能已經存在。 有時，有意複製代碼或功能，例如，以避免依賴。 在這種情況下，代碼註釋可以闡明意圖。 現有庫是否已提供引入的功能？
- 更改是否遵循標準模式？ 既定的代碼庫通常表現出圍繞命名約定，程序邏輯分解，數據類型定義等的模式。通常希望根據現有模式來實現更改
- 更改是否添加了編譯時或運行時依賴項（尤其是在子項目之間）？ 我們希望保持我們的產品鬆散耦合，並儘可能減少依賴。 對依賴項和構建系統的更改應進行嚴格審查。
- 易讀性與風格
- 考慮一下您的閱讀經驗。 您是否在合理的時間內掌握了這些概念？ 流程是否合理，變量和方法名稱是否易於理解？ 您是否能夠跟蹤多個文件或功能？ 您是否因名稱不一致而推遲？
- 該代碼是否遵守編碼準則和代碼樣式？ 代碼在樣式，API約定等方面是否與項目一致？ 如上所述，我們更喜歡使用自動化工具解決代碼規範。
- 此代碼是否有TODO？ TODO只是堆積在代碼中，並且隨著時間的流逝變得陳舊。 讓作者在GitHub Issues或JIRA上提交記錄，並將發行號附加到TODO。 建議的代碼更改不應包含註釋掉的代碼。
- 可維修性
- 閱讀測試。 如果沒有測試，應該進行測試，請提交者寫一些測試。 真正不可測試的功能很少見，而不幸的是，未經測試的功能實現很常見。 自己檢查測試：它們是否涵蓋了有趣的案例？ 它們可讀嗎？ CR是否會降低總體測試覆蓋率？ 考慮一下此代碼可能如何破解。 測試的樣式標準通常與核心代碼不同，但仍然很重要。
- 此CR是否存在破壞測試代碼，登臺堆棧或集成測試的風險？ 這些通常不作為預提交/合併檢查的一部分進行檢查，但是讓它們崩潰對每個人來說都是痛苦的。 要查找的特定內容是：刪除測試實用程序或模式，配置更改以及工件佈局/結構更改。
- 此更改會破壞向後兼容性嗎？ 如果是這樣，此時可以合併更改，還是應該將其推送到更高版本中？ 中斷可能包括數據庫或架構更改，公共API更改，用戶工作流更改等。
- 此代碼是否需要集成測試？ 有時，單獨使用單元測試無法對代碼進行充分的測試，尤其是當代碼與外部系統或配置交互時。
- 留下有關代碼級文檔，註釋和提交消息的反饋。 多餘的註釋使代碼混亂，而簡短的提交消息使將來的貢獻者迷惑不解。 這並不總是適用，但是高質量的評論和提交消息將使他們自己付出代價。 （想想您曾經看到過出色的或真正可怕的提交信息或評論。）
- 外部文檔是否已更新？ 如果您的項目維護自述文件，CHANGELOG或其他文檔，是否已對其進行更新以反映更改？ 過時的文檔可能比沒有文檔更令人困惑，並且將來對其進行修復要比現在進行更新要花費更多的成本。
- 安全
- 驗證API端點是否執行與其餘代碼庫一致的適當授權和身份驗證。 檢查其他常見弱點，例如弱配置，惡意用戶輸入，缺少日誌事件等。如有疑問，請向應用程序安全專家諮詢Review。
- 評論
- 簡潔、友好、可操作的。不要忘了讚揚簡潔、可讀、高效、優雅的代碼。 相反，拒絕或不批准代碼Review並不粗魯。 如果更改是多餘的或無關緊要的，請拒絕並說明。
- 面對面Review
- 對於大多數代碼檢查而言，基於異步差異的工具（例如Reviewable，Gerrit或GitHub）都是不錯的選擇。 當在同一臺屏幕或投影儀前親自進行或通過VTC或屏幕共享工具遠程執行時，複雜的更改或具有不同專業知識或經驗的各方之間的評論可以更有效。
示例
在以下示例中，建議的評論註釋在代碼塊中由 // R：... 註釋標識。
命名不一致
class MyClass {
private int countTotalPageVisits; //R: 變量命名不一致
private int uniqueUsersCount;
}方法簽名不一致
interface MyInterface {
/** Returns {@link Optional#empty} if s cannot be extracted. */
public Optional<string> extractString(String s);
/** Returns null if {@code s} cannot be rewritten. */
//R: 應該協調返回值：在這裡也使用Optional <>
public String rewriteString(String s);
}/<string>類庫使用
//R: 使用Guava's MapJoiner替換以下方法
String joinAndConcatenate(Map<string> map, String keyValueSeparator, String keySeparator);/<string>
個人傾向
//R: nit: I usually prefer numFoo over fooCount; up to you,
// but we should keep it consistent in this project
int dayCount;
Bugs
//R: 代碼處理numIterations+1的情況，如果是故意這樣處理，是否考慮變更numIterations值
for (int i = 0; i <= numIterations; ++i) {
...
}
架構疑慮
//R: I think we should avoid the dependency on OtherService.
// Can we discuss this in person?
otherService.call();
總結
通過有效的代碼Review，可以提高項目代碼質量，使團隊開發人員形成統一風格，並同步項目細節。同時還可以提高團隊人員的知識，提升自我。
閱讀更多 大王TinyKing 的文章
