Try   HackMD

Guides for Design Review & Code Review

Pros

  • Early identification of design issues when making changes is still cheap.
  • Achieving consensus around a design in the organization.
  • Ensuring consideration of cross-cutting concerns.
  • Scaling knowledge of senior engineers into the organization.
  • Form the basis of an organizational memory around design decisions.
  • Acts as a summary artifact in the technical portfolio of the software designer(s).

Cons

  • 在設計 & 開發階段花費較多時間

Image Not Showing Possible Reasons
  • The image file may be corrupted
  • The server hosting the image is unavailable
  • The image path is incorrect
  • The image format is not supported
Learn More →
但通常在軟體開發領域,事前先花這些時間 code review、提早找出問題的時間成本,遠比軟體上線後發生再修正、再維護所花費還要低

誰是你最佳的 Reviewer(s)?

  • 可能是程式碼的 owners
  • 但也可以針對特定程式碼段落,尋找最適合的人來跟你討論

Review 有哪些形式?

同步形式

同步的 Review 是即時進行的,需要兩個以上的人同時參與:

  • Pair Programming:兩人一組即時協作,一人負責編寫程式碼,另一人即時審查和提供建議。
  • Live Review:開發者與 Reviewer 即時同步進行程式碼審查與討論的一種方式,透過即時互動快速解決問題並確認程式碼品質。
  • Code Review Meeting:團隊成員集體參與的審查會議,針對關鍵程式碼變更進行深入討論,以獲得多方視角並提升產出品質。
  • Code Walkthrough:偏向知識交流性質的互動,由開發者逐步解說程式碼的邏輯與功能,團隊成員主要是以觀察與提問的角色參與,目的是提高對程式碼的理解和學習。

非同步形式

非同步的 Code Review 則不需要參與者即時作業,所有的審查和回覆都透過數位載體進行,例如:

  • PR Review:開發者提交 Pull Request,Reviewer 可依據自己的時間查看代碼變更、留下評論並進行交流。

Image Not Showing Possible Reasons
  • The image file may be corrupted
  • The server hosting the image is unavailable
  • The image path is incorrect
  • The image format is not supported
Learn More →
團隊還不大的時候,我們優先選擇 Live Review。是否要執行 Code Review Meeting 及 Code Walkthrough,取決於 author 的判斷。

Image Not Showing Possible Reasons
  • The image file may be corrupted
  • The server hosting the image is unavailable
  • The image path is incorrect
  • The image format is not supported
Learn More →
團隊不大的時候,何時使用 PR review?

  1. changes 很單純、很小的時候
  2. 團隊彼此對 code base 足夠熟悉的時候

Design review 的關注點

先參考 Design Docs at Google,了解何謂 design doc 及它可能包含的內容,並將 design doc 作為進行 design review 活動時的主要素材。

根據經驗,作為 reviewer,我會關注以下面向:

  • 我了解每個 public API 的職責嗎?
  • 我了解每個依賴的外部系統的職責嗎?
  • 是否使用適合的、易理解、好維護的架構/技術棧?
  • 我了解目前的架構「妥協」的項目嗎?這些被妥協的項目何時會變得又重要又緊急?
  • 我了解目前的規劃接受了哪些「風險」嗎?

Image Not Showing Possible Reasons
  • The image file may be corrupted
  • The server hosting the image is unavailable
  • The image path is incorrect
  • The image format is not supported
Learn More →
一個 checklist,參考自 《微服務實戰 (Microservices in Action)》P.309

Aspects Purposes
問題與背景 這個功能想要解決的問題? 為什麼我們要這麼做?
解決方案 打算如何解決此問題
依賴項目與整合 如何與現有的或預計要開發的服務、功能及組件進行互動
API 接口 服務會暴露哪些操作
擴展性與效能 該功能如何進行擴展?維運成本試算?
系統可靠度 希望可靠度到達什麼程度?
冗余性(redundancy) 備份、復原、部署與回滾
監控儀表板 如何了解服務的行為?
故障情境 如何因應或消除潛在的故障影響
安全性 威脅樣態、資料保護等
部署 如何使該功能上線
風險與開放問題 目前已識別了哪些風險?有哪些是開發者不知道的風險?

Code review 的關注點

