Code Review Challenge: An example of a solution

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

View Slide

Self-introduction
Name: Yuki Ando / ҆౻ ༞و 
Twitter: @m1zyuk1

Hobby: Car, Game

Main activity:  
Apply new Android feature to LINE Android

View Slide

Did you participate in

DroidKaigi 2023 offline?

View Slide

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

View Slide

Have you participated in the
Code Review Challenge?

View Slide

View Slide

What’s Code Review Challenge?

View Slide

Review “Bad Code” to be a “Good Code”
LINE Booth in DroidKaigi 2023
class Ugly {
fun dirtyFunc() { /* */ }
}
Challengers

View Slide

class Ugly {
}
Challengers
Review

View Slide

class Ugly {
}
Review
Review ideas
Challengers

View Slide

class Ugly {
}
class Good {
fun smartFunc() { /* */ }
}
Review
Review ideas
Challengers

View Slide

Explain the assumed solutions

View Slide

Full codes are hereɹɹɹ
Ref: https://engineering.linecorp.com/ja/blog/droidKaigi-2023

View Slide

Questions
1. Tip of the Ice-func

2. A Composable for everything, and everything in its Composable

3. Slip Through My Async

4. Caught on Running

5. Bitter Sweets

View Slide

Tip of the Ice-func
An object which have a function to do the back up process
object BackupCreator {
private val localDataSource = LocalDataSource()
private val remoteDataSource = RemoteDataSource()
private lateinit var progressDialog: ProgressDialogForBackup
@WorkerThread
fun createBackup(target: BackupTarget, since: Date) {
showProgressDialog()
uploadBackup(target, since)
hideProgressDialog()
}
private fun showProgressDialog() {
progressDialog = ProgressDialogForBackup()
progressDialog.showOnMainThread()
}
private fun hideProgressDialog() {
progressDialog.hideOnMainThread()
}
//...snip
}

View Slide

Tip of the Ice-func
private fun uploadBackup(target: BackupTarget, since: Date) {
compressBackupData(target, since)
remoteDataSource.upload(File("path/compressed"))
}
private fun compressBackupData(target: BackupTarget, since: Date) {
collectBackupData(target, since)
compress(source = File("path/raw"), destination = File("path/compressed"))
}
private fun collectBackupData(target: BackupTarget, since: Date) {
val data = when (target) {
BackupTarget.IMAGE -> localDataSource.selectImageData()
BackupTarget.VIDEO -> localDataSource.selectVideoData()
BackupTarget.MESSAGE -> localDataSource.selectMessageData(since)
}
writeToFile(File("path/raw"), data)
}
}

View Slide

Tip of the Ice-func
private fun writeToFile(file: File, data: BackupData) {
val outputStream = file.outputStream()
try {
outputStream.write(data.toByteArray())
} catch (e: IOException) {
// Ignore
}
outputStream.close()
}

View Slide

Major issues
1. OutputStream is not handled properly

2. Signatures that can be used incorrectly

3. Structure of functions with poor readability

View Slide

Tip of the Ice-func
Issue: IOException should not be ignored
try {
// Ignore
}
}

View Slide

Tip of the Ice-func
Issue: Exception may be thrown
try {
// Ignore
}
}

View Slide

Tip of the Ice-func
Solution: Use closable classes with `use` phrase
// Throws [IOException] if any I/O error occurred.
file.outputStream().use { outputStream ->
}
}

View Slide

Tip of the Ice-func
Solution: Handle IOException such as canceling process
// Throws [IOException] if any I/O error occurred.
file.outputStream().use { outputStream ->
}
}
}
try {
//...snip
} catch (e: IOException){
// handle IO Exception
}
}

View Slide

Major issues
1. Might not be called OutputStream.close()



View Slide

Tip of the Ice-func
Issue: Signatures that can be used incorrectly

@WorkerThread
//...snip
}

View Slide

Tip of the Ice-func

@WorkerThread
//...snip
}
enum class BackupTarget { IMAGE, VIDEO, MESSAGE }

View Slide

Tip of the Ice-func

@WorkerThread
//...snip
}
`since` is not affected for image/video.
But interface doesn’t indicate this.

View Slide

