Code review at scale - Speaker Deck

Slide 1

Slide 1 text

Code review at scaleɹɹ ɹ A case study of LINE for Androidɹɹ Munetoshi Ishikawa LINE Developer Meetup #55 in Fukuoka - Jul. 18, 2019

Slide 2

Slide 2 text

Topics 1. Scaling out code review 2. What to review 3. Reviewing Kotlin code

Slide 3

Slide 3 text

Topics 1. Scaling out code review 2. What to review 3. Reviewing Kotlin code Scaling out code review > Introduction

Slide 4

Slide 4 text

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

Slide 5

Slide 5 text

Slide 6

Slide 6 text

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

Slide 7

Slide 7 text

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

Slide 8

Slide 8 text

How to scale out review process Easier reviews + more reviewers + higher code visibility Scaling out code review > Introduction

Slide 9

Slide 9 text

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

Slide 10

Slide 10 text

Slide 11

Slide 11 text

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

Slide 12

Slide 12 text

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

Slide 13

Slide 13 text

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

Slide 14

Slide 14 text

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

Slide 15

Slide 15 text

How to split a PR Top down / bottom up approach Scaling out code review > Refuse a problematic PR

Slide 16

Slide 16 text

Slide 17

Slide 17 text

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 ﬁrst data class UserProfileData(val userId: Long, val name: String, ...) Scaling out code review > Refuse a problematic PR

Slide 18

Slide 18 text

Slide 19

Slide 19 text

Slide 20

Slide 20 text

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

Slide 21

Slide 21 text

Slide 22

Slide 22 text

Pair code review Ofﬂine discussion leads to good review comments Scaling out code review > Reviewer/reviewee education

Slide 23

Slide 23 text

Pair code review Ofﬂine 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

Slide 24

Slide 24 text

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

Slide 25

Slide 25 text

Slide 26

Slide 26 text

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

Slide 27

Slide 27 text

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

Slide 28

Slide 28 text

Bonus points of two phase code review Filters out problematic PRs - Rebalances senior engineer's work load Scaling out code review > Reviewer/reviewee education

Slide 29

Slide 29 text

Bonus points of two phase code review Filters out problematic PRs - Rebalances senior engineer's work load Guarantees few superﬁcial problems - Can focus on fundamental review Scaling out code review > Reviewer/reviewee education

Slide 30

Slide 30 text

Slide 31

Slide 31 text

Classroom lectures Sessions for code quality and conventions Scaling out code review > Reviewer/reviewee education

Slide 32

Slide 32 text

Bonus point of classroom lectures Can review with a link Scaling out code review > Reviewer/reviewee education

Slide 33

Slide 33 text

Bonus point of classroom lectures Can review with a link Can omit reason, details, and background knowledge Scaling out code review > Reviewer/reviewee education

Slide 34

Slide 34 text

Slide 35

Slide 35 text

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

Slide 36

Slide 36 text

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

Slide 37

Slide 37 text

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

Slide 38

Slide 38 text

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 ﬂags instead of feature branch - Merge back release branches to the master branch periodically Scaling out code review > Keep the master branch up to date

Slide 39

Slide 39 text

Feature ﬂag Refer to ڊେͳΞϓϦͷ։ൃΛࢧ͑Δϑϥά؅ཧज़ presented at DroidKaigi 2019 Scaling out code review > Keep the master branch up to date

Slide 40

Slide 40 text

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

Slide 41

Slide 41 text

Slide 42

Slide 42 text

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

Slide 43

Slide 43 text

Slide 44

Slide 44 text

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

Slide 45

Slide 45 text

Slide 46

Slide 46 text

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 ﬂag instead of feature branch Scaling out code review > Summary

Slide 47

Slide 47 text

Topics 1. Scaling out code review 2. What to review 3. Reviewing Kotlin code What to review > Introduction

Slide 48

Slide 48 text

Our main purpose of code review What to review > Introduction

Slide 49

Slide 49 text

Our main purpose of code review Readability What to review > Introduction

Slide 50

Slide 50 text

Our main purpose of code review Readability Correctness is just a "nice to have" What to review > Introduction

Slide 51

Slide 51 text

What to review - Style/conventions/idioms - Natural language - Inner type structure - Dependency What to review > Introduction

Slide 52

Slide 52 text

What to review - Style/conventions/idioms - Natural language - Inner type structure - Dependency What to review > Style/conventions/idioms

Slide 53

Slide 53 text

What to review Style and conventions: - Don't spend too much time - Use formatters and static analysis tools What to review > Style/conventions/idioms

Slide 54

Slide 54 text

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

Slide 55

Slide 55 text

Reviewing idioms Search standard libraries or google it What to review > Style/conventions/idioms

Slide 56

Slide 56 text

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

Slide 57

Slide 57 text

Reviewing idioms Search standard libraries or google it Better code: return numberString.toIntOrNull() What to review > Style/conventions/idioms

Slide 58

Slide 58 text

What to review - Style/conventions/idioms - Natural language - Inner type structure - Dependency What to review > Natural language

Slide 59

Slide 59 text

Natural language Summarize code in natural language - Naming - Documentation comment - Inline comment What to review > Natural language

Slide 60

Slide 60 text

Reviewing naming Write what it does/is rather than how/when it's used What to review > Natural language

Slide 61

Slide 61 text

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

Slide 62

Slide 62 text

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

Slide 63

Slide 63 text

Reviewing documentation Write a short summary to explain the code What to review > Natural language

Slide 64

Slide 64 text

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

Slide 65

Slide 65 text

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

Slide 66

Slide 66 text

What to review - Style/conventions/idioms - Natural language - Inner type structure - Dependency What to review > Natural language

Slide 67

Slide 67 text

