simon

代碼審核指南 - 代碼審核規範#

創建日期：2021 年 6 月 22 日 下午 5:47

基於 Google Code Review Practice 和 Zaihui 實踐

#術語 Term

為簡化描述，先統一術語縮寫

• TLDR - Too long, Don't Read 太長不讀的版本
• Author - 本 PR 的作者 代碼作者
• CR - Code Review 代碼審查，也簡稱為 Review
• PR - Pull Request, 代碼合併請求，包含 Change List 代碼變更
• Reviewer - 審查代碼的人
• WIP - Working In Progress 開發中，不可以被合併
• Comment
  • 本文有兩種 comment
  • 代碼中的 comment，是為注釋
  • CR 過程中留下的 comment，是為改進意見
• Code Base - 代碼庫
• Deadline - 截止日期
• Judgement Call - 依據你工程師的良心進行的個人選擇
• LGTM - Look good to me 看起來不錯
• NIT
  • NitPick
  • 雞蛋裡挑骨頭，指可以被忽略的改進建議
• SG - Style Guide, 代碼風格規範
• UT - Unit Test, 單元測試
• Emergency - 緊急情況，可以放棄部分 CR 標準
  • Hotfix
  • 出現不合理的 deadline 時，但 deadline 過後，也應當補上缺失部分

目的 Purpose#

• 進行高質量快速的 Code Review，用以持續提高 Code Base 的代碼質量
• 通過閱讀代碼或留下 comment，傳授或學習新的語言、框架、設計模式等知識

原則 Standard#

• 保證 Code Base 的整體代碼健康
• 滿足原則 1 的情況下，儘可能的 approve PR，即便它不完美
• 持續優化比完美更重要
• 儘可能留下多的改進意見 comment，如果改進不是必須的，用 'nit:' 開頭以表示不是必須改進。
• Review comment 應當儘量提供一定的改進方案，但也應當鼓勵 author 發現更好的方案。
• 除非是緊急 PR，否則 PR 不應該損害 Code Base 的整體質量，何為緊急後續有描述

關注要點 What to look for#

總結 Summary-TLDR#

• 代碼需要很好的設計
• 功能性應當對代碼的使用者友好
• 任何的 UI 變動都必須合理且良好
• 任何並行編程都要保證其安全性
• 代碼應該儘可能的簡單
• 開發者不應該提前編寫現在還不要的功能
• 代碼應該有合適的 UT
• UT 需要有良好的設計
• 開發者應該給所有東西一個簡潔清晰的命名
• 代碼注釋應該簡潔和有用，並且解釋原因，而不是解釋是什麼
• 代碼應該被合適的文檔化
• 代碼要符合代碼風格規範

設計 Design#

• 與其他代碼的交互設計合理與否？
• 與整合系統的整合性如何？

功能性 Functionality#

• PR 的作者目的是什么？
• 這個目的對系統是好的還是不好的？
• 並發編程的是否必要，以及是否會觸發死鎖（deadlock）或者競態（race condition）。儘量避免使用會造成死鎖或競態的並發模型。

複雜度 Complexity#

• PR 是否比起應當的過度複雜？
• 定義
  • 如果當前的設計，導致使用或者修改這些代碼變得更容易引入 bug，則說明其設計過於複雜
  • 過度設計 over-engineering
    • 開發者將代碼設計的比其需要的更通用
    • 或者加入了系統當前並不需要的功能
    • 未來的功能儘量留在未來解決，因為當這個需求真的需要時，其相關的客觀環境和場景可能已經發生變化，導致提前寫的代碼並不適用。
    • PS: 提前開發沒有必要，但為未來的需求在設計上留有餘地是有需要的
• 分析角度
  • 獨立的代碼行
  • 函數
  • 類

測試 Testing#

• 通常情況下，測試必須和增加的功能代碼在同一個 PR 中，除非是緊急情況
• 測試設計
  • 準則
    • 正確
    • 合理
    • 有用
    • 有效
  • 需要思考的問題
    • 數據準備是否和真實情況完全一致？不要只構造部分數據
    • 所有成功的路徑是否都覆蓋到？
    • 所有 400 的可能性是否都有針對的測試？返回的錯誤信息是否有校驗與預期一致？
    • 成功的請求，除了返回數據一致以外，數據庫內的數據是否也是準確？
    • 代碼被破壞時，測試是否會正常的失敗？
    • 如果代碼發生變化，是否會出現虛假的正確（false positives）？
      • 例如為了讓 UT 通過，設置一些特殊的數據，表面 UT 通過，實際有 bug
    • 每個測試是否都有簡單有效的校驗？
    • 測試是否合理的切分成不同的測試方法？
    • 測試也是代碼，因此不要因為他們不是主要分支，就允許他們進行不必要的複雜（complexity）

命名 Naming#

• 開發者是否每個命名都是好的？
• 好的命名需要
  • 包含足夠的信息描述它是什麼或做什麼
  • 但又不至於過長導致難以閱讀

代碼注釋 Comment#

