Code Review 對于 MR 來說,不僅是一個工作流程,還是團隊成員為了達到共同目標而一起努力的過程。
來自 GitLab Inc. 團隊的一句話做了很好的總結:
go through a code review process to ensure the code is effective, understandable, maintainable, and secure.
Effective
“有效的”要求 MR 是正確的,能解決問題的,并且也需要在性能上過關。 這是 MR 的底線,達不到這一點的 MR,Reviewer 是一定不可以通過的。
Understandable
“可理解的”是在“有效的”基礎上的要求。我們希望剛進入團隊的新人也可以很容易讀懂過往的代碼。 Reviewer 需要換位思考,假設自己不了解背景的情況下,是否也能理解 MR 的改動。
Maintainable
“可維護的”的代碼最能提現貢獻者的編碼水平。 這里隱含了一些要求,代碼的組織方式是否符合約定,方法的顆粒度如何,是否可重用,代碼是否容易擴展,是否容易測試,異常處理是否合適等等。 這需要 Reviewer 發揮自己的經驗和智慧,引導貢獻者向著更好的方向改進。
Secure
“安全的”要求貢獻者保持良好的編碼習慣,對可能暴露風險的場景添加測試。 Reviewer 需要了解常見的漏洞和攻擊手段,為貢獻者做好最后一道把關。
創建 MR 有非常多種方式,我們并沒有要求統一從議題或者從分支創建。好的 MR 一般有以下特點:
盡可能使用英語
因為我們經常需要把 JiHu 的需求或者問題跟 Upstream 討論,使用英語會大大提高跟 Upstream 的溝通效率。
清晰的 MR 標題
標題應該簡明扼要地概括出 MR 的改動。我們希望從標題中讀出,這是一個新增功能、Bug 修復還是功能重構。
請務必以大寫字母開頭,字符數控制在 3 到 72 之間,更多規則請注意 danger-review
的提示。
在準備階段使用 Draft:
標記,準備完成后去除該標記再添加 Reviewer。
詳盡的 MR 描述
請使用 MR 模板,并根據模板填寫描述的每個章節。
好的功能 MR 應該包含以下描述信息:
好的 Bug Fix MR 應該包含以下描述信息:
恰當的標記信息
及時分配 Milestone 對追溯代碼非常有好處,能方便地推斷最早引入該 MR 的 release 是什么。
添加充分的 label
, 對復盤分析非常有用。
比如通過 fix:pipeline
可以過濾出所有修復 Pipeline 的 MR,然后對它們進行分類聚類,指導我們制定的下一步優化方案。
還有一些標記可以輔助調試,比如 pipeline:run-in-ruby3
和 pipeline:run-all-jest
。
好的提交是有組織的。
每個提交都有明確的分工,能達到分別 review 的程度。多個提交之間能表達出修改的漸進和依賴關系。
單個 MR 的提交數目不宜過多,建議不超過 10 個,強制要求不超過 20 個。
越大的 MR 越難 Review,Review 的周期會變長,引入 Bug 的可能性會加大。 單個 MR 的修改文件數不宜過多,建議不超過 10 個。對于超過 20 個文件變更的 MR,需要確認這樣做的合理性和必要性。
創建 MR 時勾選 squash
選項,盡可能保證主分支提交的干凈整潔。
提交信息編寫指南 列舉了一組提交信息規則,主要包含以下幾點:
如果 MR 對用戶的使用有影響,無論是新增去除或者修改功能,都應該書寫 Changelog
,
詳見 更新日志條目 。
我們有一篇文章專門介紹 JiHu Code Review 流程, 這里要講的是流程之外還有哪些做法能讓 MR 更好。
討論式的留言
對于 Reviewer 來說,非常有把握的建議可以使用祈使句式,對于不了解或有疑問的修改,可以使用疑問句請求貢獻者解答。 在 Code Review 中,"不求甚解" 是個貶義詞,"是什么" 和 "為什么" 的提問能幫我們更好地梳理邏輯。
Reviewer 的提問會創建出一個 Thread,如果貢獻者同意 Reviewer 的修改建議,需要在提交修改后 Thread 中標記本次修改的內容。 對于沒有爭議的小問題,貢獻者可以自行解決,其他情況一般由 Reviewer 確認后解決。
如果貢獻者不同意 Reviewer 的修改建議,需要明確當前方案的優勢并展開討論,直到跟 Reviewer 達成共識。 如果當前方案跟 Reviewer 的修改建議并沒有明顯的優劣,同時雙方也未達成共識, 那么可以再邀請一位熟悉該邏輯的 Reviewer 幫忙裁決。
若始終難以抉擇,那么選擇跟現有代碼風格更相近的方案。
主動自我留言
一般來說,貢獻者比 Reviewer 更了解具體的邏輯和需求。 在開始正式 Review 之前,推薦貢獻者先進行一輪自我 Review,并對可能產生疑問的方法添加留言。 這樣 Reviewer 就相當于在閱讀一份包含更多批注的代碼,會加速 Review 的整體進程。
方法名應該簡潔、清晰明了,能夠準確描述方法的功能和用途。 一個好的方法名能夠提高代碼的可讀性和可維護性,也能夠幫助其他開發者更好地理解你的代碼。
名詞,使用專有名詞,減少通用詞匯產生的誤解。
在 Controller 或 Helper 中,通常需要定義某種動作和過程,可以使用動賓短語,比如:
before_action :check_password_expiration, if: :html_request?
如果該過程會產生副作用,包括修改對象的數據,拋異常等,都應該使用嘆號方法 !
命名, 比如:
before_action :authenticate_user!, only: [:new, :create]
在 Model 中需要面向對象的高內聚的命名,一般以名詞、形容詞來呈現。
比如:
def preferred_language
end
def is_admin?
end
得益于 Ruby 的靈活性,我們可以把對象內的變量和函數都視為對象的屬性,以方法的形式來包裝,既閱讀通順又靈活易改,比如:
def access_level
end
def access_level=(new_level)
end
Ruby on Rails 項目是典型的 MVC 結構, 又極力推崇約定優于配置的 Rails 信條, 在代碼組織方面有比較明確的業界共識。
Ruby 這樣的解釋型語言,更有必要遵循約定的代碼組織方式, 這樣讓人在不執行 Ruby 解釋器的情況下,也可以方便并準確地找到方法的定義。
模型是對象的定義和描述。
模型不一定有對應的數據庫表,能抽象成對象的都應該定義為模型。
視圖負責展示。
模板文件中存放樣式,不應該包含復雜邏輯,如果需要邏輯判斷,則應該提取到 Helper 中。
控制器是內外溝通的橋梁。
它負責參數的安全過濾和流程的銜接,不應該包含復雜邏輯。
復雜邏輯應該在模型中實現,若果邏輯跨越多個模型,則可以創建更高抽象的模型。
多個控制器的公共部分,可以通過 concern 來重用。
好的代碼讓單元測試的編寫更加容易,如果方法有合適的顆粒度,測試會顯得更加有層次,方便控制變量,也更加容易讀懂。 所以可以通過單元測試來反向評判邏輯代碼的好壞。
RSpec 以可讀性好表達能力強著稱,我們也希望充分發揮它的優勢。
具體來說,用 context
描述測試的上下文,應該包含正例和反例,用 it
描述具體的測試結果,它應該是一句通順的自然語言(特別注意英語語法的時態)。
核心的功能很可能會與其他的功能有交叉,使用 shared_examples
能幫助重用測試用例。