Slide 1
Slide 1 text
Yuki Ando, LINE Corporation
Code Review Challenge
An example of a solution
DroidKaigi 2022: ίʔυϨϏϡʔνϟϨϯδͷղઆ
Slide 2
Slide 2 text
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
Slide 3
Slide 3 text
Did you participate in
DroidKaigi 2022 offline?
Slide 4
Slide 4 text
Yes I did
“Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/
Slide 5
Slide 5 text
Have you participated in the
Code Review Challenge?
Slide 6
Slide 6 text
No content
Slide 7
Slide 7 text
No content
Slide 8
Slide 8 text
What is
the Code Review Challenge?
Slide 9
Slide 9 text
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/
Slide 10
Slide 10 text
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”
Slide 11
Slide 11 text
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”
Slide 12
Slide 12 text
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”
Slide 13
Slide 13 text
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”
Slide 14
Slide 14 text
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”
Slide 15
Slide 15 text
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”
Slide 16
Slide 16 text
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”
Slide 17
Slide 17 text
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”
Slide 18
Slide 18 text
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”
Slide 19
Slide 19 text
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”
Slide 20
Slide 20 text
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”
Slide 21
Slide 21 text
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”
Slide 22
Slide 22 text
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”
Slide 23
Slide 23 text
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”
Slide 24
Slide 24 text
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”
Slide 25
Slide 25 text
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”
Slide 26
Slide 26 text
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”
Slide 27
Slide 27 text
Explain the assumed solutions
Slide 28
Slide 28 text
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/
Slide 29
Slide 29 text
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
Slide 30
Slide 30 text
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 }
}
Slide 31
Slide 31 text
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
}
}
Slide 32
Slide 32 text
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
}
}
Slide 33
Slide 33 text
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()
}
Slide 34
Slide 34 text
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()
}
Slide 35
Slide 35 text
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
}
}
Slide 36
Slide 36 text
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
}
}
Slide 37
Slide 37 text
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
}
}
Slide 38
Slide 38 text
Everything is something
Easily broken due to lack of type constraints
class BotItemPresenter(
private val nameProvider: NameProvider,
private val coroutineScope: CoroutineScope
) { //…snip }
Slide 39
Slide 39 text
Everything is something
Easily broken due to lack of type constraints
class BotItemPresenter(
private val nameProvider: NameProvider,
private val coroutineScope: CoroutineScope
) { //…snip }
Slide 40
Slide 40 text
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 }
Slide 41
Slide 41 text
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 }
}
Slide 42
Slide 42 text
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)
Slide 43
Slide 43 text
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
}
}
Slide 44
Slide 44 text
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
}
}
Slide 45
Slide 45 text
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
Slide 46
Slide 46 text
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
Slide 47
Slide 47 text
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
Slide 48
Slide 48 text
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()
}
Slide 49
Slide 49 text
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
)
Slide 50
Slide 50 text
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
)
Slide 51
Slide 51 text
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
)
Slide 52
Slide 52 text
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
}
Slide 53
Slide 53 text
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)
}
Slide 54
Slide 54 text
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)
}
Slide 55
Slide 55 text
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()
}
Slide 56
Slide 56 text
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()
}
Slide 57
Slide 57 text
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)
}
}
}
Slide 58
Slide 58 text
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)
}
}
}
Slide 59
Slide 59 text
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
Slide 60
Slide 60 text
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
Slide 61
Slide 61 text
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)
}
}
}
Slide 62
Slide 62 text
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
Slide 63
Slide 63 text
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)
}
}
}
Slide 64
Slide 64 text
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
Slide 65
Slide 65 text
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 = mutableMapOf()
suspend fun getProfileImage(userId: UserId): Bitmap? = withContext(ioScheduler) {}
private suspend fun getProfileImageFromLocalCacheOrQuery(
userId: UserId
): Pair? = withContext(ioScheduler) { }
private suspend fun queryProfileImage(userId: UserId): Pair? = withContext(ioScheduler) { }
}
Slide 66
Slide 66 text
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 = mutableMapOf()
suspend fun getProfileImage(userId: UserId): Bitmap? = withContext(ioScheduler) {}
private suspend fun getProfileImageFromLocalCacheOrQuery(
userId: UserId
): Pair? = withContext(ioScheduler) { }
private suspend fun queryProfileImage(userId: UserId): Pair? = withContext(ioScheduler) { }
}
fun getProfileImage() {
...;
getProfileImageFromLocalCacheOrQuery()
}
fun getProfileImageFromLocalCacheOrQuery() {
...;
queryProfileImage()
}
fun queryProfileImage() { ... }
Slide 67
Slide 67 text
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
}
Slide 68
Slide 68 text
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
Slide 69
Slide 69 text
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
}
Slide 70
Slide 70 text
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
}
Slide 71
Slide 71 text
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
}
Slide 72
Slide 72 text
Truth in deep nests
Use mutex to support mutual exclusion
private val mutexMap: ConcurrentMap = 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
}
}
Slide 73
Slide 73 text
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
Slide 74
Slide 74 text
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)
}
Slide 75
Slide 75 text
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
Slide 76
Slide 76 text
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
Slide 77
Slide 77 text
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,
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 }
Slide 78
Slide 78 text
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,
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 }
Slide 79
Slide 79 text
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,
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 }
Slide 80
Slide 80 text
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?)
Slide 81
Slide 81 text
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?)
Slide 82
Slide 82 text
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?)
Slide 83
Slide 83 text
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()
}
Slide 84
Slide 84 text
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 }
Slide 85
Slide 85 text
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,
val isDetailPageOpened: Boolean,
val publicationDateText: String,
val contentSummaryText: String,
)
Slide 86
Slide 86 text
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,
val isDetailPageOpened: Boolean,
val publicationDateText: String,
val contentSummaryText: String,
)
fun setSticker(…) {}
fun setDetailLayoutOpened(…) {}
fun onReviewPageLoaded(…) {}
fun onDetailLayoutLoaded(…) {}
Slide 87
Slide 87 text
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
)
fun setSticker(…) {}
fun setDetailLayoutOpened(…) {}
fun onReviewPageLoaded(…) {}
fun onDetailLayoutLoaded(…) {}
Slide 88
Slide 88 text
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
)
fun showStickerView(stickerModel: StickerBasicDataModel) {}
fun showStickerDetailView(
stickerDetailPageState: StickerDetailPageState,
isDetailPageOpened: Boolean
) {}
fun showCommentView(commentPageState: CommentPageState) {}
Slide 89
Slide 89 text
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
Slide 90
Slide 90 text
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 = 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) { }
}
}
Slide 91
Slide 91 text
Coroutine's hard-to-understand trap
Mutual exclusion is not in place
private val getResultCache: MutableMap = 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
}
Slide 92
Slide 92 text
Coroutine's hard-to-understand trap
Use mutex to support mutual exclusion
private val mutex = Mutex()
private val getResultCache: MutableMap = 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
}
}
Slide 93
Slide 93 text
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
}
}
Slide 94
Slide 94 text
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
}
}
Slide 95
Slide 95 text
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
Slide 96
Slide 96 text
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
Slide 97
Slide 97 text
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
Slide 98
Slide 98 text
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()
Slide 99
Slide 99 text
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
}
}
Slide 100
Slide 100 text
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
}
}
Slide 101
Slide 101 text
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
}
Slide 102
Slide 102 text
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
}
Slide 103
Slide 103 text
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.
Slide 104
Slide 104 text
How did we create the question?
Slide 105
Slide 105 text
We have the “Review committee”
Slide 106
Slide 106 text
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
Slide 107
Slide 107 text
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
Slide 108
Slide 108 text
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
Slide 109
Slide 109 text
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
Slide 110
Slide 110 text
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
Slide 111
Slide 111 text
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
Slide 112
Slide 112 text
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".
Slide 113
Slide 113 text
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".
Slide 114
Slide 114 text
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".
Slide 115
Slide 115 text
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".
Slide 116
Slide 116 text
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".
Slide 117
Slide 117 text
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".
Slide 118
Slide 118 text
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".
Slide 119
Slide 119 text
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".
Slide 120
Slide 120 text
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".
Slide 121
Slide 121 text
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".
Slide 122
Slide 122 text
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".
Slide 123
Slide 123 text
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".
Slide 124
Slide 124 text
Want to hear more?
Slide 125
Slide 125 text
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
Slide 126
Slide 126 text
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.
Slide 127
Slide 127 text
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/