以下內容摘要自 Code Review Developer Guide at Google,並分成 reviewer 以及 author 兩者立場應關注的面向。

How To Do A Code Review? (as a reviewer)

A practical checklist from Code Review Developer Guide: What Do Code Reviewers Look For? - at Google

Aspects Purposes
Design 程式碼是否精心設計且適配你的系統?
Is the code well-designed and appropriate for your system?
Functionality 程式碼是否符合作者的意圖?是否顧全 user 的利益?
Does the code behave as the author likely intended? Is the way the code behaves good for its users?
Complexity 程式碼可以更簡潔嗎?其他人是否可以輕易理解並應用?
Could the code be made simpler? Would another developer be able to easily understand and use this code when they come across it in the future?
Tests 程式碼是否有正確且設計良好的自動化測試?
Does the code have correct and well-designed automated tests?
Naming 變數、類別、方法命名是否清晰?
Did the developer choose clear names for variables, classes, methods, etc.?
Comments 註解是否清晰且有用?
Are the comments clear and useful?
Style 程式碼是否遵循我們的慣例?
Does the code follow our style guides?
Documentation 相關文件是否有更新?
Did the developer also update relevant documentation?

The CL Author's Guide (as an author)

CL: change list. interchangeable terms: code changes, MR (merge request), and PR (pull request)

Writing good CL descriptions

see original page for some good and bad examples

  • 應在 PR title 對你的改動有簡短的總結,並提供足夠清楚的訊息於 PR description body
  • 又通常小改動 (i.e. only 1 line),更需要你闡述清楚的是 context

Small CLs

每次改動範圍小,好處多多:

  • reviewer 被打斷的時間更短,review 更快、更詳盡、周全
  • 容易設計的更好,再寫出 bug 的機率較低
  • 若被 reject,工作量也不會太大
  • 容易 merge、容易 rollback (revert)

How to handle reviewer comments

Image Not Showing Possible Reasons
  • The image file may be corrupted
  • The server hosting the image is unavailable
  • The image path is incorrect
  • The image format is not supported
Learn More →
別往心裡去 (Don't Take it Personally)

  • 記得,大家都是為了如何讓程式碼品質上升、維護好 code base 及產品的品質而努力。絕非針對你個人的能力或人格作出攻擊或評價
  • 當然, reviewer 也有責任提出有建設性的建議,而不要作出不禮貌或違反職場禮儀的評論
  • 若你覺得 reviwer 有任何冒犯之處,請嘗試像他表達你的感受。如情況未改善,請找更高層的 manager 反應此情事

Image Not Showing Possible Reasons
  • The image file may be corrupted
  • The server hosting the image is unavailable
  • The image path is incorrect
  • The image format is not supported
Learn More →
修正程式碼 (Fix the Code)

  • 如果 reviewer 提出了一段 code 他看不懂,表示很有可能其他 developers 也會看不懂
  • 如果你同意 reviewer 的意見,此時你應該是修改程式碼撰寫風格使其更簡潔清晰,或在程式碼中加入適當的註解以解釋為何存在。而不是寫在 code review tool (e.g. Gitlab),因為其他 developers 在未來看到的是 code 而不是當年的 MR。
  • 當然,如果你不同意,或 reviewer 的意見真的無關緊要,直接回應在 code review tool 上就可以了

Image Not Showing Possible Reasons
  • The image file may be corrupted
  • The server hosting the image is unavailable
  • The image path is incorrect
  • The image format is not supported
Learn More →
自我覺察 (Think for Yourself)

  • 你花了很多力氣寫 CL description,你當然希望 reviewer 少廢話、給我過。但請收到意見回饋時退一步思考該 reviewer 的建議有沒有價值。此時可放在心上的第一個疑問是:「reviewer 的意見到底正不正確?」
  • 如果你無法判斷 reviewer 的建議是否正確,請向 reviewer 要求再進一步的澄清、說明
  • 如果你已經考慮過,並且仍然認為自己的判斷較合適,請隨時準備好解釋為什麼你的做事方法對 code base、users、products 與/或 company 更好
  • reviewer 的確很可能缺少足夠的上下文來做出最正確的評論,你也確實知道一些 reviewer 不知道的細節。試著給 reviewer 更多的背景,以讓雙方在技術事實上達成共識

References