Tip of the Ice-func

}
try {
//...snip
}
}
`since` is not affected for image/video.
But interface doesn’t indicate this.

View Slide

Tip of the Ice-func
Solution: Use sealed class to represent behavior correctly by interface
private fun collectBackupData(target: BackupTarget) {
BackupTarget.MESSAGE -> localDataSource.selectMessageData(target.since)
}
try {
//...snip
}
}
sealed class BackupTarget {
data object Image : BackupTarget()
data object Video : BackupTarget()
data class Message(val since: Date) : BackupTarget()
}

View Slide

Major issues
1. Might not be called OutputStream.close()



View Slide

Tip of the Ice-func
Issue: Structure of functions with poor readability
fun createBackup() {
uploadBackup()
}
fun uploadBackup() {
compressBackupData()
}
fun compressBackupData() {
collectBackupData()
}
fun collectBackupData() {
// collect backup data
}

View Slide

Tip of the Ice-func
Solution: Avoid unnecessary nesting and make it easier to understand
fun createBackup(target: BackupTarget) {
val tmpFiles = mutableListOf()
try {
val rawDataFile = collectBackupData(target).also(tmpFiles::add)
val compressedDataFile = compress(rawDataFile).also(tmpFiles::add)
remoteDataSource.upload(compressedDataFile)
// TODO: Handle error.
} finally {
tmpFiles.forEach(File::delete)
}
}

View Slide

Tip of the Ice-func
Other issues

• Implicit dependencies in
fi
le paths

• createBackup function is not thread safe

• BackupCreator object should be class with injecting properties

• ProgressDialog

• Should be nullable var instead of lateinit var

• Might be shown incorrectly in case of calling `createBackup` twice

• Should not display by BackupCreator

• Temporary
fi
les should be deleted

View Slide

Questions




5. Bitter Sweets/

View Slide

A Composable for everything, and everything in its Composable
A login screen which constructed by Composable function
@Composable
fun LoginScreen() {
Column(
modifier = Modifier.padding(DEFAULT_SIZE.dp)
) {
Spacer(
modifier = Modifier.weight(2f)
)
title()
Spacer(
)
val email = email()
Spacer(
modifier = Modifier.height(DEFAULT_SIZE.dp)
)
val password = password()
Spacer(
)
loginButton(
email = email,
password = password
)
}
}

View Slide

@Composable
private fun title() {
Text(
text = "DroidKaigi 2023",
fontSize = LARGE_SIZE.sp,
fontWeight = FontWeight.Bold
)
Spacer(
modifier = Modifier
.height(SMALL_SIZE.dp)
)
Text(
fontSize = DEFAULT_SIZE.sp,
text = "Let's enjoy!"
)
}
@Composable
private fun email(): String {
var email by remember {
mutableStateOf("")
}
TextField(
modifier = Modifier.fillMaxWidth(),
value = email,
onValueChange = { email = it },
placeholder = { Text("Email") }
)
return email
}

View Slide

@Composable
private fun password(): String {
var password by remember {
mutableStateOf("")
}
TextField(
value = password,
onValueChange = { password = it },
placeholder = { Text("Password") },
visualTransformation =
PasswordVisualTransformation()
)
return password
}
@Composable
private fun loginButton(
modifier: Modifier = Modifier,
password: String,
email: String,
viewModel: LoginViewModel = viewModel()
) {
var clicked by remember { mutableStateOf(false) }
val onBackPressedDispatcher =
LocalOnBackPressedDispatcherOwner.current
?.onBackPressedDispatcher
if (clicked) {
runBlocking {
if (viewModel.login(email, password)) {
onBackPressedDispatcher
?.onBackPressed()
}
}
}
Button(/* ...snip*/) {
//...snip
}
}

View Slide

Major issues
1. Composable functions that draw UI must not return values

2. Implementing the login process in an unnecessarily complex way

3. Do not use `runBlocking` for asynchronous processing

View Slide

Issue: Composable functions that draw UI must not return values
@Composable
fun LoginScreen() {
Column(
modifier = Modifier.padding(DEFAULT_SIZE.dp)
) {
Spacer(
)
title()
Spacer(
)
val email = email()
Spacer(
modifier = Modifier.height(DEFAULT_SIZE.dp)
)
val password = password()
Spacer(
)
loginButton(
email = email,
password = password
)
}
}

