Confronting quality problems of long-lived codebase

長く生きるコードベースの「品質」問題に向き合う

Munetoshi Ishikawa, LINE Corporation

Transcript

1. Munetoshi Ishikawa, LINE Corporation Confronting quality problems of long-lived codebase ௕͘ੜ͖Δίʔυϕʔεͷʮ඼࣭ʯ໰୊ʹ޲͖߹͏
2. Long-lived codebase Successful / important product - Many developers - Many lines of code - Long history Codebase Developers Team A Team B history “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/
3. Communication app, “LINE” for Android • Started 10 years ago • More than 80 active developers • Many LoC - Kotlin: 1.6M LoC, 15k Files - Java: 0.81M LoC, 4.8k Files
4. Problems of a long-lived codebase 1/2 Loose "context" of source code - Only author and reviewers know Team A Team B merge fetch request review Codebase “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/
5. Independent teams making silos - Can be an obstacle against updating a rule, library, or best-practice Team A Team B Team C library rule Problems of a long-lived codebase 2/2
6. Objectives Record for history and background Understandable code without context Break silos for knowledge and develop culture sharing
7. Our effort 1. Two phase review 2. Design doc 3. “Code readability” presentation 4. Tech consultant 5. Review committee
8. Our effort 1. Two phase review 2. Design doc 3. “Code readability” presentation 4. Tech consultant 5. Review committee
9. Two phase review: Problem Inflexible relationships between authors and reviewers Team A Team B code review code review “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/
10. Two phase review: Concept Review by someone who does not know the code - First reviewer: Normal review - Second reviewer: Readability review Team A Team B first review second review “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/
11. Two phase review: Process 1. Conduct “normal” review 2. Assign reviewer randomly after review Team A Team B first review request Random assign bot notify “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/
12. Two phase review: Benefits • Code readability check without “context” • Feedback to reviewers • Inter-team knowledge sharing
13. Our effort 1. Two phase review 2. Design doc 3. “Code readability” presentation 4. Tech consultant 5. Review committee
14. May overlook architecture design issue only with code review Design doc: Problem LGTM LGTM LGTM + + “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/
15. Design doc: Concept Discuss on design before implementation - Makes technical issues clear - No concrete format - Can be updated during implementation Design doc Implement
16. Design doc: Contents Objective Background Abstract design Design details Concerns, known issues Our design doc example for refactoring
17. Design doc: Our tips Not mandatory: write only for middle to long task Small step to start: don’t try to create a perfect doc before discussion Traceable: put link to pull request
18. Our effort 1. Two phase review 2. Design doc 3. “Code readability” presentation 4. Tech consultant 5. Review committee
19. "Code readability" presentation: Problem Need to share knowledge of good code and structure - In order to make “two phase review” and “design doc” effective
20. "Code readability" presentation 8 hour sessions for ideas to improve code readability Content: naming, state management, dependency… Code Readability Munetoshi Ishikawa https://speakerdeck.com/munetoshi/code-readability
21. "Code readability" presentation: Example 1/2 “Split by object”: Two ways to split a function condition or object Account type
 
 UI Normal Premium Background Gray Gold Account icon Should split function for each UI, not account type “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/
22. "Code readability" presentation: Example 2/2 Benefits of “Split by object”: readability, robustness, and further refactoring Case 1: split by condition fun updateAccountViews(accountType: AccountType) = when(accountType) { AccountType.NORMAL -> updateViewsForNormalAccount() AccountType.PREMIUM -> updateViewsForPremiumAccount() }
 Case 2: split by object fun updateAccountViews(accountType: AccountType) { updateBackgroundColor(accountType) updateIconImage(accountType) }
23. "Code readability" presentation: Achievement Available as - External resource: https://speakerdeck.com/munetoshi/code-readability - In-company trainings: LINE Class, Z Academia
24. Our effort 1. Two phase review 2. Design doc 3. “Code readability” presentation 4. Tech consultant 5. Review committee
25. Tech consultant: Problem Hard to standardize skills and cultures - Architectural design, rules, best practices, libraries, process
26. Tech consultant: Basic idea A senior engineer joins a team - Reviews pull requests thoroughly - Shares feedbacks: knowledge, skill, process Team senior eng. review request feedback write documents update structure “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/
27. Tech consultant: Check list Project rule: conventions, platform usage… Task management: discussion, design doc, scheduling… Architectural design: layers, dependencies, state… Code details: naming, logic flow, correctness…
28. Our effort 1. Two phase review 2. Design doc 3. “Code readability” presentation 4. Tech consultant 5. Review committee
29. Review committee: Problem Requires knowledge sharing with all teams - Tech consulting is for only one team
30. Review committee: Basic idea 1. A team voluntarily picks a “merged” pull-request once per week 2. A senior engineer reviews the picked pull-request 3. The senior engineer then writes and shares a weekly report Team A Team B senior eng. send a merged PR feedback write report share “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/
31. Review committee: Report example Smell: sealed class without “a child specific property” sealed class AccountType(val accountId: Long, val accountName: String) { class NormalAccount(accountId: Long, accountName: String) : AccountType(accountId, accountName) class PremiumAccount(accountId: Long, accountName: String) : AccountType(accountId, accountName) } Fixed code: extract type as enum class AccountModel(val id: Long, val name: String, val type: AccountType) enum class AccountType { NORMAL, PREMIUM }
32. Our effort 1. Two phase review 2. Design doc 3. “Code readability” presentation 4. Tech consultant 5. Review committee
33. Future plan One misleading point in this presentation “A senior engineer” in “tech consulting” and “review committee”
 == only me Need to remove single point of failure and scale out
34. Summary Efforts improving quality for long-lived code - Problem: Code context and siloing of teams” - Five solutions Current culture - Improve everyone’s skill level effectively - Improve everyone’s development environment