Code Review Challenge: An example of a solution

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

Self-introduction Name: Yuki Ando / ҆౻ ༞و  Twitter: @m1zyuk1 Hobby:
Car, Game Main activity:   Apply new Android feature to LINE Android

Did you participate in DroidKaigi 2023 offline?

Yes I am “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’s Code Review Challenge?

Code Review Challenge Review “Bad Code” to be a “Good
Code” LINE Booth in DroidKaigi 2023 class Ugly { fun dirtyFunc() { /* */ } } Challengers “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/

Code” LINE Booth in DroidKaigi 2023 class Ugly { fun dirtyFunc() { /* */ } } Challengers Review “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/

Code” LINE Booth in DroidKaigi 2023 class Ugly { fun dirtyFunc() { /* */ } } Review Review ideas Challengers “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/

Code” LINE Booth in DroidKaigi 2023 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/

Explain the assumed solutions

Full codes are hereɹɹɹ Ref: https://engineering.linecorp.com/ja/blog/droidKaigi-2023 “Twemoji” ©Twitter, Inc and
other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/

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

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 }

to do the back up process object BackupCreator { private val localDataSource = LocalDataSource() private val remoteDataSource = RemoteDataSource() private lateinit var progressDialog: ProgressDialogForBackup 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) } }

to do the back up process private fun writeToFile(file: File, data: BackupData) { val outputStream = file.outputStream() try { outputStream.write(data.toByteArray()) } catch (e: IOException) { // Ignore } outputStream.close() }

Major issues 1. OutputStream is not handled properly 2. Signatures
that can be used incorrectly 3. Structure of functions with poor readability

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

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

Tip of the Ice-func Solution: Use closable classes with `use`
phrase // Throws [IOException] if any I/O error occurred. private fun writeToFile(file: File, data: BackupData) { file.outputStream().use { outputStream -> outputStream.write(data.toByteArray()) } }

Tip of the Ice-func Solution: Handle IOException such as canceling
process // Throws [IOException] if any I/O error occurred. private fun writeToFile(file: File, data: BackupData) { file.outputStream().use { outputStream -> outputStream.write(data.toByteArray()) } } private fun collectBackupData(target: BackupTarget, since: Date) { val data = when (target) { BackupTarget.IMAGE -> localDataSource.selectImageData() BackupTarget.VIDEO -> localDataSource.selectVideoData() BackupTarget.MESSAGE -> localDataSource.selectMessageData(since) } try { //...snip writeToFile(File("path/raw"), data) } catch (e: IOException){ // handle IO Exception } }

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

Tip of the Ice-func Issue: Signatures that can be used
incorrectly object BackupCreator { private val localDataSource = LocalDataSource() private val remoteDataSource = RemoteDataSource() private lateinit var progressDialog: ProgressDialogForBackup @WorkerThread fun createBackup(target: BackupTarget, since: Date) { //...snip }

incorrectly object BackupCreator { private val localDataSource = LocalDataSource() private val remoteDataSource = RemoteDataSource() private lateinit var progressDialog: ProgressDialogForBackup @WorkerThread fun createBackup(target: BackupTarget, since: Date) { //...snip } enum class BackupTarget { IMAGE, VIDEO, MESSAGE }

incorrectly object BackupCreator { private val localDataSource = LocalDataSource() private val remoteDataSource = RemoteDataSource() private lateinit var progressDialog: ProgressDialogForBackup @WorkerThread fun createBackup(target: BackupTarget, since: Date) { //...snip } `since` is not affected for image/video. But interface doesn’t indicate this. enum class BackupTarget { IMAGE, VIDEO, MESSAGE }

incorrectly private fun collectBackupData(target: BackupTarget, since: Date) { val data = when (target) { BackupTarget.IMAGE -> localDataSource.selectImageData() BackupTarget.VIDEO -> localDataSource.selectVideoData() BackupTarget.MESSAGE -> localDataSource.selectMessageData(since) } try { //...snip writeToFile(File("path/raw"), data) } catch (e: IOException){ // handle IO Exception } } `since` is not affected for image/video. But interface doesn’t indicate this. enum class BackupTarget { IMAGE, VIDEO, MESSAGE }

Tip of the Ice-func Solution: Use sealed class to represent
behavior correctly by interface private fun collectBackupData(target: BackupTarget) { val data = when (target) { BackupTarget.IMAGE -> localDataSource.selectImageData() BackupTarget.VIDEO -> localDataSource.selectVideoData() BackupTarget.MESSAGE -> localDataSource.selectMessageData(target.since) } try { //...snip writeToFile(File("path/raw"), data) } catch (e: IOException){ // handle IO Exception } } sealed class BackupTarget { data object Image : BackupTarget() data object Video : BackupTarget() data class Message(val since: Date) : BackupTarget() }

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

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 }

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

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

everything, and everything in its Composable 3. Slip Through My Async 4. Caught on Running 5. Bitter Sweets/

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( modifier = Modifier.weight(1f) ) val email = email() Spacer( modifier = Modifier.height(DEFAULT_SIZE.dp) ) val password = password() Spacer( modifier = Modifier.weight(2f) ) loginButton( email = email, password = password ) } }

login screen which constructed by Composable function @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 }

login screen which constructed by Composable function @Composable private fun password(): String { var password by remember { mutableStateOf("") } TextField( modifier = Modifier.fillMaxWidth(), 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 } }

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

A Composable for everything, and everything in its Composable Issue:
Composable functions that draw UI must not return values @Composable fun LoginScreen() { Column( modifier = Modifier.padding(DEFAULT_SIZE.dp) ) { Spacer( modifier = Modifier.weight(2f) ) title() Spacer( modifier = Modifier.weight(1f) ) val email = email() Spacer( modifier = Modifier.height(DEFAULT_SIZE.dp) ) val password = password() Spacer( modifier = Modifier.weight(2f) ) loginButton( email = email, password = password ) } }

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

A Composable for everything, and everything in its Composable 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, placeholder = { Text("Password") }, visualTransformation = PasswordVisualTransformation() ) } @Composable private fun Email( email: String, onEmailChange: (String) -> Unit, modifier: Modifier = Modifier ){ TextField( modifier = modifier.fillMaxWidth(), value = email, onValueChange = onEmailChange, placeholder = { Text("Email") }, singleLine = true ) }

Implementing the login process in an unnecessarily complex way @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( modifier = modifier.fillMaxWidth(), onClick = { clicked = true } ) { Text( modifier = modifier, text = "Login" ) } }

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, modifier: Modifier = Modifier ) { Button( modifier = modifier.fillMaxWidth(), onClick = onClick ) { Text(text = "Login") } }

Do not use runBlocking for asynchronous processing @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( modifier = modifier.fillMaxWidth(), onClick = { clicked = true } ) { Text( modifier = modifier, text = "Login" ) } }

Use appropriate CoroutineScope 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, modifier: Modifier = Modifier ) { Button( modifier = modifier.fillMaxWidth(), onClick = onClick ) { Text(text = "Login") } }

Use appropriate CoroutineScope 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, modifier: Modifier = Modifier ) { Button( modifier = modifier.fillMaxWidth(), onClick = onClick ) { Text(text = "Login") } } val scope = rememberCoroutineScope() scope.launch { // Do something }

A Composable for everything, and everything in its Composable 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

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

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

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

item suspend fun store(shopItem: ShopItem): Boolean { val thumbnail = createThumbnail(shopItem) val differed = scope.async { // …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 } }

Major issues 1. Cause the app to crash 2. Hard
to understand what this function does 3. Race condition occurs

Slip Through My Async Issue: Causes the app to crash
suspend fun store(shopItem: ShopItem): Boolean { val thumbnail = createThumbnail(shopItem) val differed = scope.async { // …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 } }

Slip Through My Async Solution: Make try-catch scope inside `async`
suspend fun store(shopItem: ShopItem): Boolean { val thumbnail = createThumbnail(shopItem) val deferred = scope.async { // …snip // Step 2: Store shop item. try { shopItemDao.upsert(shopItem true } catch (exception: DataBaseQueryException) { errorReporter.report(ErrorTag.SHOP_ITEM, "Failed to store an item", exception) false } } return deferred.await() }

Major issues 1. Cause the app to crash 2. Hard
to understand what this function does 3. Race condition occurs

Slip Through My Async Issue: Hard to understand what this
function does 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 } }

Slip Through My Async Issue: Hard to understand what this
function does suspend fun store(shopItem: ShopItem): Boolean { val thumbnail = createThumbnail(shopItem) val differed = scope.async { // …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 } }

Slip Through My Async Solution: Split functions to ensure readability
suspend fun store(shopItem: ShopItem): Boolean { val differed = scope.asyncWithLock(mutex) { val isThumbnailStoreSuccessful = storeThumbnail(shopItem) if (!isThumbnailStoreSuccessful) { errorReporter.report(ErrorTag.SHOP_ITEM, "Failed to store thumbnail") 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() }

A Composable for everything, and everything in its Composable 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

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 = TODO("Call remote api") } class UserDataRepository(private val userDataApi: UserDataApi = UserDataApiImpl()) { suspend fun getUserData(userId: UserId): Result<UserData> = runCatching { userDataApi.findUser(userId) } suspend fun getUserData(userName: UserName): Result<UserData> = runCatching { userDataApi.findUser(userName) } }

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() } 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 = TODO("Call remote api") } class UserDataRepository(private val userDataApi: UserDataApi = UserDataApiImpl()) { suspend fun getUserData(userId: UserId): Result<UserData> = runCatching { userDataApi.findUser(userId) } suspend fun getUserData(userName: UserName): Result<UserData> = runCatching { userDataApi.findUser(userName) } }

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<TextView>(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" } }

Major issues 1. Do not use `runCatching` 2. Do not
overload functions 3. Data class has value that doesn’t have implementation of equals

Caught on Running Issue: Do not use `runCatching` class UserDataRepository(private
val userDataApi: UserDataApi = UserDataApiImpl()) { suspend fun getUserData(userId: UserId): Result<UserData> = runCatching { userDataApi.findUser(userId) } suspend fun getUserData(userName: UserName): Result<UserData> = runCatching { userDataApi.findUser(userName) } }

Caught on Running Issue: Do not use `runCatching` .equals( )
class UserDataRepository(private val userDataApi: UserDataApi = UserDataApiImpl()) { suspend fun getUserData(userId: UserId): Result<UserData> = runCatching { userDataApi.findUser(userId) } suspend fun getUserData(userName: UserName): Result<UserData> = runCatching { userDataApi.findUser(userName) } } suspend fun getUserData(userId: UserId): Result<UserData> = try { Result.success(userDataApi.findUser(userId)) } catch (e: Throwable) { Result.failure(e) }

class UserDataRepository(private val userDataApi: UserDataApi = UserDataApiImpl()) { suspend fun
getUserData(userId: UserId): Result<UserData> = runCatching { userDataApi.findUser(userId) } suspend fun getUserData(userName: UserName): Result<UserData> = runCatching { userDataApi.findUser(userName) } } Caught on Running Issue: Do not use `runCatching` .equals( ) suspend fun getUserData(userId: UserId): Result<UserData> = try { Result.success(userDataApi.findUser(userId)) } catch (e: Throwable) { Result.failure(e) } Throwable Exception Error RuntimeException IOException NullPointerException CancellationException

getUserData(userId: UserId): Result<UserData> = runCatching { userDataApi.findUser(userId) } suspend fun getUserData(userName: UserName): Result<UserData> = runCatching { userDataApi.findUser(userName) } } Caught on Running Issue: Do not use `runCatching` .equals( ) suspend fun getUserData(userId: UserId): Result<UserData> = try { Result.success(userDataApi.findUser(userId)) } catch (e: Throwable) { Result.failure(e) } This is System Error. Don’t catch! Throwable Exception Error RuntimeException IOException NullPointerException CancellationException

getUserData(userId: UserId): Result<UserData> = runCatching { userDataApi.findUser(userId) } suspend fun getUserData(userName: UserName): Result<UserData> = runCatching { userDataApi.findUser(userName) } } Caught on Running Issue: Do not use `runCatching` .equals( ) suspend fun getUserData(userId: UserId): Result<UserData> = try { Result.success(userDataApi.findUser(userId)) } catch (e: Throwable) { Result.failure(e) } Don’t catch! This is used for coroutine cancellation. Don’t catch! Throwable Exception Error RuntimeException IOException NullPointerException CancellationException

getUserData(userId: UserId): Result<UserData> = runCatching { userDataApi.findUser(userId) } suspend fun getUserData(userName: UserName): Result<UserData> = runCatching { userDataApi.findUser(userName) } } Caught on Running Issue: Do not use `runCatching` .equals( ) suspend fun getUserData(userId: UserId): Result<UserData> = try { Result.success(userDataApi.findUser(userId)) } catch (e: Throwable) { Result.failure(e) } Don’t catch! Usually, this is caused by bugs. Don’t catch! Don’t catch! Throwable Exception Error RuntimeException IOException NullPointerException CancellationException

getUserData(userId: UserId): Result<UserData> = runCatching { userDataApi.findUser(userId) } suspend fun getUserData(userName: UserName): Result<UserData> = runCatching { userDataApi.findUser(userName) } } Caught on Running Issue: Do not use `runCatching` .equals( ) suspend fun getUserData(userId: UserId): Result<UserData> = try { Result.success(userDataApi.findUser(userId)) } catch (e: Throwable) { Result.failure(e) } Don’t catch! Don’t catch! Don’t catch! You should catch this! Throwable Exception Error RuntimeException IOException NullPointerException CancellationException

Caught on Running Issue: Do not use `runCatching` class UserDataRepository(private
val userDataApi: UserDataApi = UserDataApiImpl()) { suspend fun getUserData(userId: UserId): Result<UserData> = runCatching { userDataApi.findUser(userId) } suspend fun getUserData(userName: UserName): Result<UserData> = runCatching { userDataApi.findUser(userName) } }

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) } } catch (e: IOException) { null } suspend fun getUserData(userName: UserName): UserData? = try { withContext(ioDispatcher) { userDataApi.findUser(userName) } } catch (e: IOException) { null } }