Inner type structure Make class/function responsibility clear - Object state - Function ﬂow What to review > Inner type structure

Slide 68

Slide 68 text

Reviewing object state Remove/encapsulate illegal state What to review > Inner type structure

Slide 69

Slide 69 text

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

Slide 70

Slide 70 text

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

Slide 71

Slide 71 text

Reviewing function ﬂow Extract as a local value or private function What to review > Inner type structure

Slide 72

Slide 72 text

Reviewing function ﬂow 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

Slide 73

Slide 73 text

Reviewing function ﬂow 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

Slide 74

Slide 74 text

What to review - Style/conventions/idioms - Natural language - Inner type structure - Dependencies What to review > Natural language

Slide 75

Slide 75 text

Dependencies Prefer weak, acyclic, and explicit dependencies - Coupling (= strength) - Direction What to review > Dependencies

Slide 76

Slide 76 text

Reviewing coupling Encapsulate implementation details What to review > Dependencies

Slide 77

Slide 77 text

Reviewing coupling Encapsulate implementation details Strong coupling: class Calculator { var result: Int var parameter: Int What to review > Dependencies

Slide 78

Slide 78 text

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

Slide 79

Slide 79 text

Reviewing coupling Encapsulate implementation details Relaxed coupling: class Calculator { fun calculate(parameter: Int): Int = ... ... val result = calculator.calculate(42) What to review > Dependencies

Slide 80

Slide 80 text

Reviewing dependency direction Remove unnecessary callbacks What to review > Dependencies

Slide 81

Slide 81 text

Reviewing dependency direction Remove unnecessary callbacks Cyclic dependency: fun loadItemData(presenter: ItemPresenter) { val path = presenter.itemPath ... ... loadItemData(this) What to review > Dependencies

Slide 82

Slide 82 text

Reviewing dependency direction Remove unnecessary callbacks Acyclic dependency: fun loadItemData(itemPath: String) { ... ... loadItemData(this.itemPath) What to review > Dependencies

Slide 83

Slide 83 text

What to review: Summary Readability ﬁrst: - Correctness is a nice to have What to review > Summary

Slide 84

Slide 84 text

What to review: Summary Readability ﬁrst: - Correctness is a nice to have Many points to review: - Style/idioms/naming/comments/structure/dependencies... What to review > Summary

Slide 85

Slide 85 text

Topics 1. Scaling out code review 2. What to review 3. Reviewing Kotlin code Reviewing Kotlin code > Introduction

Slide 86

Slide 86 text

Kotlin features - Extension functions - Data classes - Sealed classes - Coroutines - ... Reviewing Kotlin code > Introduction

Slide 87

Slide 87 text

Kotlin features - Extension functions - Data classes - Sealed classes - Coroutines - ... Powerful and helpful to write code Reviewing Kotlin code > Introduction

Slide 88

Slide 88 text

What we should care about Easy to write == easy to read? No, not always. Reviewing Kotlin code > Introduction

Slide 89

Slide 89 text

Drawbacks of Kotlin Easy to make - Top-level extensions that are over-speciﬁc - Confusing scope functions - Mutable singletons - Setter properties with large side-effects - ... Reviewing Kotlin code > Introduction

Slide 90

Slide 90 text

Drawbacks of Kotlin Easy to makeɹ - Top-level extensions that are over-speciﬁc - Confusing scope functions Reviewing Kotlin code > Top-level extensions

Slide 91

Slide 91 text

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

Slide 92

Slide 92 text

Drawback of top-level extensions IDE shows unrelated and feature-speciﬁc extensions Reviewing Kotlin code > Top-level extensions

Slide 93

Slide 93 text

Drawback of top-level extensions IDE shows unrelated and feature-speciﬁc extensions Reviewing Kotlin code > Top-level extensions

Slide 94

Slide 94 text

Reviewing top-level extensions Conﬁrm it's natural even if deﬁned in the original class Reviewing Kotlin code > Top-level extensions

Slide 95

Slide 95 text

Reviewing top-level extensions Conﬁrm it's natural even if deﬁned in the original class class String { // This function looks wrong if defined in the class `String` fun toYourData() = ... Reviewing Kotlin code > Top-level extensions

Slide 96

Slide 96 text

Other considerable options Instead of creating a public top-level extension, - Limit the visibility to internal or ﬁle/class private - Create an object for a namespace - Change the receiver to the ﬁrst parameter Reviewing Kotlin code > Top-level extensions

Slide 97

Slide 97 text

Drawbacks of Kotlin Easy to make - Top-level extensions that are over-speciﬁc - Confusing scope functions Reviewing Kotlin code > Scope functions

Slide 98

Slide 98 text

Scope functions Function which runs a code block within a context Reviewing Kotlin code > Scope functions

Slide 99

Slide 99 text

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

Slide 100

Slide 100 text

Drawback of scope functions Can easily modify outside of the scope Reviewing Kotlin code > Scope functions

Slide 101

Slide 101 text

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

Slide 102

Slide 102 text

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

Slide 103

Slide 103 text

Reviewing scope functions Conﬁrm all statements in the block require the context Reviewing Kotlin code > Scope functions

Slide 104

Slide 104 text

Reviewing scope functions Conﬁrm 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

Slide 105

Slide 105 text

Reviewing scope functions Conﬁrm 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

Slide 106

Slide 106 text

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

Slide 107

Slide 107 text

Summary of this talk Summary

Slide 108

Slide 108 text

Summary of this talk To scale out code review: - Make review easy and code visible; educate reviewers Summary

Slide 109

Slide 109 text

Summary of this talk To scale out code review: - Make review easy and code visible; educate reviewers What to review: - Readability ﬁrst - Not only style/idioms, but also naming/comments/structure... - Kotlin language feature usage Summary