View Slide

Issue: Composable functions that draw UI must not return values
@Composable
private fun password(): String {
var password by remember {
mutableStateOf("")
}
TextField(
value = password,
onValueChange = { password = it },
visualTransformation =
PasswordVisualTransformation()
)
return password
}
@Composable
private fun email(): String {
var email by remember {
mutableStateOf("")
}
TextField(
value = email,
onValueChange = { email = it },
placeholder = { Text("Email") }
)
return email
}

View Slide

Solution: Handle value on the caller side and pass value as an argument
@Composable
private fun Password(
password: String,
onPasswordChange: (String) -> Unit,
modifier: Modifier = Modifier
) {
TextField(
modifier = modifier.fillMaxWidth(),
value = password,
onValueChange = onPasswordChange,
visualTransformation = PasswordVisualTransformation()
)
}
@Composable
private fun Email(
email: String,
onEmailChange: (String) -> Unit,
){
TextField(
value = email,
onValueChange = onEmailChange,
placeholder = { Text("Email") },
singleLine = true
)
}

View Slide

Major issues



View Slide

Issue: Implementing the login process in an unnecessarily complex way
@Composable
password: String,
email: String,
) {
val onBackPressedDispatcher = LocalOnBackPressedDispatcherOwner.current?.onBackPressedDispatcher
if (clicked) {
runBlocking {
onBackPressedDispatcher?.onBackPressed()
}
}
}
Button(
onClick = { clicked = true }
) {
Text(
modifier = modifier,
text = "Login"
)
}
}

View Slide

Solution: Apply `state hoisting` strategy
fun LoginScreen() {
//...snip
var email by remember { mutableStateOf("") }
var password by remember { mutableStateOf("") }
LoginButton(
onClick = { viewModel.login(email, password) }
)
}
@Composable
private fun LoginButton(
onClick: () -> Unit,
) {
Button(
onClick = onClick
) {
Text(text = "Login")
}
}

View Slide

Major issues



View Slide

Issue: Do not use runBlocking for asynchronous processing
@Composable
password: String,
email: String,
) {
val onBackPressedDispatcher = LocalOnBackPressedDispatcherOwner.current?.onBackPressedDispatcher
if (clicked) {
runBlocking {
onBackPressedDispatcher?.onBackPressed()
}
}
}
Button(
onClick = { clicked = true }
) {
Text(
modifier = modifier,
text = "Login"
)
}
}

View Slide

Solution: Use appropriate CoroutineScope
fun LoginScreen() {
//...snip
LoginButton(
)
}
@Composable
) {
Button(
onClick = onClick
) {
}
}

View Slide

Solution: Use appropriate CoroutineScope
fun LoginScreen() {
//...snip
LoginButton(
)
}
@Composable
) {
Button(
onClick = onClick
) {
}
}
val scope = rememberCoroutineScope()
scope.launch {
// Do something
}

View Slide

Other issues

• Should follow the convention for Composable

• Naming

• Argument Order

• Only one UI Composable should be placed at the root

• Modi
fi
er argument is speci
fi
ed only for the outermost UI component

• Meaningless constants should be stopped

• Use rememberSavable or SavedStateHandle instead of remember

• Use keyboardOptions and keyboardActions

View Slide

Questions




5. Bitter Sweets/

View Slide

Slip Through My Async
A repository class which handles shop item
/**
* A repository class to save/load a [ShopItem] to the local storage.
*/
class ShopItemListRepository(
private val shopItemDao: ShopItemDao,
private val shopItemThumbnailDao: ShopItemThumbnailDao,
private val errorReporter: ErrorReporter
) {
/** [CoroutineScope] for DAO requests. This is to make jobs non-cancellable and running IO dispatcher. */
private val scope: CoroutineScope = CoroutineScope(NonCancellable + Dispatchers.IO)
/**
* Stores the given [ShopItem] to the local storage with a thumbnail. Then, returns true iff the item and thumbnail
* have been stored successfully.
*
* Even if the caller coroutine is cancelled, this contitues to storing data to prevent a race condition between
* `ShopItem` and the thumbnail. This function is safe to call concurrently because of the non-cancellable coroutine
* scope.
*/
suspend fun store(shopItem: ShopItem): Boolean { /* snip */ }
private suspend fun createThumbnail(shopItem: ShopItem): Bitmap { /* snip */ }
}