• 原則
  • 簡潔
  • 可理解
  • 必須
• 內容
  • 是代碼所無法表達的內容
  • 應該出現的內容
    • 決定背後的原因
    • 為何這些代碼會存在
    • todo 未來需要做的事，以 todo 開頭進行 comment
  • 不應該出現的內容
    • 不應該解釋代碼在做什麼
      • 代碼功能解釋應該由代碼自己來表達
      • 如果代碼不夠簡潔，需要文字描述來說明其功能，那麼代碼需要被簡化
      • 例外：正則表達式或者複雜算法依賴詳細的 comment 解釋
    • 不應該解釋代碼應該怎麼用，有什麼表現
      • 這是文檔該做的事情

風格 Style#

• 公司內部，每一種使用的主要語言都要有對應的強制統一的合理風格規範（style guide，SG）
• SG 是絕對的權威。PR 應該符合 SG，SG 不應該適應 PR。
• 如果你想提出 SG 中不存在的代碼風格改進，需要加上 “nit”，表明其不是必須的。不應該因為個人的代碼風格偏好，拖慢 PR 通過的速度。
• 必須的 style 應該加入到 SG 中
• 如果進行大量的代碼風格修改，必須單獨提出 PR，不可以與其他功能代碼混合。

一致性 Consistency#

• PR 是否與 SG 一致？
• 如果 SG 的內容只是推薦，而不是強制要求，那是否修改就依賴 Judgement call
  • 但還是優先偏向符合 SG，除非改動後會使代碼可讀性下降

文檔 Document#

• 何時需要文檔？
  • 當 PR 改變了代碼的使用、測試、交互或發布流程
  • PR 刪除或棄用（deprecated）代碼，考慮刪除相關文檔
  • 如果發現文檔缺失，請要求原作者添加文檔！
• 需要修改的文檔
  • README
  • 任何相關的文檔

每一行 Every Line#

• 請閱讀每一行你被分配到要 review 的代碼
• 你可以掃讀的內容
  • 數據文件
  • 生成的代碼
  • 大型數據結構
• 不可掃讀的內容
  • 人為書寫的 類、方法、代碼塊
• 部分代碼值得獲得更多的關注
  • 這依賴 reviewer 的 judgement call
  • 對於不想給太多關注的代碼，你也必須至少確定理解這些代碼在做什麼
• 如果你發現代碼過長或過於複雜，妨礙了你的 review 速度和理解速度
  • 你應當立即告知 PR 作者
  • 等待 PR 作者簡化或者聲明清楚代碼作用後，再繼續進行 code review
• 如果你理解這些代碼，但感到自己的能力不夠評審其中部分的代碼
  • 請確保邀請一個你認為能力足夠的 reviewer，加入到 code review 中

上下文 Context#

• 添加的代碼塊往往只是部分邏輯，需要結合上下文的代碼來閱讀
• 如果上下文不多，可以在代碼評審工具中展開查看
• 如果上下文過於複雜，則需要將代碼下載到本地，使用 IDE 或其他工具查看

好實踐 Good Things#

• 通常，review 留下的 comment 是提出批評改進意見的
• 但，當你發現代碼中有很棒的東西時，請不要吝嗇用 comment 發出你的讚美，表達你對 PR author 的感激，鼓勵大家使用更好的實踐。
  • 比如：“牛逼啊”
  • 比如：“這段代碼寫的很精妙，學習了”
  • 除了讚美詞，要是有些言之有物的讚美就更加棒啦，可以和作者產生共鳴

如何看一個 PR#

總結 - TLDR#

• 符合我們的 PR 規範
• 這個 PR 變動是否合理，PR 不合理需要立即打回
• 檢查 PR 的主要部分的設計，出現設計問題，需要立即反饋
• 以代碼邏輯鏈的順序看完餘下部分

Step zero: 是否符合必要的規範#

• 是否關聯的 Jira ticket
• commit message 是否符合規範
  • Jira-ticket(tag): description
  • tag:
    • feat 功能
    • fix 修復
    • ut 單元測試
    • mi 表改動
    • refactor 重構
    • inf 基建
• 是否 rebase 了目標分支

Step one: 對變動進行一個大致的了解#

• 查看 PR 描述，了解其大致在做什麼
• 改動本身是否 make sense
• 如果感到不合理，需要立即反饋
  • 反饋時，先肯定對方的工作，再給出不接受的理由，並提出下一步該怎麼做
• 出現持續的不合理 PR 時，需要反思一下開發流程（團隊內部和延伸團隊）
  • 使這些不合理的 PR，在開發前就被叫停

Step two: 檢查 PR 的主要部分#

• 找到主要代碼邏輯文件
• 如果找不到，就詢問作者，或者要求作者進行 PR 拆分
• 先檢查主要的設計問題，如果出現問題，必須立即反饋，並停止當前的 CR
  • 一般開發者提出 PR 後，會繼續開發，如果基於這個 PR 開發，需要儘快叫停，及時止損
  • 設計問題需要大量的時間來修復，大部分功能都有一個 deadline，提早提出修改，有利於趕上 deadline

