Code Review Challenge An example of a solution

Yuki Ando, LINE Corporation Code Review Challenge An example of
a solution DroidKaigi 2022: ίʔυϨϏϡʔνϟϨϯδͷ໰୊ղઆ

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

Did you participate in DroidKaigi 2022 offline?

Yes I did “Twemoji” ©Twitter, Inc and other contributors (Licensed
under CC-BY 4.0) https://twemoji.twitter.com/

Have you participated in the Code Review Challenge?

What is the Code Review Challenge?

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/

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”

{ 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”

{ 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”

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”

{ 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”

{ 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”

Explain the assumed solutions

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/

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

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 } }

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 } }

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() }

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() }

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 } }

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 } }

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 } }

Everything is something Easily broken due to lack of type
constraints class BotItemPresenter( private val nameProvider: NameProvider, private val coroutineScope: CoroutineScope ) { //…snip }

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 }

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 } }

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)

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 } }

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 } }

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

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

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

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() }

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 )

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 )

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 )

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 }

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) }

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) }

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() }

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() }

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) } } }

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) } } }

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

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

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) } } }

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

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) } } }

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

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) { } }

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() { ... }

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 }

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

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 } 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) 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 }

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 } }

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

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) }

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

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

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 }

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 }

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 }

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?)

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() }

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 }

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, )

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(…) {}

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(…) {}

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) {}

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

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) { } } }

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 }

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 } }

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 } }

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

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

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

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()

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 } }

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 } }

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 }

/** * 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 }

/** * 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.

How did we create the question?

We have the “Review committee”

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

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

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

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

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

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

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".

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".

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".

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".

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".

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".

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".

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".

Want to hear more?

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

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.

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/

Code Review Challenge An example of a solution

Code Review Challenge An example of a solution

More Decks by LINE Developers

Other Decks in Technology

Featured

Transcript