View Slide

// Old Java code
public class ShopItemThumbnailDao {
public void registerCallbackFor(@NonNull ShopItem item, @NonNull Consumer callback) { /* snip */ }
public void save(@NonNull ShopItem item, @NonNull Bitmap thumbnail) { /* snip */ }
public void unregisterCallback(@NonNull Consumer callback) { /* snip */ }
}

View Slide

suspend fun store(shopItem: ShopItem): Boolean {
val thumbnail = createThumbnail(shopItem)
val differed = scope.async {
// Step 1: Store thumbnail.
// `suspendCoroutine` is to convert the callback interface of `shopItemThumbnailDao` to coroutine.
var thumbnailCallback: ((Boolean) -> Unit)? = null
val isThumbnailStoreSuccessful = suspendCoroutine { continuation ->
val callback = { isSuccessful: Boolean -> continuation.resume(isSuccessful) }
thumbnailCallback = callback
shopItemThumbnailDao.registerCallbackFor(shopItem, callback)
shopItemThumbnailDao.save(shopItem, thumbnail)
}
val callback = thumbnailCallback
if (callback != null) {
shopItemThumbnailDao.unregisterCallback(callback)
}
if (!isThumbnailStoreSuccessful) {
errorReporter.report(ErrorTag.SHOP_ITEM, "Failed to store thumbnail")
return@async false
}
//…snip
}
}

View Slide

// …snip
// Step 2: Store shop item.
shopItemDao.upsert(shopItem)
return@async true
}
return try {
differed.await()
} catch (exception: DataBaseQueryException) {
// At the step 2, this exception may be thrown.
errorReporter.report(ErrorTag.SHOP_ITEM, "Failed to store an item", exception)
false
}
}

View Slide

Major issues
1. Cause the app to crash

2. Hard to understand what this function does

3. Race condition occurs

View Slide

Issue: Causes the app to crash
// …snip
return@async true
}
return try {
differed.await()
false
}
}

View Slide

Solution: Make try-catch scope inside `async`
val deferred = scope.async {
// …snip
try {
shopItemDao.upsert(shopItem
true
false
}
}
return deferred.await()
}

View Slide

Major issues
1. Cause the app to crash

2. Hard to understand what this function does

3. Race condition occurs

View Slide

Issue: Hard to understand what this function does
// Step 1: Store thumbnail.
// `suspendCoroutine` is to convert the callback interface of `shopItemThumbnailDao` to coroutine.
var thumbnailCallback: ((Boolean) -> Unit)? = null
val isThumbnailStoreSuccessful = suspendCoroutine { continuation ->
val callback = { isSuccessful: Boolean -> continuation.resume(isSuccessful) }
thumbnailCallback = callback
shopItemThumbnailDao.registerCallbackFor(shopItem, callback)
shopItemThumbnailDao.save(shopItem, thumbnail)
}
val callback = thumbnailCallback
if (callback != null) {
shopItemThumbnailDao.unregisterCallback(callback)
}
return@async false
}
//…snip
}
}

View Slide

Issue: Hard to understand what this function does
// …snip
return@async true
}
return try {
differed.await()
false
}
}

View Slide

Solution: Split functions to ensure readability
val differed = scope.asyncWithLock(mutex) {
val isThumbnailStoreSuccessful = storeThumbnail(shopItem)
return@asyncWithLock false
}
val isShopItemStoreSuccessful = storeShopItem(shopItem)
if (!isShopItemStoreSuccessful) {
errorReporter.report(ErrorTag.SHOP_ITEM, "Failed to store item")
deleteThumbnail(shopItem)
return@asyncWithLock false
}
true
}
return differed.await()
}

View Slide

Other issues

• NonCancellable is not needed

• Too wide scope of Dispatcher.IO

• Too wide scope of thumbnailCallback

• Unregistering callback does not work properly

• Thumbnail
fi
le remains if upsert fails

