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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Bonus points of two phase code review Filters out problematic PRs - Rebalances senior engineer's work load Scaling out code review > Reviewer/reviewee education
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
Bonus point of classroom lectures Can review with a link Can omit reason, details, and background knowledge Scaling out code review > Reviewer/reviewee education
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Reviewing coupling Encapsulate implementation details Strong coupling: class Calculator { var result: Int var parameter: Int What to review > Dependencies
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
Reviewing coupling Encapsulate implementation details Relaxed coupling: class Calculator { fun calculate(parameter: Int): Int = ... ... val result = calculator.calculate(42) What to review > Dependencies
Reviewing dependency direction Remove unnecessary callbacks Cyclic dependency: fun loadItemData(presenter: ItemPresenter) { val path = presenter.itemPath ... ... loadItemData(this) What to review > Dependencies
Reviewing dependency direction Remove unnecessary callbacks Acyclic dependency: fun loadItemData(itemPath: String) { ... ... loadItemData(this.itemPath) What to review > Dependencies
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
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
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
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
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
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
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
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
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
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
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
line_developers
shimashima35
dskst
tgalopin
nenonaninu
lmi
iwashi86
hirosatogamo
megabitsenmzq
nasa9084
hanhan1978
dena_tech
hayatokudou
bkeepers
rocio
revolveconf
carmenintech
eileencodes
sachag
chriscoyier
afnizarnur
scottboms
mojombo
sugarenia
destraynor