Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Improving quality of reviews: case study of LINE for Android

Improving quality of reviews: case study of LINE for Android

Munetoshi Ishikawa, LINE Corporation

A3966f193f4bef226a0d3e3c1f728d7f?s=128

LINE Developers
PRO

December 11, 2021
Tweet

More Decks by LINE Developers

Other Decks in Technology

Transcript

  1. Munetoshi Ishikawa, LINE Corporation Improving quality of reviews: case study

    of LINE for Android
  2. Review for code quality Correctness - Pitfalls: Android API, language,

    mutual execution… - Requirements: Security, performance… Readability - Style, format, convention… - Class/module structure, dependency…
  3. How would you review the following code? suspend fun incrementViewCount()

    = } Suspicious coroutine code withContext(singleThreadDispatcher) { val count = repository.getCount() repository.setCount(count + 1) private val singleThreadDispatcher = Executors.newSingleThreadExecutor().asCoroutineDispatcher()
  4. Why a single thread does not work Race condition can

    happen if repository.setCount is suspend… - First call suspends during repository.setCount - Second call executes repository.getCount synchronized won’t work as expected for the same reason withContext(singleThreadDispatcher) { val count = repository.getCount() repository.setCount(count + 1)
  5. How to comment Show options to fix this problem -

    Use kotlinx.coroutines.sync.Mutex - Change the repository to provide transaction - Make an atomic interface to accumulate
  6. Can one of your team members discover all the problematic

    code?
  7. Can all of your team members discover all the problematic

    code?
  8. Dilemma on code review - All code needs to be

    reviewed to a high standard - Reviewer’s skill level is not consistent Not realistic to focus reviews on one super engineer
  9. - Sharing knowledge - Drawing up of rules and guidelines

    - Patterning by libraries Our efforts for improving code reviews
  10. - Sharing knowledge - Drawing up of rules and guidelines

    - Patterning by libraries Our efforts for improving code reviews
  11. Sharing knowledge: Review randomization 1/3 People tend to ask for

    reviews from people they are already friendly with 
 Limited knowledge sharing Team A Team B code review code review “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/
  12. Sharing knowledge: Review randomization 2/3 Two phase review - First

    reviewer: Someone who knows the function/module - Second reviewer: Randomly selected Team A Team B first review second review “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/
  13. Sharing knowledge: Review randomization 3/3 Bot chooses a second reviewer

    randomly Team A Team B first review “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ request Random assign bot notify
  14. Sharing knowledge: In-team discussion Discussion after (virtual) daily stand-up meeting

    Team B “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Discussion
  15. Our efforts for improving code reviews - Sharing knowledge -

    Drawing up of rules and guidelines - Patterning by libraries
  16. Rules and guidelines Objective - Support learning best-practices - Avoid

    bike-shedding discussions Implementations - Coding convention for Kotlin/Java - Coroutine guidelines - Modularization guidelines
  17. Example: CancellationException Caller code runs unintentionally if CancellationException caught -

    Pay highly attention to T.runCatching
  18. Guideline: “How to use Kotlin Coroutines” - Explanations of typical

    usages - Notices about pitfalls Document links can be useful for
 review comments
  19. Rule enforcement by CI Custom SonarQube rule and Lint -

    Helps reviews by automatically finding and reporting problems - Requires authors and reviewers to follow the rules - Gives hints about how to fix
  20. Review support by CI: Example Banned: Triple nested lambda -

    Easy to make a deep nest because of extension and scope functions Bot review comment produced by a custom SonarQube rule
  21. Our efforts for improving code reviews - Sharing knowledge -

    Drawing up of rules and guidelines - Patterning by libraries
  22. Patterning by libraries Objective - Enforce correct API usage and

    architecture - Make controversial points less common Implementation - Lich (https://github.com/line/lich), Aug 2019 first release
  23. Example: Pitfall of launchWhenStarted Flow.collect is unsafe in launchWhenStarted -

    The coroutine won’t be cancelled when “stopped” - May cause resource leak lifecycleScope.launchWhenStarted { sensorDataEmitter.asFlow().collect { sensorValue -> // Use sensorValue } }
  24. Alternatives to launchWhenStarted 1/2 Lifecycle.repeatOnLifecycle is a good replacement -

    Stable release was on Oct 27, 2021 (… We could not wait)
  25. Alternatives to launchWhenStarted 2/2 AutoResetLifecycleScope of Lich private val autoResetLifecycleScope:

    CoroutineScope = AutoResetLifecycleScope(this) // Lifecycle or the owner … autoResetLifecycleScope.launch { sensorDataEmitter.asFlow().collect { sensorValue -> // Use sensorValue } }
  26. Other patterns offered by Lich Lich component (cf. https://www.youtube.com/watch?v=5PNIVPi_0-A) -

    Modularization - Repository layer structure Lich savedstate (cf. https://engineering.linecorp.com/ja/blog/lich-viewmodel-2-0/) - Data handling via Bundle
  27. Summary Improve review quality by: - Making skill level consistent

    through knowledge sharing - Following rules and guidelines provides an easier review process - Shortening design discussions by using pre-accepted libraries