• Unnecessary return@async

View Slide

Questions




5. Bitter Sweets/

View Slide

Caught on Running
A repository class which gets user data from remote server
interface UserDataApi {
suspend fun findUser(userId: UserId): UserData
suspend fun findUser(userName: UserName): UserData
}
class UserDataApiImpl : UserDataApi {
override suspend fun findUser(userId: UserId): UserData =
TODO("Call remote api")
override suspend fun findUser(userName: UserName): UserData =
}
class UserDataRepository(private val userDataApi: UserDataApi = UserDataApiImpl()) {
suspend fun getUserData(userId: UserId): Result =
runCatching { userDataApi.findUser(userId) }
suspend fun getUserData(userName: UserName): Result =
runCatching { userDataApi.findUser(userName) }
}

View Slide

Caught on Running
A repository class which gets user data from remote server
data class UserData(
val id: String,
val userName: UserName,
val userType: UserType,
val profileImage: Bitmap
)
value class UserId(val value: String)
value class UserName(val value: String)
sealed class UserType {
object ADMIN : UserType()
object NORMAL : UserType()
}
suspend fun findUser(userId: UserId): UserData
suspend fun findUser(userName: UserName): UserData
}
class UserDataApiImpl : UserDataApi {
override suspend fun findUser(userId: UserId): UserData =
override suspend fun findUser(userName: UserName): UserData =
}
}

View Slide

Caught on Running
A repository is used from SomeActivity
class SomeActivity : AppCompatActivity() {
private val logger: Logger = Logger()
private val userDataRepository = UserDataRepository()
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
setContentView(R.layout.activity_huge_legacy)
val userId = intent.getStringExtra(USER_ID) ?: "failed"
UpdateUserData(userId)
}
fun UpdateUserData(userId: String) {
val textView = findViewById(R.id.text)
lifecycleScope.launch(Dispatchers.Main) {
if (userId != "falled") {
userDataRepository.getUserData(UserId(userId))
.onSuccess { userData -> textView.text = userData.userName.value }
.onFailure {
Toast.makeText(this@SomeActivity, "Something wrong with it.", Toast.LENGTH_SHORT).show()
logger.sendLog()
}
}
}
}
companion object { const val USER_ID = "user_name" }
}

View Slide

Major issues
1. Do not use `runCatching`

2. Do not overload functions

3. Data class has value that doesn’t have implementation of equals

View Slide

Caught on Running
Issue: Do not use `runCatching`
}

View Slide

Caught on Running

.equals( )
}
suspend fun getUserData(userId: UserId): Result = try {
Result.success(userDataApi.findUser(userId))
} catch (e: Throwable) {
Result.failure(e)
}

View Slide

}
Caught on Running

.equals( )
Result.failure(e)
}
Throwable
Exception
Error
RuntimeException
IOException
NullPointerException CancellationException

View Slide

}
Caught on Running

.equals( )
Result.failure(e)
}
This is System Error.
Don’t catch!
Throwable
Exception
Error
RuntimeException
IOException

View Slide

}
Caught on Running

.equals( )
Result.failure(e)
}
Don’t catch!
This is used for
coroutine cancellation.
Don’t catch!
Throwable
Exception
Error
RuntimeException
IOException

View Slide

}
Caught on Running

.equals( )
Result.failure(e)
}
Don’t catch!
Usually, this is caused by bugs.
Don’t catch!
Don’t catch!
Throwable
Exception
Error
RuntimeException
IOException

View Slide

}
Caught on Running

.equals( )
Result.failure(e)
}
Don’t catch!
Don’t catch!
Don’t catch!
You should catch this!
Throwable
Exception
Error
RuntimeException
IOException

View Slide

Caught on Running

}

View Slide

Caught on Running
Solution: Use `withContext` and minimal `try-catch` scope
class UserDataRepository(
private val userDataApi: UserDataApi = UserDataApiImpl(),
private val ioDispatcher: CoroutineDispatcher = Dispatchers.IO
) {
suspend fun getUserData(userId: UserId): UserData? = try {
withContext(ioDispatcher) {
userDataApi.findUser(userId)
}
null
}
suspend fun getUserData(userName: UserName): UserData? = try {
userDataApi.findUser(userName)
}
null
}
}