Step three: 以合理的順序查看餘下的 PR#

• 找出 PR 的邏輯鏈
• 按照邏輯鏈 Review
• 可以先讀 UT，有利於了解設計理念

快速的 CR#

總結 - TLRD#

• 即使很忙碌也要即時反饋
• 最長反饋時間在一個工作日內
• 如果你正在專注工作，請不要中斷手上的工作
• 大型的 PR 應當被拆分成小的 PR，如無法拆分，也必須完成 step one 的 review
• 代碼的複雜度導致 CR 速度下降時，需要立即反饋作者進行簡化或給出說明

原因#

• 過慢的 CR 會降低團隊效率，阻塞其他人的開發
• 過慢的 CR 可能導致開發者抵觸 CR 流程
• Code base 的代碼質量會受到影響
  • 一方面提升代碼不能有效的合併
  • 另一方面，過慢的 CR 會降低開發者的開發質量

應當多快？#

• 除非你在專注的進行一項任務，否則接到 CR 請求時應當立即開始 review
• 最長反饋時間不可以超過 1 個工作日

速度 VS 打斷#

• 保證不被打斷比 CR 的速度重要
• 因為從打斷恢復為專注狀態需要大量的時間
• 應當選擇一個專注中斷的點，開始 CR

快速響應#

• 即使你很忙，也應當有個及時的響應
• 如果真的非常忙，可以先提供一些大致的意見，也就是上文的 step one

跨時區 CR#

• 保證在對方下班前完成 CR，不然 CR 流程就會因為時差大大的延長

LGTM with comments#

• LGTM 意味著可以合併
• 需要注意的是，LGTM 的本意是，這段代碼符合 “我們” 的標準，而 不是 “我”
• 有的時候 LGTM 也有 comment，但不影響給 approve
  • reviewer 對開發者有信心，知道其後續會解決這個問題
  • 餘下的 comment 不關鍵，不是必須要解決

大型 PRs#

• 要求 author 將 PR 拆分成小的 PR
• 如果無法拆分，那麼儘量過一下整體設計，即 step one
• 保證降低代碼質量的前提下，不阻塞開發的下一步流程

Author 在 CR 中應當做什麼#

關聯一個 Jira ticket#

• PR 的內容要與 Jira ticket 一致
• 出現不同 task，需要新建 ticket 關聯，單獨提 PR

以 feature branch 提出#

• 不應當用自己遠端中與主要分支同名的分支提出 CR
• 也不應用 alpha-only, free-only 這種非穩定分支提測
• 應當在自己的遠端，基於目標分支為 code base，創建的 feature branch 進行開發和 CR

以 Code Review 的角度審視一遍自己的 PR#

• 詳情參照 第四節

完成冒煙用例自測#

• 提出 CR 請求前，需要完成自測。
• 測試大佬會提供冒煙用例 checklist，需要全部完成。

遵守 commit message 規範#

• 一個 PR，一個 ticket，一個 commit
• 關聯 Jira ticket
• 記得移除 WIP 的前綴
• 格式：見 第五節 的 step zero

保證 pipeline 通過#

• code style
• test
• build

指定必須要 review 的 assigner#

• 對於新的功能邏輯，除了 author，還需要一位 backup 人員作為主要的 reviewer，這樣同樣的邏輯就不止一個人知曉
• 對於舊邏輯的改動，需要 assign 給原始代碼的作者進行 review，因為對方更加了解這部分的業務邏輯和歷史問題。原作者的信息可以通過 git blame 獲知

跟進 CR 的進度，提供必要的幫助和反饋#

• 一旦 CR 提出，author 就有義務和責任推動 PR 快速被合併 code base
• 如果代碼邏輯鏈路比較長，可以通過 PR 描述 或 直接給 reviewer 解釋的方式，幫助 reviewer 快速理解主要邏輯鏈。但要注意，儘量減少對 reviewer 的打斷，只有當 reviewer 表示開始 CR 才進行口頭描述。
• 對於 Reviewer 提出的 comment，都需要以 comment 的形式回覆，給出是否修改的回覆。如果不準備修改，需要提供合理的理由。如果 reviewer 還是不予同意，則需要線下討論或引入第二個 reviewer 進行仲裁。
• 如果 PR 在 CR 提出後一個工作日內還沒有被 review，第二日晨會的時候需要提出。

PR 合併後，應儘快提測#

• PR 合併表示這段代碼可以進入測試流程
• 如果是前後端聯合開發，一般由前端提測。純後端開發功能由後端提測。
• 但本著分步提測的原則，後端 PR 合併後也可單獨提測，這點需要和測試提前溝通確認

Review 的開始#

原則：

• reviewer 的精神狀態要完備
• reviewer 不處於心流的工作狀態
• reviewer 和 author 可以高效地互動

結論：

• CR 需要有個固定的時間
• 推薦在晚飯後的一個小時