Improving quality of reviews: case study of LINE for Android

Munetoshi Ishikawa, LINE Corporation

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