Caught on Running Issue: Do not overload functions 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) } } catch (e: IOException) { null } suspend fun getUserData(userName: UserName): UserData? = try { withContext(ioDispatcher) { userDataApi.findUser(userName) } } catch (e: IOException) { null } }

Caught on Running Issue: Do not overload functions 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) } } catch (e: IOException) { null } suspend fun getUserData(userName: UserName): UserData? = try { withContext(ioDispatcher) { userDataApi.findUser(userName) } } catch (e: IOException) { null } } interface UserDataApi { suspend fun findUser(userId: UserId): UserData? suspend fun findUser(userName: UserName): UserData? }

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) } } catch (e: IOException) { null } suspend fun getUserData(userName: UserName): UserData? = try { withContext(ioDispatcher) { userDataApi.findUser(userName) } } catch (e: IOException) { null } } Caught on Running Issue: Do not overload functions public interface UserDataApi { public @Nullable UserData findUser(String userId); public @Nullable UserData findUser(String userName); } interface UserDataApi { suspend fun findUser(userId: UserId): UserData? suspend fun findUser(userName: UserName): UserData? }

ioDispatcher: CoroutineDispatcher = Dispatchers.IO ) { suspend fun getUserData(userId: UserId): UserData? = try { withContext(ioDispatcher) { userDataApi.findUser(userId) } } catch (e: IOException) { null } suspend fun getUserData(userName: UserName): UserData? = try { withContext(ioDispatcher) { userDataApi.findUser(userName) } } catch (e: IOException) { null } } Caught on Running Issue: Do not overload functions public interface UserDataApi { public @Nullable UserData findUser(String userId); public @Nullable UserData findUser(String userName); } interface UserDataApi { suspend fun findUser(userId: UserId): UserData? suspend fun findUser(userName: UserName): UserData? } String userId = "userId"; userDataRepository.getUserData(userId, new Continuation<Result<? extends UserData>>() { @NonNull @Override public CoroutineContext getContext() { // ...snip } @Override public void resumeWith(@NonNull Object o) { // ...snip } });

ioDispatcher: CoroutineDispatcher = Dispatchers.IO ) { suspend fun getUserData(userId: UserId): UserData? = try { withContext(ioDispatcher) { userDataApi.findUser(userId) } } catch (e: IOException) { null } suspend fun getUserData(userName: UserName): UserData? = try { withContext(ioDispatcher) { userDataApi.findUser(userName) } } catch (e: IOException) { null } } Caught on Running Issue: Do not overload functions public interface UserDataApi { public @Nullable UserData findUser(String userId); public @Nullable UserData findUser(String userName); } interface UserDataApi { suspend fun findUser(userId: UserId): UserData? suspend fun findUser(userName: UserName): UserData? } String userId = "userId"; userDataRepository.getUserData(userId, new Continuation<Result<? extends UserData>>() { @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

ioDispatcher: CoroutineDispatcher = Dispatchers.IO ) { suspend fun getUserData(userId: UserId): UserData? = try { withContext(ioDispatcher) { userDataApi.findUser(userId) } } catch (e: IOException) { null } suspend fun getUserData(userName: UserName): UserData? = try { withContext(ioDispatcher) { userDataApi.findUser(userName) } } catch (e: IOException) { null } } Caught on Running Issue: Do not overload functions public interface UserDataApi { public @Nullable UserData findUser(String userId); public @Nullable UserData findUser(String userName); } interface UserDataApi { suspend fun findUser(userId: UserId): UserData? suspend fun findUser(userName: UserName): UserData? }

ioDispatcher: CoroutineDispatcher = Dispatchers.IO ) { suspend fun getUserDataById(userId: UserId): UserData? = try { withContext(ioDispatcher) { userDataApi.findUser(userId) } } catch (e: IOException) { null } suspend fun getUserDataByName(userName: UserName): UserData? = try { withContext(ioDispatcher) { userDataApi.findUser(userName) } } catch (e: IOException) { null } } Caught on Running Solution: Make appropriate name for functions without overloading public interface UserDataApi { public @Nullable UserData findUserById(String userId); public @Nullable UserData findUserByName(String userName); } interface UserDataApi { suspend fun findUserById(userId: UserId): UserData? suspend fun findUserByName(userName: UserName): UserData? }

Caught on Running Solution: Make appropriate name for functions without
overloading public interface UserDataApi { public @Nullable UserData findUserById(String userId); public @Nullable UserData findUserByName(String userName); } class UserDataRepository( private val userDataApi: UserDataApi = UserDataApiImpl(), private val ioDispatcher: CoroutineDispatcher = Dispatchers.IO ) { suspend fun getUserDataById(userId: UserId): UserData? = try { withContext(ioDispatcher) { userDataApi.findUser(userId) } } catch (e: IOException) { null } suspend fun getUserDataByName(userName: UserName): UserData? = try { withContext(ioDispatcher) { userDataApi.findUser(userName) } } catch (e: IOException) { null } } interface UserDataApi { suspend fun findUserById(userId: UserId): UserData? suspend fun findUserByName(userName: UserName): UserData? }

Caught on Running Issue: Data class has value that doesn’t
have implementation of equals data class UserData( val id: UserId, val userName: UserName, val userType: UserType, val profileImage: Bitmap )

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

Caught on Running Solution: Use ordinal class class UserData( val
id: UserId, val userName: UserName, val userType: UserType, val profileImage: Bitmap )

Caught on Running Solution: Use class for which equals is
implemented. data class UserData( val id: UserId, val userName: UserName, val userType: UserType, val profileImageUrl: String )

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

everything, and everything in its Composable 3. Slip Through My Async 4. Caught on Running 5. Bitter Sweets

Bitter Sweets An activity which receives Parcelable request class MainActivity
: AppCompatActivity() { override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) 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) } }

