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

Code Review Challenge An example of a solution

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

LINE Developers
PRO

October 12, 2022
Tweet

More Decks by LINE Developers

Other Decks in Technology

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?

  6. None
  7. None
  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) ?: [email protected] 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) { [email protected] memoryCachedData.bitmap } val (bitmap, expirationMillis) = getProfileImageFromLocalCacheOrQuery(userId) ?: [email protected] 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) { [email protected] resultFromCache } val result = try { apiClient.get(id) } catch (_: Throwable) { showErrorMessage() null } getResultCache[id] = result [email protected] 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) { [email protected] 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/