View Slide

Major issues



View Slide

Caught on Running
Issue: Do not overload functions
) {
}
null
}
}
null
}
}

View Slide

Caught on Running
) {
}
null
}
}
null
}
}
suspend fun findUser(userId: UserId): UserData?
suspend fun findUser(userName: UserName): UserData?
}

View Slide

) {
}
null
}
}
null
}
}
Caught on Running
public interface UserDataApi {
public @Nullable UserData findUser(String userId);
public @Nullable UserData findUser(String userName);
}
}

View Slide

) {
}
null
}
}
null
}
}
Caught on Running
}
}
String userId = "userId";
userDataRepository.getUserData(userId, new Continuation>() {
@NonNull
@Override
public CoroutineContext getContext() {
// ...snip
}
@Override
public void resumeWith(@NonNull Object o) {
// ...snip
}
});

View Slide

) {
}
null
}
}
null
}
}
Caught on Running
}
}
String userId = "userId";
userDataRepository.getUserData(userId, new Continuation>() {
@NonNull
@Override
public CoroutineContext getContext() {
// ...snip
}
@Override
public void resumeWith(@NonNull Object o) {
// ...snip
}
}); Ambiguous method call. Both

getUserData (String,Continuation super Result extends UserData>>) in UserDataRepository and

getUserData(String,Continuation super Result extends UserData>>)in UserDataRepository match

View Slide

) {
}
null
}
}
null
}
}
Caught on Running
}
}

View Slide

) {
suspend fun getUserDataById(userId: UserId): UserData? = try {
}
null
}
suspend fun getUserDataByName(userName: UserName): UserData? = try {
}
null
}
}
Caught on Running
Solution: Make appropriate name for functions without overloading
public @Nullable UserData findUserById(String userId);
public @Nullable UserData findUserByName(String userName);
}
suspend fun findUserById(userId: UserId): UserData?
suspend fun findUserByName(userName: UserName): UserData?
}

View Slide

Caught on Running
Solution: Make appropriate name for functions without overloading
public @Nullable UserData findUserById(String userId);
public @Nullable UserData findUserByName(String userName);
}
) {
suspend fun getUserDataById(userId: UserId): UserData? = try {
}
null
}
suspend fun getUserDataByName(userName: UserName): UserData? = try {
}
null
}
}
suspend fun findUserById(userId: UserId): UserData?
suspend fun findUserByName(userName: UserName): UserData?
}

View Slide

Major issues



View Slide

Caught on Running
Issue: Data class has value that doesn’t have implementation of equals
val id: UserId,
)

View Slide

Issue: Data class has value that doesn’t have implementation of equals
Caught on Running
val id: UserId,
){
fun equals(other: Any?): Boolean { }
fun hashCode(): Int { }
fun toString(): String { }
}

View Slide

Caught on Running
Solution: Use ordinal class
class UserData(
val id: UserId,
)

View Slide

Caught on Running
Solution: Use class for which equals is implemented.
val id: UserId,
val profileImageUrl: String
)

View Slide

Caught on Running
Other issues

• Bug on intent value handling

• UserType class is enough to be an enum class

• UserData.id should use UserId class if we make this class

• Need @JvmInline annotation to value class

• Minimize the function's public range

• Logger has ambiguous function name

• Unnecessary deep nests

• Unnecessary Dispatchers.Main

View Slide

Questions




5. Bitter Sweets

View Slide

Bitter Sweets
An activity which receives Parcelable request
class MainActivity : AppCompatActivity() {
setContentView(R.layout.activity_main)
val request: Request = if (Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU) {
@Suppress("DEPRECATION")
intent.getParcelableExtra("request")
} else {
intent.getParcelableExtra("request", Request::class.java)
} ?: return
val viewModel = MainViewModel()
lifecycleScope.launch {
viewModel.createDataFlow(request).collect(::updateView)
}
}
private fun updateView(viewData: String) {
// TODO: Update Views by [viewData].
}
companion object {
fun createIntent(context: Context, request: Request): Intent =
Intent(context, MainActivity::class.java)
.putExtra("request", request)
}
}

View Slide

