Code review at scale

Code review at scale

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

53850955f15249a1a9dc49df6113e400?s=128

LINE Developers

July 18, 2019
Tweet

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