Bitter Sweets An activity which receives Parcelable request class MainViewModel
: ViewModel() { suspend fun createDataFlow(request: Request): Flow<String> { 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<String> = 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'" } }

Bitter Sweets An activity which receives Parcelable request 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<Request> { override fun createFromParcel(parcel: Parcel): Request = Request(checkNotNull(parcel.readString()), parcel.readInt()) override fun newArray(size: Int): Array<Request?> = arrayOfNulls(size) }

Major issues 1. Unintentional synchronous execution of async functions 2.
OS bug regarding Parcelable that occurs only in Android Tiramisu

Bitter Sweets Issue: Unintentional synchronous execution of async functions class
MainViewModel : ViewModel() { suspend fun createDataFlow(request: Request): Flow<String> { 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<String> = 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'" } }

Bitter Sweets Solution: Use coroutine scope correctly class MainRepository {
fun createDataFlow(request: Request): Flow<String> = flow { coroutineScope { val localCacheDataDeferred = loadCacheDataAsync(request) val remoteDataDeferred = fetchRemoteDataAsync(request) emit(localCacheDataDeferred.await()) emit(remoteDataDeferred.await()) } } private fun CoroutineScope.loadCacheDataAsync(request: Request): Deferred<String> = async { loadCacheData(request) } private fun CoroutineScope.fetchRemoteDataAsync(request: Request): Deferred<String> = 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'" } }

Major issues 1. Unintentional synchronous execution of async functions 2.
OS bug regarding Parcelable that occurs only in Android Tiramisu

Bitter Sweets Issue: OS bug regarding Parcelable that occurs only
in Android Tiramisu class MainActivity : AppCompatActivity() { override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) 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) } }

Bitter Sweets Issue: OS bug regarding Parcelable that occurs only
in Android Tiramisu class MainActivity : AppCompatActivity() { override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) 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) } } 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<Request> { override fun createFromParcel(parcel: Parcel): Request = Request(checkNotNull(parcel.readString()), parcel.readInt()) override fun newArray(size: Int): Array<Request?> = arrayOfNulls(size) }

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<String> by lazy { repository.createDataFlow(request) } object Factory : ViewModelProvider.Factory { override fun <T : ViewModel> create(modelClass: Class<T>, 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

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<String> by lazy { repository.createDataFlow(request) } object Factory : ViewModelProvider.Factory { override fun <T : ViewModel> create(modelClass: Class<T>, 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)

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

How did we create the question?

We have the “Review committe”

Review committee Team A Team B Committee members “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. Committee members review “merged” pull-requests Team A
Team B Committee members Review merged PRs “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. Committee members review “merged” pull-requests 2. Committee
members then write and share a weekly report Team A Team B Committee members Review merged PRs 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

Review committee 1. Committee members review “merged” pull-requests 2. Committee
members then write and share a weekly report Team A Team B Committee members Review merged PRs 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 a "Bad Code". Code maker “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Weekly reports

reverse to create a "Bad Code". Code maker “Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/ Weekly reports Make “Bad Code” Questions

reverse to create a "Bad Code". 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

class Ugly1 { fun dirtyFun1() { /* */ } }
How to make the question? Apply the weekly report in reverse to create a "Bad Code". 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() { /* */ } }

reverse to create a "Bad Code". 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() { /* */ } }

reverse to create a "Bad Code". 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() { /* */ } }

reverse to create a "Bad Code". 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() { /* */ } }

reverse to create a "Bad Code". “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() { /* */ } }

Want to learn more?

Watch this video of DroidKaigi 2021 Ref: Confronting quality problems
of long-lived codebase, Munetoshi Ishikawa, LINE Corporation https://www.youtube.com/watch?v=z8PwD68_zwg

Thank you for watching!

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