Code review at scale

2019/7/18に開催されたLINE Developer Meetup #55 in Fukuokaでの登壇資料です

Transcript

1. Code review at scaleɹɹ ɹ A case study of LINE for Androidɹɹ Munetoshi Ishikawa LINE Developer Meetup #55 in Fukuoka - Jul. 18, 2019
2. Topics 1. Scaling out code review 2. What to review 3. Reviewing Kotlin code
3. Topics 1. Scaling out code review 2. What to review 3. Reviewing Kotlin code Scaling out code review > Introduction
4. Stats of LINE client for Android - About 1.5M lines of code - More than 50 active engineers for only the main repository - Many submodules - Many libraries and dependencies Scaling out code review > Introduction
5. Stats of LINE client for Android - About 1.5M lines of code - More than 50 active engineers for only the main repository - Many submodules - Many libraries and dependencies Scaling out code review > Introduction
6. What happens in a large team Even only for code review... - Heavy review request to only a few reviewers - Uneven review quality Scaling out code review > Introduction
7. What happens in a large team Even only for code review... - Heavy review request to only a few reviewers - Uneven review quality Harder to scale code review than code writing Scaling out code review > Introduction
8. How to scale out review process Easier reviews + more reviewers + higher code visibility Scaling out code review > Introduction
9. How to scale out review process Easier reviews + more reviewers + higher code visibility - Refuse problematic PRs - Educate reviewers and reviewees - Keep the master branch up to date Scaling out code review > Introduction
10. How to scale out review process Easier reviews + more reviewers + higher code visibility - Refuse problematic PRs - Educate reviewers and reviewees - Keep the master branch up to date Scaling out code review > Refuse a problematic PR
11. Refuse a problematic PR Close a large/complicated PR without looking at the detail Must not spend a lot of time Scaling out code review > Refuse a problematic PR
12. Refuse a problematic PR Close a large/complicated PR without looking at the detail Must not spend a lot of time Scaling out code review > Refuse a problematic PR
13. After closing a problematic PR Ask the reviewee to - Split PRs - Discuss the architectural design - Write a document for the plan Scaling out code review > Refuse a problematic PR
14. After closing a problematic PR Ask the reviewee to - Split PRs - Discuss the architectural design - Write a document for the plan Scaling out code review > Refuse a problematic PR
15. How to split a PR Top down / bottom up approach Scaling out code review > Refuse a problematic PR
16. How to split a PR Top down approach: Show dependencies with stub classes class UserProfilePresenter(profileView: View, profileUseCase: ...) { fun onEditButtonClicked() = TODO() Scaling out code review > Refuse a problematic PR
17. How to split a PR Top down approach: Show dependencies with stub classes class UserProfilePresenter(profileView: View, profileUseCase: ...) { fun onEditButtonClicked() = TODO() Bottom up approach: Create small classes first data class UserProfileData(val userId: Long, val name: String, ...) Scaling out code review > Refuse a problematic PR
18. How to split a PR Top down approach: Show dependencies with stub classes class UserProfilePresenter(profileView: View, profileUseCase: ...) { fun onEditButtonClicked() = TODO() Bottom up approach: Create small classes first data class UserProfileData(val userId: Long, val name: String, ...) Describe the future plan in the PR description Scaling out code review > Refuse a problematic PR
19. How to scale out review process Easier reviews + more reviewers + higher code visibility - Refuse problematic PRs - Educate reviewers and reviewees - Keep the master branch up to date Scaling out code review > Reviewer/reviewee education
20. Educate reviewers and reviewees Discussion, knowledge sharing, and training - Discussion before implementation, design docs - Pair programming - Pair code review - Two phase code review - Classroom lectures Scaling out code review > Reviewer/reviewee education
21. Educate reviewers and reviewees Discussion, knowledge sharing, and training - Discussion before implementation, design docs - Pair programming - Pair code review - Two phase code review - Classroom lectures Scaling out code review > Reviewer/reviewee education
22. Pair code review Offline discussion leads to good review comments Scaling out code review > Reviewer/reviewee education
23. Pair code review Offline discussion leads to good review comments Reviewers can be confused about what to comment - "Something smells wrong, but no idea what" - "There are several approches, but which is the best?" Scaling out code review > Reviewer/reviewee education
24. Key ideas of pair code review Same as "pair programming" - Go to review assignee's desk - Share the key idea of what to comment - Take initiative if the partner gets stuck Scaling out code review > Reviewer/reviewee education
25. Educate reviewers and reviewees Discussion, knowledge sharing, and training - Discussion before implementation, design docs - Pair programming - Pair code review - Two phase code review - Classroom lectures Scaling out code review > Reviewer/reviewee education
26. Two phase code review Take junior engineer's review before senior's review = Assign a senior engineer after a junior approved Scaling out code review > Reviewer/reviewee education
27. Two phase code review Take junior engineer's review before senior's review = Assign a senior engineer after a junior approved The junior engineer learns - Which points should be checked - How to reach an agreement with the reviewee Scaling out code review > Reviewer/reviewee education
28. Bonus points of two phase code review Filters out problematic PRs - Rebalances senior engineer's work load Scaling out code review > Reviewer/reviewee education
29. Bonus points of two phase code review Filters out problematic PRs - Rebalances senior engineer's work load Guarantees few superficial problems - Can focus on fundamental review Scaling out code review > Reviewer/reviewee education
30. Educate reviewers and reviewees Discussion, knowledge sharing, and training - Discussion before implementation, design docs - Pair programming - Pair code review - Two phase code review - Classroom lectures Scaling out code review > Reviewer/reviewee education
31. Classroom lectures Sessions for code quality and conventions Scaling out code review > Reviewer/reviewee education
32. Bonus point of classroom lectures Can review with a link Scaling out code review > Reviewer/reviewee education
33. Bonus point of classroom lectures Can review with a link Can omit reason, details, and background knowledge Scaling out code review > Reviewer/reviewee education
34. How to scale out review process Easier reviews + more reviewers + higher code visibility - Refuse problematic PRs - Educate reviewers and reviewees - Keep the master branch up to date Scaling out code review > Keep the master branch up to date
35. Limitations of code review Impossible to make review quality perfectly consistent - Finite education/development time - Hierarchal team structure - Limitations in tooling Scaling out code review > Keep the master branch up to date
36. Limitations of code review Impossible to make review quality perfectly consistent - Finite education/development time - Hierarchal team structure - Limitations in tooling Problematic code may be merged Scaling out code review > Keep the master branch up to date
37. Keep the master branch up to date Find problematic code as soon as possible Scaling out code review > Keep the master branch up to date
38. Keep the master branch up to date Find problematic code as soon as possible Merge code to the master branch even if incomplete - Use feature flags instead of feature branch - Merge back release branches to the master branch periodically Scaling out code review > Keep the master branch up to date
39. Feature flag Refer to ڊେͳΞϓϦͷ։ൃΛࢧ͑Δϑϥά؅ཧज़ presented at DroidKaigi 2019 Scaling out code review > Keep the master branch up to date
40. Scaling out code review: Summary Easier reviews + more reviewers + higher code visibility - Refuse problematic PRs - Educate reviewers and reviewees - Keep the master branch up to date Scaling out code review > Summary
41. Scaling out code review: Summary Easier reviews + more reviewers + higher code visibility - Refuse problematic PRs - Educate reviewers and reviewees - Keep the master branch up to date Scaling out code review > Summary
42. Scaling out code review: Summary Easier reviews + more reviewers + higher code visibility - Ask the reviewee to split PRs, discuss, write a documentation - Educate reviewers and reviewees - Keep the master branch up to date Scaling out code review > Summary
43. Scaling out code review: Summary Easier reviews + more reviewers + higher code visibility - Ask the reviewee to split PRs, discuss, write a documentation - Educate reviewers and reviewees - Keep the master branch up to date Scaling out code review > Summary
44. Scaling out code review: Summary Easier reviews + more reviewers + higher code visibility - Ask the reviewee to split PRs, discuss, write a documentation - Conduct pair/two-phase code review and classroom lectures - Keep the master branch up to date Scaling out code review > Summary
45. Scaling out code review: Summary Easier reviews + more reviewers + higher code visibility - Ask the reviewee to split PRs, discuss, write a documentation - Conduct pair/two-phase code review and classroom lectures - Keep the master branch up to date Scaling out code review > Summary
46. Scaling out code review: Summary Easier reviews + more reviewers + higher code visibility - Ask the reviewee to split PRs, discuss, write a documentation - Conduct pair/two-phase code review and classroom lectures - Use feature flag instead of feature branch Scaling out code review > Summary
47. Topics 1. Scaling out code review 2. What to review 3. Reviewing Kotlin code What to review > Introduction
48. Our main purpose of code review What to review > Introduction
49. Our main purpose of code review Readability What to review > Introduction
50. Our main purpose of code review Readability Correctness is just a "nice to have" What to review > Introduction
51. What to review - Style/conventions/idioms - Natural language - Inner type structure - Dependency What to review > Introduction
52. What to review - Style/conventions/idioms - Natural language - Inner type structure - Dependency What to review > Style/conventions/idioms
53. What to review Style and conventions: - Don't spend too much time - Use formatters and static analysis tools What to review > Style/conventions/idioms
54. What to review Style and conventions: - Don't spend too much time - Use formatters and static analysis tools Idioms: - Use standard libraries as much as possible - Avoid reinventing the wheel What to review > Style/conventions/idioms
55. Reviewing idioms Search standard libraries or google it What to review > Style/conventions/idioms
56. Reviewing idioms Search standard libraries or google it Bad code: var parsedInt = null if (isParseableAsInt(numberString)) { parsedInt = numberString.toInt() } return parsedInt What to review > Style/conventions/idioms
57. Reviewing idioms Search standard libraries or google it Better code: return numberString.toIntOrNull() What to review > Style/conventions/idioms
58. What to review - Style/conventions/idioms - Natural language - Inner type structure - Dependency What to review > Natural language
59. Natural language Summarize code in natural language - Naming - Documentation comment - Inline comment What to review > Natural language
60. Reviewing naming Write what it does/is rather than how/when it's used What to review > Natural language
61. Reviewing naming Write what it does/is rather than how/when it's used Bad naming: fun onUserProfileUpdated(userData: UserData, isInMainPage: Boolean) What to review > Natural language
62. Reviewing naming Write what it does/is rather than how/when it's used Bad naming: fun onUserProfileUpdated(userData: UserData, isInMainPage: Boolean) Better naming: fun updateUserNameView(userData: UserData, showsFullName: Boolean) What to review > Natural language
63. Reviewing documentation Write a short summary to explain the code What to review > Natural language
64. Reviewing documentation Write a short summary to explain the code Bad documentation: /** * Returns null if the given ID is invalid */ What to review > Natural language
65. Reviewing documentation Write a short summary to explain the code Better documentation: /** * Returns a local file path of the given file ID. * This returns null if an invalid file ID is given. */ What to review > Natural language
66. What to review - Style/conventions/idioms - Natural language - Inner type structure - Dependency What to review > Natural language
67. Inner type structure Make class/function responsibility clear - Object state - Function flow What to review > Inner type structure
68. Reviewing object state Remove/encapsulate illegal state What to review > Inner type structure
69. Reviewing object state Remove/encapsulate illegal state Bad class design: class QueryResponse { private val result: Result? private val error: Error? What to review > Inner type structure
70. Reviewing object state Remove/encapsulate illegal state Better class design: sealed class QueryResponse { data class Result(value: String): QueryResponse() data class Error(errorCode: Int): QueryResponse() What to review > Inner type structure
71. Reviewing function flow Extract as a local value or private function What to review > Inner type structure
72. Reviewing function flow Extract as a local value or private function Bad function design: fun startColorAnimation() = ValueAnimator(...).apply { addUpdateListener { // View color update logic } }.start() What to review > Inner type structure
73. Reviewing function flow Extract as a local value or private function Better function design: fun startColorAnimation() { val colorAnimator = ValueAnimator(...) colorAnimator.addUpdateListener(::applyProfileViewColor) colorAnimator.start() } What to review > Inner type structure
74. What to review - Style/conventions/idioms - Natural language - Inner type structure - Dependencies What to review > Natural language
75. Dependencies Prefer weak, acyclic, and explicit dependencies - Coupling (= strength) - Direction What to review > Dependencies
76. Reviewing coupling Encapsulate implementation details What to review > Dependencies
77. Reviewing coupling Encapsulate implementation details Strong coupling: class Calculator { var result: Int var parameter: Int What to review > Dependencies
78. Reviewing coupling Encapsulate implementation details Strong coupling: class Calculator { var result: Int var parameter: Int calculator.parameter = 42 calculator.calculate() val result = calculator.result What to review > Dependencies
79. Reviewing coupling Encapsulate implementation details Relaxed coupling: class Calculator { fun calculate(parameter: Int): Int = ... ... val result = calculator.calculate(42) What to review > Dependencies
80. Reviewing dependency direction Remove unnecessary callbacks What to review > Dependencies
81. Reviewing dependency direction Remove unnecessary callbacks Cyclic dependency: fun loadItemData(presenter: ItemPresenter) { val path = presenter.itemPath ... ... loadItemData(this) What to review > Dependencies
82. Reviewing dependency direction Remove unnecessary callbacks Acyclic dependency: fun loadItemData(itemPath: String) { ... ... loadItemData(this.itemPath) What to review > Dependencies
83. What to review: Summary Readability first: - Correctness is a nice to have What to review > Summary
84. What to review: Summary Readability first: - Correctness is a nice to have Many points to review: - Style/idioms/naming/comments/structure/dependencies... What to review > Summary
85. Topics 1. Scaling out code review 2. What to review 3. Reviewing Kotlin code Reviewing Kotlin code > Introduction
86. Kotlin features - Extension functions - Data classes - Sealed classes - Coroutines - ... Reviewing Kotlin code > Introduction
87. Kotlin features - Extension functions - Data classes - Sealed classes - Coroutines - ... Powerful and helpful to write code Reviewing Kotlin code > Introduction
88. What we should care about Easy to write == easy to read? No, not always. Reviewing Kotlin code > Introduction
89. Drawbacks of Kotlin Easy to make - Top-level extensions that are over-specific - Confusing scope functions - Mutable singletons - Setter properties with large side-effects - ... Reviewing Kotlin code > Introduction
90. Drawbacks of Kotlin Easy to makeɹ - Top-level extensions that are over-specific - Confusing scope functions Reviewing Kotlin code > Top-level extensions
91. Extension function of Kotlin Adding methods to an existing data type fun String.toYourDataType() = ... val yourData = "data text".toYourDataType() Reviewing Kotlin code > Top-level extensions
92. Drawback of top-level extensions IDE shows unrelated and feature-specific extensions Reviewing Kotlin code > Top-level extensions
93. Drawback of top-level extensions IDE shows unrelated and feature-specific extensions Reviewing Kotlin code > Top-level extensions
94. Reviewing top-level extensions Confirm it's natural even if defined in the original class Reviewing Kotlin code > Top-level extensions
95. Reviewing top-level extensions Confirm it's natural even if defined in the original class class String { // This function looks wrong if defined in the class `String` fun toYourData() = ... Reviewing Kotlin code > Top-level extensions
96. Other considerable options Instead of creating a public top-level extension, - Limit the visibility to internal or file/class private - Create an object for a namespace - Change the receiver to the first parameter Reviewing Kotlin code > Top-level extensions
97. Drawbacks of Kotlin Easy to make - Top-level extensions that are over-specific - Confusing scope functions Reviewing Kotlin code > Scope functions
98. Scope functions Function which runs a code block within a context Reviewing Kotlin code > Scope functions
99. Scope functions Function which runs a code block within a context Example of mutable model class initialization: val userData = UserData().apply { name = "FIRST LAST" age = 20 } Reviewing Kotlin code > Scope functions
100. Drawback of scope functions Can easily modify outside of the scope Reviewing Kotlin code > Scope functions
101. Drawback of scope functions Can easily modify outside of the scope Example of confusing scope function: var userData: UserData = ... fun setDefaultUserData() = UserData().apply { name = "FIRST LAST" userData = this } Reviewing Kotlin code > Scope functions
102. Drawback of scope functions Can easily modify outside of the scope Example of confusing scope function: var userData: UserData = ... // Hard to distinguish the side effect fun setDefaultUserData() = UserData().apply { name = "FIRST LAST" userData = this // Hard to find the `userData` scope } Reviewing Kotlin code > Scope functions
103. Reviewing scope functions Confirm all statements in the block require the context Reviewing Kotlin code > Scope functions
104. Reviewing scope functions Confirm all statements in the block require the context Better example: var userData: UserData = ... fun setDefaultUserData() { userData = UserData().apply { name = "FIRST LAST" } Reviewing Kotlin code > Scope functions
105. Reviewing scope functions Confirm all statements in the block require the context Better example: var userData: UserData = ... fun setDefaultUserData() { // Move the outside modification out of the scope function userData = UserData().apply { name = "FIRST LAST" } Reviewing Kotlin code > Scope functions
106. Reviewing Kotlin code: Summary Use Kotlin language features to make code readable - Don't make "using features" an objective - Catch language feature overkill Reviewing Kotlin code > Summary
107. Summary of this talk Summary
108. Summary of this talk To scale out code review: - Make review easy and code visible; educate reviewers Summary
109. Summary of this talk To scale out code review: - Make review easy and code visible; educate reviewers What to review: - Readability first - Not only style/idioms, but also naming/comments/structure... - Kotlin language feature usage Summary