Bitter Sweets
class MainViewModel : ViewModel() {
suspend fun createDataFlow(request: Request): Flow {
val localCacheDeferred = loadDataAsync(request, true)
val remoteCacheDeferred = loadDataAsync(request, false)
return flow {
emit(localCacheDeferred.await())
emit(remoteCacheDeferred.await())
}
}
private suspend fun loadDataAsync(request: Request, local: Boolean): Deferred =
coroutineScope { async { loadData(request, local) } }
private suspend fun loadData(request: Request, local: Boolean): String {
// TODO: This is dummy. Load data from local cache or remote.
delay(if (local) 1000L else 5000L)
return "result of '$request'"
}
}

View Slide

Bitter Sweets
data class Request(val word: String, val page: Int) : Parcelable {
override fun writeToParcel(parcel: Parcel, flags: Int) {
parcel.writeString(word)
parcel.writeInt(page)
}
override fun describeContents(): Int = 0
companion object {
@JvmField
val CREATOR = RequestCreator
}
}
object RequestCreator : Parcelable.Creator {
override fun createFromParcel(parcel: Parcel): Request =
Request(checkNotNull(parcel.readString()), parcel.readInt())
override fun newArray(size: Int): Array = arrayOfNulls(size)
}

View Slide

Major issues
1. Unintentional synchronous execution of async functions

2. OS bug regarding Parcelable that occurs only in Android Tiramisu

View Slide

Bitter Sweets
Issue: Unintentional synchronous execution of async functions
class MainViewModel : ViewModel() {
suspend fun createDataFlow(request: Request): Flow {
val localCacheDeferred = loadDataAsync(request, true)
val remoteCacheDeferred = loadDataAsync(request, false)
return flow {
emit(localCacheDeferred.await())
emit(remoteCacheDeferred.await())
}
}
private suspend fun loadDataAsync(request: Request, local: Boolean): Deferred =
coroutineScope { async { loadData(request, local) } }
private suspend fun loadData(request: Request, local: Boolean): String {
// TODO: This is dummy. Load data from local cache or remote.
delay(if (local) 1000L else 5000L)
return "result of '$request'"
}
}

View Slide

Bitter Sweets
Solution: Use coroutine scope correctly
class MainRepository {
fun createDataFlow(request: Request): Flow = flow {
coroutineScope {
val localCacheDataDeferred = loadCacheDataAsync(request)
val remoteDataDeferred = fetchRemoteDataAsync(request)
emit(localCacheDataDeferred.await())
emit(remoteDataDeferred.await())
}
}
private fun CoroutineScope.loadCacheDataAsync(request: Request): Deferred =
async { loadCacheData(request) }
private fun CoroutineScope.fetchRemoteDataAsync(request: Request): Deferred =
async { fetchRemoteData(request) }
private suspend fun loadCacheData(request: Request): String {
delay(1000)
return "local cache result of '$request'"
}
private suspend fun fetchRemoteData(request: Request): String {
delay(5000)
return "remote result of '$request'"
}
}

View Slide

Major issues
1. Unintentional synchronous execution of async functions

2. OS bug regarding Parcelable that occurs only in Android Tiramisu

View Slide

Bitter Sweets
Issue: OS bug regarding Parcelable that occurs only in Android Tiramisu
} else {
} ?: return
}
}
}
companion object {
}
}

View Slide

Bitter Sweets
Issue: OS bug regarding Parcelable that occurs only in Android Tiramisu
} else {
} ?: return
}
}
}
companion object {
}
}
data class Request(val word: String, val page: Int) : Parcelable {
override fun writeToParcel(parcel: Parcel, flags: Int) {
parcel.writeString(word)
parcel.writeInt(page)
}
override fun describeContents(): Int = 0
companion object {
@JvmField
val CREATOR = RequestCreator
}
}
object RequestCreator : Parcelable.Creator {
override fun createFromParcel(parcel: Parcel): Request =
Request(checkNotNull(parcel.readString()), parcel.readInt())
override fun newArray(size: Int): Array = arrayOfNulls(size)
}

View Slide

