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?
- changes 很單純、很小的時候
- 團隊彼此對 code base 足夠熟悉的時候
Design review 的關注點
先參考 Design Docs at Google,了解何謂 design doc 及它可能包含的內容,並將 design doc 作為進行 design review 活動時的主要素材。
根據經驗,作為 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 →
一個 checklist,參考自 《微服務實戰 (Microservices in Ac》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