Code Review Challenge An example of a solution

Code Review Challenge An example of a solution
DroidKaigi 2022: コードレビューチャレンジの問題解説

Droid Kaigi 2022のオフラインブースで行う予定のCode Review Challengeの裏側(問題準備など)とそれを支えたLINEのレビュー文化について紹介する予定です。

安藤 祐貴 (Yuki Ando) / LINE株式会社 / iOS/Androidエクスペリエンス開発チーム Software(Android) Engineer
2021年にLINEに新卒入社し、LINEアプリのAndroidクライアントについて、主にAndroid OSの新機能を適用する業務に携わっています。車を走らせることが大好きです。
Twitter: https://twitter.com/m1zyuk1

Transcript

1. Yuki Ando, LINE Corporation Code Review Challenge An example of a solution DroidKaigi 2022: ίʔυϨϏϡʔνϟϨϯδͷ໰୊ղઆ
2. Self-introduction Name: Yuki Ando / ҆౻ ༞و
 Twitter: @m1zyuk1 Speaker of DroidKaigi 2022 “࣮ફ `edge-to-edge`: ୺͔Β୺·Ͱղઆ” Hobbies: Car, Game Main activity: 
 Apply new Android features to LINE Android
3. Did you participate in DroidKaigi 2022 offline?
4. Yes I did “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/
5. Have you participated in the Code Review Challenge?
8. What is the Code Review Challenge?
9. Code Review Challenge Review “Bad Code” to become “Good Code” LINE Booth in DroidKaigi 2022 class Ugly { fun dirtyFunc() { /* */ } } Challengers “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/
10. Code Review Challenge LINE Booth in DroidKaigi 2022 class Ugly { fun dirtyFunc() { /* */ } } Challengers Review “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Review “Bad Code” to become “Good Code”
11. Code Review Challenge LINE Booth in DroidKaigi 2022 class Ugly { fun dirtyFunc() { /* */ } } Challengers Review “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Review “Bad Code” to become “Good Code”
12. Code Review Challenge LINE Booth in DroidKaigi 2022 class Ugly { fun dirtyFunc() { /* */ } } Challengers Review “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Review “Bad Code” to become “Good Code”
13. Code Review Challenge LINE Booth in DroidKaigi 2022 class Ugly { fun dirtyFunc() { /* */ } } Review Review ideas Challengers “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Review “Bad Code” to become “Good Code”
14. Code Review Challenge LINE Booth in DroidKaigi 2022 class Ugly { fun dirtyFunc() { /* */ } } class Good { fun smartFunc() { /* */ } } Review Review ideas Challengers “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Review “Bad Code” to become “Good Code”
15. Code Review Challenge LINE Booth in DroidKaigi 2022 class Ugly { fun dirtyFunc() { /* */ } } class Good { fun smartFunc() { /* */ } } Review Review ideas Challengers “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Review “Bad Code” to become “Good Code”
16. Code Review Challenge LINE Booth in DroidKaigi 2022 class Ugly { fun dirtyFunc() { /* */ } } class Good { fun smartFunc() { /* */ } } Review Review ideas Challengers “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Review “Bad Code” to become “Good Code”
17. Code Review Challenge LINE Booth in DroidKaigi 2022 class Ugly { fun dirtyFunc() { /* */ } } class Good { fun smartFunc() { /* */ } } Review Review ideas Challengers “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Review “Bad Code” to become “Good Code”
18. Code Review Challenge LINE Booth in DroidKaigi 2022 class Ugly { fun dirtyFunc() { /* */ } } class Good { fun smartFunc() { /* */ } } Review Review ideas Challengers “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Review “Bad Code” to become “Good Code”
19. Code Review Challenge LINE Booth in DroidKaigi 2022 class Good { fun smartFunc() { /* */ } } Review ideas Participants “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Review “Bad Code” to become “Good Code”
20. Code Review Challenge LINE Booth in DroidKaigi 2022 class Good { fun smartFunc() { /* */ } } Review ideas Participants “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Review “Bad Code” to become “Good Code”
21. Code Review Challenge LINE Booth in DroidKaigi 2022 class Good { fun smartFunc() { /* */ } } Review ideas Participants “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Review “Bad Code” to become “Good Code”
22. Code Review Challenge LINE Booth in DroidKaigi 2022 class Good { fun smartFunc() { /* */ } } Review ideas Participants “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Review “Bad Code” to become “Good Code”
23. Code Review Challenge LINE Booth in DroidKaigi 2022 class Good { fun smartFunc() { /* */ } } Review ideas Participants “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ x2 Review “Bad Code” to become “Good Code”
24. Code Review Challenge LINE Booth in DroidKaigi 2022 class Good { fun smartFunc() { /* */ } } Review ideas Participants “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ x2 Review “Bad Code” to become “Good Code”
25. Code Review Challenge LINE Booth in DroidKaigi 2022 class Good { fun smartFunc() { /* */ } } Review ideas Participants “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ x2 Review “Bad Code” to become “Good Code”
26. Code Review Challenge LINE Booth in DroidKaigi 2022 class Good { fun smartFunc() { /* */ } } Review ideas Participants “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ x3 Review “Bad Code” to become “Good Code”
27. Explain the assumed solutions
28. Full code is hereɹɹɹ Ref: https://engineering.linecorp.com/ja/blog/code_review_challenge/ “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/
29. Questions 1. Everything is something 2. Split by object, not condition 3. Truth in deep nests 4. What layout state class was really needed? 5. Coroutine's hard-to-understand trap that is easy to use for us
30. Everything is something There are two presenter classes which have several name providers class friend_item_Presenter( private val friendNameProvider: FriendNameProvider, private val coroutineScope: CoroutineScope ) { fun updateViews(itemId: String) { //…snip } } class BotItemPresenter( private val nameProvider: NameProvider, private val coroutineScope: CoroutineScope ) { fun updateViews(itemId: String) { //…snip } } interface NameProvider { suspend fun getName(uuid: String, contactType: ContactType): String? } class FriendNameProvider(val friendRepository: FriendRepository) : NameProvider { override suspend fun getName(uuid: String, contactType: ContactType): String { //…snip } } class BotNameProvider(private val botRepository: BotRepository) : NameProvider { override suspend fun getName(uuid: String, contactType: ContactType): String? { //…snip } }
31. Consider the case of more types Logic breaks down when more ContactType are added. class BotNameProvider( private val botRepository: BotRepository ) : NameProvider { override suspend fun getName(uuid: String, contactType: ContactType): String? { if (contactType == ContactType.Friend) { return null } val name = botRepository.getSuperCoolBotName(uuid) return name } }
32. Consider the case of more types Logic breaks down when more ContactType are added. class BotNameProvider( private val botRepository: BotRepository ) : NameProvider { override suspend fun getName(uuid: String, contactType: ContactType): String? { if (contactType == ContactType.Friend) { return null } val name = botRepository.getSuperCoolBotName(uuid) return name } }
33. Consider the case of more types Logic breaks down when more ContactType are added. class BotNameProvider( private val botRepository: BotRepository ) : NameProvider { override suspend fun getName(uuid: String, contactType: ContactType): String? { if (contactType == ContactType.Friend) { return null } val name = botRepository.getSuperCoolBotName(uuid) return name } } sealed class ContactType { object Friend : ContactType() object Bot : ContactType() }
34. Consider the case of more types Logic breaks down when more ContactType are added. class BotNameProvider( private val botRepository: BotRepository ) : NameProvider { override suspend fun getName(uuid: String, contactType: ContactType): String? { if (contactType == ContactType.Friend) { return null } val name = botRepository.getSuperCoolBotName(uuid) return name } } sealed class ContactType { object Friend : ContactType() object Bot : ContactType() object Official : ContactType() }
35. Consider the case of more types Logic breaks down when more ContactType are added. // With O ff i cial type class BotNameProvider( private val botRepository: BotRepository ) : NameProvider { override suspend fun getName(uuid: String, contactType: ContactType): String? { if (contactType == ContactType.Friend) { return null } val name = botRepository.getSuperCoolBotName(uuid) return name } }
36. Consider the case of more types Logic breaks down when more ContactType are added. class BotNameProvider( private val botRepository: BotRepository ) : NameProvider { override suspend fun getName(uuid: String, contactType: ContactType): String? { if (contactType != ContactType.Bot) { return null } val name = botRepository.getSuperCoolBotName(uuid) return name } }
37. Consider the case of more types Logic breaks down when more ContactType are added. class BotNameProvider( private val botRepository: BotRepository ) : NameProvider { override suspend fun getName(uuid: String, contactType: ContactType): String? { if (contactType == ContactType.Bot) { val name = botRepository.getSuperCoolBotName(uuid) return name } return null } }
38. Everything is something Easily broken due to lack of type constraints class BotItemPresenter( private val nameProvider: NameProvider, private val coroutineScope: CoroutineScope ) { //…snip }
39. Everything is something Easily broken due to lack of type constraints class BotItemPresenter( private val nameProvider: NameProvider, private val coroutineScope: CoroutineScope ) { //…snip }
40. Everything is something Easily broken due to lack of type constraints class BotItemPresenter( private val nameProvider: BotNameProvider, private val coroutineScope: CoroutineScope ) { //…snip } class BotNameProvider( private val botRepository: BotRepository ) : NameProvider { //…snip }
41. Everything is something Don’t make unnecessary interface interface NameProvider { suspend fun getName(uuid: String, contactType: ContactType): String? } class FriendNameProvider(val friendRepository: FriendRepository) : NameProvider { override suspend fun getName(uuid: String, contactType: ContactType): String { //…snip } } class BotNameProvider(private val botRepository: BotRepository) : NameProvider { override suspend fun getName(uuid: String, contactType: ContactType): String? { //…snip } }
42. Everything is something Don’t make unnecessary interface interface NameProvider { suspend fun getName(uuid: String, contactType: ContactType): String? } class FriendNameProvider(val friendRepository: FriendRepository) : NameProvider { override suspend fun getName(uuid: String, contactType: ContactType): String { //…snip } } class BotNameProvider(private val botRepository: BotRepository) : NameProvider { override suspend fun getName(uuid: String, contactType: ContactType): String? { //…snip } } val name = nameProvider.getName(itemId, ContactType.Bot) ?: return@launch nameTextView.text = friendNameProvider.getName(itemId, ContactType.Friend)
43. Everything is something Don’t make unnecessary interface class BotNameProvider( private val botRepository: BotRepository ) : NameProvider { override suspend fun getName(uuid: String, contactType: ContactType): String? { if (contactType == ContactType.Friend) { return null } val name = botRepository.getSuperCoolBotName(uuid) return name } }
44. Everything is something Don’t make unnecessary interface class BotNameProvider( private val botRepository: BotRepository ) { suspend fun getBotName(uuid: String): String? { val name = botRepository.getSuperCoolBotName(uuid) return name } }
45. Questions 1. Everything is something 2. Split by object, not condition 3. Truth in deep nests 4. What layout state class was really needed? 5. Coroutine's hard-to-understand trap that is easy to use for us
46. Split by object, not condition There is a presenter class which handles playing media with progress bar class PlayerPresenter( private val loadingView: View, private val playButton: View, private val pauseButton: View, private val progressView: TextView, private val player: Player, private val lifecycleScope: CoroutineScope ) { private var progressUpdateJob: Job? = null init { showLoadingView() lifecycleScope.launch { withContext(Dispatchers.IO) { player.prepare() } showPausingView() } } //…snip
47. Split by object, not condition The view update process is divided by display status class PlayerPresenter( private val loadingView: View, private val playButton: View, private val pauseButton: View, private val progressView: TextView, private val player: Player, private val lifecycleScope: CoroutineScope ) { private var progressUpdateJob: Job? = null init { showLoadingView() lifecycleScope.launch { withContext(Dispatchers.IO) { player.prepare() } showPausingView() } } //…snip
48. Split by object, not condition The view update process is divided by display status private fun showLoadingView() { loadingView.isVisible = true playButton.isVisible = false pauseButton.isVisible = false progressView.isVisible = false } private fun showPausingView() { loadingView.isVisible = false playButton.isVisible = true playButton.setOnClickListener { player.play() showPLayingView() } pauseButton.isVisible = false progressView.isVisible = true progressUpdateJob?.cancel() }
49. Split by object, not condition The view update process is divided by display status private fun showLoadingView() { loadingView.isVisible = true playButton.isVisible = false pauseButton.isVisible = false progressView.isVisible = false } private fun showPausingView() { loadingView.isVisible = false playButton.isVisible = true playButton.setOnClickListener { player.play() showPLayingView() } pauseButton.isVisible = false progressView.isVisible = true progressUpdateJob?.cancel() } class PlayerPresenter( private val loadingView: View, private val playButton: View, private val pauseButton: View, private val progressView: TextView, private val player: Player, private val lifecycleScope: CoroutineScope )
50. Split by object, not condition The view update process is divided by display status private fun showLoadingView() { loadingView.isVisible = true playButton.isVisible = false pauseButton.isVisible = false progressView.isVisible = false } private fun showPausingView() { loadingView.isVisible = false playButton.isVisible = true playButton.setOnClickListener { player.play() showPLayingView() } pauseButton.isVisible = false progressView.isVisible = true progressUpdateJob?.cancel() } class PlayerPresenter( private val loadingView: View, private val playButton: View, private val pauseButton: View, private val progressView: TextView, private val someAddedView: View, private val player: Player, private val lifecycleScope: CoroutineScope )
51. Split by object, not condition The view update process is divided by display status private fun showLoadingView() { loadingView.isVisible = true playButton.isVisible = false pauseButton.isVisible = false progressView.isVisible = false // Maintenance Forgotten } private fun showPausingView() { loadingView.isVisible = false playButton.isVisible = true playButton.setOnClickListener { player.play() showPLayingView() } pauseButton.isVisible = false progressView.isVisible = true progressUpdateJob?.cancel() // Maintenance Forgotten } class PlayerPresenter( private val loadingView: View, private val playButton: View, private val pauseButton: View, private val progressView: TextView, private val someAddedView: View, private val player: Player, private val lifecycleScope: CoroutineScope )
52. Split by object, not condition Change the method of function division private fun updateView(viewState: ViewState) { loadingView.isVisible = viewState == ViewState.Loading progressView.isVisible = viewState == ViewState.Loading playButton.isVisible = viewState == ViewState.Pausing pauseButton.isVisible = viewState == ViewState.Playing }
53. Split by object, not condition Apply strategy pattern private fun updateView(viewState: ViewState) { loadingView.isVisible = viewState == ViewState.Loading progressView.isVisible = viewState == ViewState.Loading playButton.isVisible = viewState == ViewState.Pausing pauseButton.isVisible = viewState == ViewState.Playing } private enum class ViewState( val isProgressVisible: Boolean, val isPlayButtonVisible: Boolean, val isPauseButtonVisible: Boolean ) { LOADING(true, false, false), PLAYING(false, false, true), PAUSING(false, true, false) }
54. Split by object, not condition Apply strategy pattern private fun updateView(viewState: ViewState) { loadingView.isVisible = viewState == ViewState.Loading progressView.isVisible = viewState.isProgressVisible playButton.isVisible = viewState.isPlayButtonVisible pauseButton.isVisible = viewState.isPauseButtonVisible } private enum class ViewState( val isProgressVisible: Boolean, val isPlayButtonVisible: Boolean, val isPauseButtonVisible: Boolean ) { LOADING(true, false, false), PLAYING(false, false, true), PAUSING(false, true, false) }
55. Split by object, not condition View state is synchronized only with function calls private fun showPausingView() { loadingView.isVisible = false playButton.isVisible = true playButton.setOnClickListener { player.play() showPLayingView() } pauseButton.isVisible = false progressView.isVisible = true progressUpdateJob?.cancel() }
56. Split by object, not condition View state is synchronized only with function calls private fun showPausingView() { loadingView.isVisible = false playButton.isVisible = true playButton.setOnClickListener { player.play() // Cannot to know actual state of player showPLayingView() } pauseButton.isVisible = false progressView.isVisible = true progressUpdateJob?.cancel() }
57. Split by object, not condition View state is synchronized only with function calls private fun showPLayingView() { loadingView.isVisible = false playButton.isVisible = false pauseButton.isVisible = true pauseButton.setOnClickListener { player.pause() showPausingView() } progressView.isVisible = true progressUpdateJob = lifecycleScope.launch { while (true) { progressView.text = "${player.currentPosition / 1000}/${player.duration / 1000}" delay(100) } } }
58. Split by object, not condition View state is synchronized only with function calls private fun showPLayingView() { loadingView.isVisible = false playButton.isVisible = false pauseButton.isVisible = true // Continues to be displayed after playback is finished pauseButton.setOnClickListener { player.pause() showPausingView() } progressView.isVisible = true progressUpdateJob = lifecycleScope.launch { while (true) { progressView.text = "${player.currentPosition / 1000}/${player.duration / 1000}" delay(100) } } }
59. Split by object, not condition Sync between player state and view state private fun showPLayingView() { loadingView.isVisible = false playButton.isVisible = false pauseButton.isVisible = true pauseButton.setOnClickListener { player.pause() showPausingView() } progressView.isVisible = true progressUpdateJob = lifecycleScope.launch { while (true) { progressView.text = "${player.currentPosition / 1000}/${player.duration / 1000}" delay(100) } } } • Sync between player and view by observe
60. Split by object, not condition Sync between player state and view state private fun showPLayingView() { loadingView.isVisible = false playButton.isVisible = false pauseButton.isVisible = true pauseButton.setOnClickListener { player.pause() showPausingView() } progressView.isVisible = true progressUpdateJob = lifecycleScope.launch { while (true) { progressView.text = "${player.currentPosition / 1000}/${player.duration / 1000}" delay(100) } } } • Sync between player and view by observe • Make wrapper class for player
61. Split by object, not condition The while statement for updating progressView is not canceled private fun showPLayingView() { loadingView.isVisible = false playButton.isVisible = false pauseButton.isVisible = true pauseButton.setOnClickListener { player.pause() showPausingView() } progressView.isVisible = true progressUpdateJob = lifecycleScope.launch { while (true) { progressView.text = "${player.currentPosition / 1000}/${player.duration / 1000}" delay(100) } } }
62. Split by object, not condition The while statement for updating progressView is not canceled private fun showPLayingView() { loadingView.isVisible = false playButton.isVisible = false pauseButton.isVisible = true pauseButton.setOnClickListener { player.pause() showPausingView() } progressView.isVisible = true progressUpdateJob = lifecycleScope.launch { while (true) { progressView.text = "${player.currentPosition / 1000}/${player.duration / 1000}" delay(100) } } } Change isPlaying: true→false→true in 0.1 sec
63. Split by object, not condition Cancel the previous job before executing a new job private fun showPLayingView() { loadingView.isVisible = false playButton.isVisible = false pauseButton.isVisible = true pauseButton.setOnClickListener { player.pause() showPausingView() } progressView.isVisible = true progressUpdateJob?.cancel() progressUpdateJob = lifecycleScope.launch { while (true) { progressView.text = "${player.currentPosition / 1000}/${player.duration / 1000}" delay(100) } } }
64. Questions 1. Everything is something 2. Split by object, not condition (Ikenaga-san 3. Truth in deep nests 4. What layout state class was really needed? 5. Coroutine's hard-to-understand trap that is easy to use for us
65. Truth in deep nests The repository class which handles image with memory and fi le cache class ProfileImageRepository( private val context: Context, private val metadataDao: ProfileImageMetadataDao = ProfileImageMetadataDao(), private val apiClient: RemoteApiClient = RemoteApiClient(context), private val ioScheduler: CoroutineContext = Dispatchers.IO ) { private data class ProfileImageCache( val userId: UserId, val bitmap: Bitmap, val expirationMillis: Long ) private val fileCacheDirectory: File = context.cacheDir.resolve(PROFILE_IMAGE_DIRECTORY_NAME) private val memoryCache: MutableMap<UserId, ProfileImageCache> = mutableMapOf() suspend fun getProfileImage(userId: UserId): Bitmap? = withContext(ioScheduler) {} private suspend fun getProfileImageFromLocalCacheOrQuery( userId: UserId ): Pair<Bitmap, Long>? = withContext(ioScheduler) { } private suspend fun queryProfileImage(userId: UserId): Pair<Bitmap, Long>? = withContext(ioScheduler) { } }
66. Truth in deep nests A deep function call nest makes di ffi cult to read class ProfileImageRepository( private val context: Context, private val metadataDao: ProfileImageMetadataDao = ProfileImageMetadataDao(), private val apiClient: RemoteApiClient = RemoteApiClient(context), private val ioScheduler: CoroutineContext = Dispatchers.IO ) { private data class ProfileImageCache( val userId: UserId, val bitmap: Bitmap, val expirationMillis: Long ) private val fileCacheDirectory: File = context.cacheDir.resolve(PROFILE_IMAGE_DIRECTORY_NAME) private val memoryCache: MutableMap<UserId, ProfileImageCache> = mutableMapOf() suspend fun getProfileImage(userId: UserId): Bitmap? = withContext(ioScheduler) {} private suspend fun getProfileImageFromLocalCacheOrQuery( userId: UserId ): Pair<Bitmap, Long>? = withContext(ioScheduler) { } private suspend fun queryProfileImage(userId: UserId): Pair<Bitmap, Long>? = withContext(ioScheduler) { } } fun getProfileImage() { ...; getProfileImageFromLocalCacheOrQuery() } fun getProfileImageFromLocalCacheOrQuery() { ...; queryProfileImage() } fun queryProfileImage() { ... }
67. Truth in deep nests Change the way functions are divided suspend fun getProfileImage(userId: UserId): Bitmap? = withContext(ioScheduler) { val memoryCachedData = memoryCache[userId] ?.takeIf { it.expirationMillis >= System.currentTimeMillis() } if (memoryCachedData != null) { return@withContext memoryCachedData.bitmap } val (bitmap, expirationMillis) = getProfileImageFromLocalCacheOrQuery(userId) ?: return@withContext null memoryCache[userId] = ProfileImageCache(userId, bitmap, expirationMillis) bitmap }
68. suspend fun getProfileImage(userId: String): Bitmap { val imageData: Bitmap = getCachedImageDataOnMemory(userId) ?: getCachedImageDataOnLocalStorage(userId) ?: queryRemoteImageData(userId) storeOnStorageCache(userId, imageData) storeOnMemoryCache(userId, imageData) return imageData } Change the way functions are divided Truth in deep nests
69. Truth in deep nests Mutual exclusion is not in place suspend fun getProfileImage(userId: String): Bitmap { val imageData: Bitmap = getCachedImageDataOnMemory(userId) ?: getCachedImageDataOnLocalStorage(userId) ?: queryRemoteImageData(userId) storeOnStorageCache(userId, imageData) storeOnMemoryCache(userId, imageData) return imageData }
70. Truth in deep nests Mutual exclusion is not in place suspend fun getProfileImage(userId: String): Bitmap { val imageData: Bitmap = getCachedImageDataOnMemory(userId) ?: getCachedImageDataOnLocalStorage(userId) ?: queryRemoteImageData(userId) storeOnStorageCache(userId, imageData) storeOnMemoryCache(userId, imageData) return imageData } suspend fun getProfileImage(userId: String): Bitmap { val imageData: Bitmap = getCachedImageDataOnMemory(userId) ?: getCachedImageDataOnLocalStorage(userId) ?: queryRemoteImageData(userId) storeOnStorageCache(userId, imageData) storeOnMemoryCache(userId, imageData) return imageData }
71. Truth in deep nests Mutual exclusion is not in place suspend fun getProfileImage(userId: String): Bitmap { val imageData: Bitmap = getCachedImageDataOnMemory(userId) ?: getCachedImageDataOnLocalStorage(userId) ?: queryRemoteImageData(userId) storeOnMemoryCache(imageData) storeOnStorageCache(imageData) return imageData } // Multiple calls at the same time suspend fun getProfileImage(userId: String): Bitmap { val imageData: Bitmap = getCachedImageDataOnMemory(userId) ?: getCachedImageDataOnLocalStorage(userId) ?: queryRemoteImageData(userId) storeOnStorageCache(userId, imageData) storeOnMemoryCache(userId, imageData) return imageData } suspend fun getProfileImage(userId: String): Bitmap { val imageData: Bitmap = getCachedImageDataOnMemory(userId) ?: getCachedImageDataOnLocalStorage(userId) ?: queryRemoteImageData(userId) storeOnStorageCache(userId, imageData) storeOnMemoryCache(userId, imageData) return imageData }
72. Truth in deep nests Use mutex to support mutual exclusion private val mutexMap: ConcurrentMap<UserId, Mutex> = ConcurrentHashMap() suspend fun getProfileImage(userId: UserId) { val mutex = mutexMap.getOrPut(userId) { Mutex() } return mutex.withLock { val imageData = getCachedImageDataOnMemory(userId) ?: getCachedImageDataOnLocalStorage(userId) ?: queryRemoteImageData(userId) storeOnStorageCache(userId, imageData) storeOnMemoryCache(userId, imageData) imageData } }
73. Cancellation causes inconsistency suspend fun storeOnMemoryCache(userId: UserId, imageData: Bitmap) = withContext(ioScheduler) { …
 memoryCache[userId] = ProfileImageCache(userId, bitmap, expirationMillis) } suspend fun storeOnStorageCache(userId: UserId, imageData: Bitmap) = withContext(ioScheduler) { … metadataDao.storeExpirationMillis(userId, expirationMillis) cacheFile.writeAsBitmap(bitmap) } Truth in deep nests
74. Cancellation causes inconsistency Truth in deep nests suspend fun storeOnMemoryCache(userId: UserId, imageData: Bitmap) = withContext(ioScheduler) { …
 memoryCache[userId] = ProfileImageCache(userId, bitmap, expirationMillis) } suspend fun storeOnStorageCache(userId: UserId, imageData: Bitmap) = withContext(ioScheduler) { … metadataDao.storeExpirationMillis(userId, expirationMillis) cacheFile.writeAsBitmap(bitmap) }
75. Cancellation causes inconsistency suspend fun storeOnMemoryCache(userId: UserId, imageData: Bitmap) = withContext(NonCancellable + ioScheduler) { … memoryCache[userId] = ProfileImageCache(userId, bitmap, expirationMillis) } suspend fun storeOnStorageCache(userId: UserId, imageData: Bitmap) = withContext(NonCancellable + ioScheduler) { … metadataDao.storeExpirationMillis(userId, expirationMillis) cacheFile.writeAsBitmap(bitmap) } Truth in deep nests
76. Questions 1. Everything is something 2. Split by object, not condition (Ikenaga-san 3. Truth in deep nests 4. What layout state class was really needed? 5. Coroutine's hard-to-understand trap that is easy to use for us
77. What layout state class was really needed? There is a huge data classes placed directly in fi le with global functions data class StickerLayoutState( val stickerID: StickerID, val stickerName: String, val stickerImage: String, val stickerType: StickerType, val creatorName: String, val creatorImage: String, val reviewPageIndex: Int, val reviewPageTotalCount: Int, val currentPageReviews: List<ReviewModel>, val isDetailPageOpened: Boolean, val publicationDateText: String, val contentSummaryText: String, ) private var layoutState: StickerLayoutState = StickerLayoutState.Empty class StickerType(val isOfficial: Boolean, val isUserMade: Boolean, val pageUrl: PageUrl) class PageUrl(val officialUrl: String?, val userPageUrl: String?) fun setSticker(binding: StickerLayoutBinding, stickerModel: StickerModel) { //…snip }
78. What layout state class was really needed? Minimize the scope of disclosure data class StickerLayoutState( val stickerID: StickerID, val stickerName: String, val stickerImage: String, val stickerType: StickerType, val creatorName: String, val creatorImage: String, val reviewPageIndex: Int, val reviewPageTotalCount: Int, val currentPageReviews: List<ReviewModel>, val isDetailPageOpened: Boolean, val publicationDateText: String, val contentSummaryText: String, ) private var layoutState: StickerLayoutState = StickerLayoutState.Empty class StickerType(val isOfficial: Boolean, val isUserMade: Boolean, val pageUrl: PageUrl) class PageUrl(val officialUrl: String?, val userPageUrl: String?) fun setSticker(binding: StickerLayoutBinding, stickerModel: StickerModel) { //…snip }
79. What layout state class was really needed? Minimize the scope of disclosure class StickerViewPresenter(val binding: StickerViewBinding) { data class StickerLayoutState( val stickerID: StickerID, val stickerName: String, val stickerImage: String, val stickerType: StickerType, val creatorName: String, val creatorImage: String, val reviewPageIndex: Int, val reviewPageTotalCount: Int, val currentPageReviews: List<ReviewModel>, val isDetailPageOpened: Boolean, val publicationDateText: String, val contentSummaryText: String, ) private var layoutState: StickerLayoutState = StickerLayoutState.Empty class StickerType(val isOfficial: Boolean, val isUserMade: Boolean, val pageUrl: PageUrl) class PageUrl(val officialUrl: String?, val userPageUrl: String?) fun setSticker(binding: StickerLayoutBinding, stickerModel: StickerModel) { //…snip }
80. What layout state class was really needed? Beware of implicit dependence class StickerType(val isOfficial: Boolean, val isUserMade: Boolean, val pageUrl: PageUrl) class PageUrl(val officialUrl: String?, val userPageUrl: String?)
81. What layout state class was really needed? Beware of implicit dependence class StickerType(val isOfficial: Boolean, val isUserMade: Boolean, val pageUrl: PageUrl) class PageUrl(val officialUrl: String?, val userPageUrl: String?)
82. What layout state class was really needed? Beware of implicit dependence class StickerType(val isOfficial: Boolean, val isUserMade: Boolean, val pageUrl: PageUrl) class PageUrl(val officialUrl: String?, val userPageUrl: String?)
83. What layout state class was really needed? Represents the required data. sealed class StickerType { data class UserMade(val userPageUrl: String) : StickerType() data class Official(val officialUrl: String) : StickerType() }
84. What layout state class was really needed? Represents the required data. sealed class StickerType { data class UserMade(val userPageUrl: String) : StickerType() data class Official(val officialUrl: String) : StickerType() } enum class StickerType { OFFICIAL, USER_MADE }
85. What layout state class was really needed? Still a huge data class data class StickerLayoutState( val stickerID: StickerID, val stickerName: String, val stickerImage: String, val stickerType: StickerType, val creatorName: String, val creatorImage: String, val reviewPageIndex: Int, val reviewPageTotalCount: Int, val currentPageReviews: List<ReviewModel>, val isDetailPageOpened: Boolean, val publicationDateText: String, val contentSummaryText: String, )
86. What layout state class was really needed? Still a huge data class data class StickerLayoutState( val stickerID: StickerID, val stickerName: String, val stickerImage: String, val stickerType: StickerType, val creatorName: String, val creatorImage: String, val reviewPageIndex: Int, val reviewPageTotalCount: Int, val currentPageReviews: List<ReviewModel>, val isDetailPageOpened: Boolean, val publicationDateText: String, val contentSummaryText: String, ) fun setSticker(…) {} fun setDetailLayoutOpened(…) {} fun onReviewPageLoaded(…) {} fun onDetailLayoutLoaded(…) {}
87. What layout state class was really needed? Separate state model class data class StickerBasicDataModel( val stickerId: StickerId, val stickerName: String, val creatorName: String, val stickerImageUrl: String ) data class StickerDetailPageState( val stickerType: StickerType, val creatorName: String, val stickerImageUrl: String val creatorImageUrl: String val publicationDateText: String, val contentSummaryText: String ) data class CommentPageState( val reviewPageIndex: Int, val reviewPageTotalCount: Int, val currentPageReviews: List<ReviewModel> ) fun setSticker(…) {} fun setDetailLayoutOpened(…) {} fun onReviewPageLoaded(…) {} fun onDetailLayoutLoaded(…) {}
88. What layout state class was really needed? Fix function de fi nitions data class StickerBasicDataModel( val stickerId: StickerId, val stickerName: String, val creatorName: String, val stickerImageUrl: String ) data class StickerDetailPageState( val stickerType: StickerType, val creatorName: String, val stickerImageUrl: String val creatorImageUrl: String val publicationDateText: String, val contentSummaryText: String ) data class CommentPageState( val reviewPageIndex: Int, val reviewPageTotalCount: Int, val currentPageReviews: List<ReviewModel> ) fun showStickerView(stickerModel: StickerBasicDataModel) {} fun showStickerDetailView( stickerDetailPageState: StickerDetailPageState, isDetailPageOpened: Boolean ) {} fun showCommentView(commentPageState: CommentPageState) {}
89. Questions 1. Everything is something 2. Split by object, not condition (Ikenaga-san 3. Truth in deep nests 4. What layout state class was really needed? 5. Coroutine's hard-to-understand trap that is easy to use
90. Coroutine's hard-to-understand trap There is a customized ContentProvider class class MyContentProvider : ContentProvider() { private val singleThreadCoroutineDispatcher: CoroutineDispatcher = Executors.newSingleThreadExecutor().asCoroutineDispatcher() private val getResultCache: MutableMap<Long, String> = mutableMapOf() private val apiClient: ApiClient = ApiClient() override fun query(…): Cursor { /* Call get(Long) function */ } private suspend fun get(id: Long): String = withContext(singleThreadCoroutineDispatcher) { /* Call apiClient.get(id) */ } private suspend fun showErrorMessage() = withContext(Dispatchers.Main) { } private class ApiClient { @Throws(IOException::class) suspend fun get(id: Long): String = withContext(Dispatchers.IO) { } } }
91. Coroutine's hard-to-understand trap Mutual exclusion is not in place private val getResultCache: MutableMap<Long, String> = mutableMapOf() private suspend fun get(id: Long): String = withContext(singleThreadCoroutineDispatcher) { val resultFromCache = getResultCache[id] if (resultFromCache != null) { return@withContext resultFromCache } val result = try { apiClient.get(id) } catch (_: Throwable) { showErrorMessage() null } getResultCache[id] = result return@withContext result }
92. Coroutine's hard-to-understand trap Use mutex to support mutual exclusion private val mutex = Mutex() private val getResultCache: MutableMap<Long, String> = mutableMapOf() private suspend fun get(id: Long): String? = withContext(Dispatchers.IO) { mutex.withLock { val resultFromCache = getResultCache[id] if (resultFromCache != null) { return@withLock resultFromCache } val result = try { apiClient.get(id) } catch (_: Throwable) { showErrorMessage() null } getResultCache[id] = result result } }
93. Coroutine's hard-to-understand trap Danger of runBlocking and single threading override fun query(…): Cursor { val id: Long = TODO() val result = runBlocking { get(id) } return MatrixCursor(TODO()) } private suspend fun get(id: Long): String = withContext(singleThreadCoroutineDispatcher) { //…snip val result = try { apiClient.get(id) } catch (_: Throwable) { showErrorMessage() null } //…snip } private suspend fun showErrorMessage() = withContext(Dispatchers.Main) { Toast.makeText(this, R.string.error, Toast.LENGTH_SHORT).show() } private class ApiClient { @Throws(IOException::class) suspend fun get(id: Long): String = withContext(Dispatchers.IO) { //…snip } }
94. Coroutine's hard-to-understand trap Danger of runBlocking and single threading override fun query(…): Cursor { val id: Long = TODO() val result = runBlocking { get(id) } return MatrixCursor(TODO()) } private suspend fun get(id: Long): String = withContext(singleThreadCoroutineDispatcher) { //…snip val result = try { apiClient.get(id) } catch (_: Throwable) { showErrorMessage() null } //…snip } private suspend fun showErrorMessage() = withContext(Dispatchers.Main) { Toast.makeText(this, R.string.error, Toast.LENGTH_SHORT).show() } private class ApiClient { @Throws(IOException::class) suspend fun get(id: Long): String = withContext(Dispatchers.IO) { //…snip } }
95. Coroutine's hard-to-understand trap Danger of runBlocking and single threading override fun query(…): Cursor { val id: Long = TODO() val result = runBlocking { get(id) } return MatrixCursor(TODO()) } private suspend fun get(id: Long): String = withContext(singleThreadCoroutineDispatcher) { //…snip val result = try { apiClient.get(id) } catch (_: Throwable) { showErrorMessage() null } //…snip } private suspend fun showErrorMessage() = withContext(Dispatchers.Main) { Toast.makeText(this, R.string.error, Toast.LENGTH_SHORT).show() } private class ApiClient { @Throws(IOException::class) suspend fun get(id: Long): String = withContext(Dispatchers.IO) { //…snip } } • Call from other process with binder thread
96. Coroutine's hard-to-understand trap Danger of runBlocking and single threading override fun query(…): Cursor { val id: Long = TODO() val result = runBlocking { get(id) } return MatrixCursor(TODO()) } private suspend fun get(id: Long): String = withContext(singleThreadCoroutineDispatcher) { //…snip val result = try { apiClient.get(id) } catch (_: Throwable) { showErrorMessage() null } //…snip } private suspend fun showErrorMessage() = withContext(Dispatchers.Main) { Toast.makeText(this, R.string.error, Toast.LENGTH_SHORT).show() } private class ApiClient { @Throws(IOException::class) suspend fun get(id: Long): String = withContext(Dispatchers.IO) { //…snip } } • Call from other process with binder thread • Call from same process with Caller's thread
97. Coroutine's hard-to-understand trap Danger of runBlocking and single threading override fun query(…): Cursor { val id: Long = TODO() val result = runBlocking { get(id) } return MatrixCursor(TODO()) } private suspend fun get(id: Long): String = withContext(singleThreadCoroutineDispatcher) { //…snip val result = try { apiClient.get(id) } catch (_: Throwable) { showErrorMessage() null } //…snip } private suspend fun showErrorMessage() = withContext(Dispatchers.Main) { Toast.makeText(this, R.string.error, Toast.LENGTH_SHORT).show() } private class ApiClient { @Throws(IOException::class) suspend fun get(id: Long): String = withContext(Dispatchers.IO) { //…snip } } • Call from same process with main thread
98. Coroutine's hard-to-understand trap Danger of runBlocking and single threading override fun query(…): Cursor { val id: Long = TODO() val result = runBlocking { get(id) } return MatrixCursor(TODO()) } private suspend fun get(id: Long): String = withContext(singleThreadCoroutineDispatcher) { //…snip val result = try { apiClient.get(id) } catch (_: Throwable) { showErrorMessage() null } //…snip } private suspend fun showErrorMessage() = withContext(Dispatchers.Main) { Toast.makeText(this, R.string.error, Toast.LENGTH_SHORT).show() } private class ApiClient { @Throws(IOException::class) suspend fun get(id: Long): String = withContext(Dispatchers.IO) { //…snip } } private val singleThreadCoroutineDispatcher: CoroutineDispatcher = Executors.newSingleThreadExecutor().asCoroutineDispatcher()
99. Coroutine's hard-to-understand trap Danger of runBlocking and single threading override fun query(…): Cursor { val id: Long = TODO() val result = runBlocking { get(id) } return MatrixCursor(TODO()) } private suspend fun get(id: Long): String = withContext(singleThreadCoroutineDispatcher) { //…snip val result = try { apiClient.get(id) } catch (_: Throwable) { showErrorMessage() null } //…snip } private suspend fun showErrorMessage() = withContext(Dispatchers.Main) { Toast.makeText(this, R.string.error, Toast.LENGTH_SHORT).show() } private class ApiClient { @Throws(IOException::class) suspend fun get(id: Long): String = withContext(Dispatchers.IO) { //…snip } }
100. Coroutine's hard-to-understand trap Danger of runBlocking and single threading override fun query(…): Cursor { val id: Long = TODO() val result = runBlocking { get(id) } // Blocking main thread return MatrixCursor(TODO()) } private suspend fun get(id: Long): String = withContext(singleThreadCoroutineDispatcher) { //…snip val result = try { apiClient.get(id) } catch (_: Throwable) { showErrorMessage() null } //…snip } private suspend fun showErrorMessage() = withContext(Dispatchers.Main) { Toast.makeText(this, R.string.error, Toast.LENGTH_SHORT).show() } private class ApiClient { @Throws(IOException::class) suspend fun get(id: Long): String = withContext(Dispatchers.IO) { //…snip } }
101. Coroutine's hard-to-understand trap Narrow down the scope of one's responsibility override fun query(…): Cursor { val id: Long = TODO() val result = runBlocking { get(id) } return MatrixCursor(TODO()) } private suspend fun get(id: Long): String = withContext(singleThreadCoroutineDispatcher) { //…snip val result = try { apiClient.get(id) } catch (_: Throwable) { null } //…snip }
102. Coroutine's hard-to-understand trap Narrow down the scope of one's responsibility /** * Retrieve data from a cache or API server. * Note: Don't call this function from main thread. */ override fun query(…): Cursor { val id: Long = TODO() val result = runBlocking { get(id) } return MatrixCursor(TODO()) } private suspend fun get(id: Long): String = withContext(singleThreadCoroutineDispatcher) { //…snip val result = try { apiClient.get(id) } catch (_: Throwable) { null } //…snip }
103. Coroutine's hard-to-understand trap Narrow down the scope of one's responsibility /** * Retrieve data from a cache or API server. * Note: Don't call this function from main thread. */ override fun query(…): Cursor { val id: Long = TODO() val result = runBlocking { get(id) } return MatrixCursor(TODO()) } private suspend fun get(id: Long): String = withContext(singleThreadCoroutineDispatcher) { //…snip val result = try { apiClient.get(id) } catch (_: Throwable) { null } //…snip } Add custom linter to avoid calling this function from main thread.
104. How did we create the question?
105. We have the “Review committee”
106. Review committee Team A Team B senior eng. “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Ref: Confronting quality problems of long-lived codebase, Munetoshi Ishikawa, LINE Corporation https://www.youtube.com/watch?v=z8PwD68_zwg
107. Review committee 1. A team voluntarily picks a “merged” pull-request once per week Team A Team B senior eng. send a merged PR “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Ref: Confronting quality problems of long-lived codebase, Munetoshi Ishikawa, LINE Corporation https://www.youtube.com/watch?v=z8PwD68_zwg
108. Review committee 1. A team voluntarily picks a “merged” pull-request once per week 2. A senior engineer reviews the picked pull-request Team A Team B senior eng. send a merged PR feedback “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Ref: Confronting quality problems of long-lived codebase, Munetoshi Ishikawa, LINE Corporation https://www.youtube.com/watch?v=z8PwD68_zwg
109. Review committee 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 “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Ref: Confronting quality problems of long-lived codebase, Munetoshi Ishikawa, LINE Corporation https://www.youtube.com/watch?v=z8PwD68_zwg
110. Review committee 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/ Ref: Confronting quality problems of long-lived codebase, Munetoshi Ishikawa, LINE Corporation https://www.youtube.com/watch?v=z8PwD68_zwg
111. How to make the question? Apply the weekly report in reverse to create "Bad Code". Code maker “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Weekly reports
112. How to make the question? Code maker “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Weekly reports Apply the weekly report in reverse to create "Bad Code".
113. How to make the question? Code maker “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Weekly reports Apply the weekly report in reverse to create "Bad Code".
114. How to make the question? Code maker “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Weekly reports Apply the weekly report in reverse to create "Bad Code".
115. How to make the question? Code maker “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Weekly reports Apply the weekly report in reverse to create "Bad Code".
116. How to make the question? Code maker “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Weekly reports Apply the weekly report in reverse to create "Bad Code".
117. How to make the question? Code maker “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Weekly reports Make “Bad Code” Questions Apply the weekly report in reverse to create "Bad Code".
118. How to make the question? Code maker “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Weekly reports Make “Bad Code” class Ugly1 { fun dirtyFun1() { /* */ } } Questions Apply the weekly report in reverse to create "Bad Code".
119. class Ugly1 { fun dirtyFun1() { /* */ } } How to make the question? Code maker “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Weekly reports Make “Bad Code” Questions class Ugly2 { fun dirtyFun2() { /* */ } } Apply the weekly report in reverse to create "Bad Code".
120. How to make the question? Code maker “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Weekly reports Make “Bad Code” Questions class Ugly1 { fun dirtyFun1() { /* */ } } class Ugly2 { fun dirtyFun2() { /* */ } } class Ugly3 { fun dirtyFun3() { /* */ } } Apply the weekly report in reverse to create "Bad Code".
121. How to make the question? Code maker “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Weekly reports Make “Bad Code” Questions class Ugly1 { fun dirtyFun1() { /* */ } } class Ugly2 { fun dirtyFun2() { /* */ } } class Ugly3 { fun dirtyFun3() { /* */ } } class Ugly4 { fun dirtyFun4() { /* */ } } Apply the weekly report in reverse to create "Bad Code".
122. How to make the question? Code maker “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Weekly reports Make “Bad Code” Questions class Ugly1 { fun dirtyFun1() { /* */ } } class Ugly2 { fun dirtyFun2() { /* */ } } class Ugly3 { fun dirtyFun3() { /* */ } } class Ugly4 { fun dirtyFun4() { /* */ } } class Ugly5 { fun dirtyFun5() { /* */ } } Apply the weekly report in reverse to create "Bad Code".
123. How to make the question? “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ White board class Ugly1 { fun dirtyFun1() { /* */ } } class Ugly2 { fun dirtyFun2() { /* */ } } class Ugly3 { fun dirtyFun3() { /* */ } } class Ugly4 { fun dirtyFun4() { /* */ } } class Ugly5 { fun dirtyFun5() { /* */ } } Apply the weekly report in reverse to create "Bad Code".
124. Want to hear more?
125. Watch this video 
 from DroidKaigi 2021 Ref: Confronting quality problems of long-lived codebase, Munetoshi Ishikawa, LINE Corporation https://www.youtube.com/watch?v=z8PwD68_zwg
126. Summary Code Review Challenge in DroidKaigi 2022 - Review “Bad Code” to become “Good Code” - Got a lot of "Good" reviews After DroidKaigi 2022 - Explain an example solutions - Introduce “Review Committee”, the source of the questions.
127. Detailed explanations of the questions will also be available hereɹɹɹ Ref: https://engineering.linecorp.com/ja/blog/code_review_challenge/ “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/