Bitter Sweets
Solution: Use IntentCompat (and kotlinx-parcelize)
class MainViewModel(
savedStateHandle: SavedStateHandle,
private val repository: MainRepository
) {
private val request: Request = checkNotNull(savedStateHandle[KEY_REQUEST])
val viewDataFlow: Flow by lazy { repository.createDataFlow(request) }
object Factory : ViewModelProvider.Factory {
override fun create(modelClass: Class, extras: CreationExtras): T {
return MainViewModel(extras.createSavedStateHandle(), MainRepository()) as T
}
}
companion object {
const val KEY_REQUEST = "request"
}
}
@Parcelize
@Keep
data class Request(val word: String, val page: Int) : Parcelable

View Slide

Bitter Sweets
Solution: Use IntentCompat (and kotlinx-parcelize)
class MainViewModel(
savedStateHandle: SavedStateHandle,
private val repository: MainRepository
) {
private val request: Request = checkNotNull(savedStateHandle[KEY_REQUEST])
val viewDataFlow: Flow by lazy { repository.createDataFlow(request) }
object Factory : ViewModelProvider.Factory {
override fun create(modelClass: Class, extras: CreationExtras): T {
return MainViewModel(extras.createSavedStateHandle(), MainRepository()) as T
}
}
companion object {
const val KEY_REQUEST = "request"
}
}
@Parcelize
@Keep
data class Request(val word: String, val page: Int) : Parcelable
IntentCompat.getParcelableExtra(intent, name, class)

View Slide

Bitter Sweets
Other issues

• Data is discarded when rotating the screen, etc.

• Should use Parcelize library to reduce boilerplate codes

• Should use SavedStateHandle when intent use on ViewModel

• Activity may be displayed in a halfway initialized state

• Should make constant value instead of direct value

• Should make repository class

• Flow on lifecycleScope might be run even if state is STOPPED

• Unnecessary suspendable function to provide
fl
ow

View Slide

How did we create the question?

View Slide

We have the “Review committe”

View Slide

Review committee
Team A
Team B
Committee members
Ref: Confronting quality problems of long-lived codebase, Munetoshi Ishikawa, LINE Corporation https://www.youtube.com/watch?v=z8PwD68_zwg

View Slide

Review committee
1. Committee members review “merged” pull-requests
Team A
Team B
Committee members
Review merged PRs

View Slide

Review committee

2. Committee members then write and share a weekly report
Team A
Team B
Committee members
Review merged PRs
write report

View Slide

Review committee

2. Committee members then write and share a weekly report
Team A
Team B
Committee members
Review merged PRs
write report
share

View Slide

How to make the question?
Apply the weekly report in reverse to create a "Bad Code".
Code maker
Weekly reports

View Slide

Code maker
Weekly reports
Make “Bad Code”
Questions

View Slide

Code maker
Weekly reports
Make “Bad Code”
class Ugly1 {
fun dirtyFun1() { /* */ }
}
Questions

View Slide

class Ugly1 {
}
Code maker
Weekly reports
Make “Bad Code”
Questions
class Ugly2 {
}

View Slide

Code maker
Weekly reports
Make “Bad Code”
Questions
class Ugly1 {
}
class Ugly2 {
} class Ugly3 {
}

View Slide

Code maker
Weekly reports
Make “Bad Code”
Questions
class Ugly1 {
}
class Ugly2 {
} class Ugly3 {
} class Ugly4 {
}

View Slide

Code maker
Weekly reports
Make “Bad Code”
Questions
class Ugly1 {
}
class Ugly2 {
} class Ugly3 {
} class Ugly4 {
} class Ugly5 {
}

View Slide

White board
class Ugly1 {
}
class Ugly2 {
}
class Ugly3 {
}
class Ugly4 {
}
class Ugly5 {
}

View Slide

Want to learn more?

View Slide

Watch this video of
DroidKaigi 2021

View Slide

Thank you for watching!

View Slide

Code Review Challenge: An example of a solution

Code Review Challenge: An example of a solution

LINE Developers
PRO

More Decks by LINE Developers

Other Decks in Technology

Featured

Transcript

Code Review Challenge: An example of a solution

Code Review Challenge: An example of a solution

LINE Developers PRO

More Decks by LINE Developers

Other Decks in Technology

Featured

Transcript

LINE Developers
PRO