$30 off During Our Annual Pro Sale. View Details »

Code Review Challenge: An example of a solution

Code Review Challenge: An example of a solution

DroidKaigi 2023: コードレビューチャレンジの問題解説

◾️ 発表者
安藤 祐貴
LINE株式会社 / iOS/Androidエクスペリエンス開発チーム Software(Android) Engineer
2021年にLINEに新卒入社し、LINEアプリのAndroidクライアントについて、主にAndroid OSの新機能を適用する業務に携わっています。車を走らせることが大好きです。
https://x.com/m1zyuk1

こちらは以下のイベントの発表資料です。
https://zozotech-inc.connpass.com/event/295096/

LINE Developers

September 25, 2023
Tweet

More Decks by LINE Developers

Other Decks in Technology

Transcript

  1. Yuki Ando, LINE Corporation Code Review Challenge An example of

    a solution DroidKaigi 2023: ίʔυϨϏϡʔνϟϨϯδͷ໰୊ղઆ
  2. Self-introduction Name: Yuki Ando / ҆౻ ༞و
 Twitter: @m1zyuk1 Hobby:

    Car, Game Main activity: 
 Apply new Android feature to LINE Android
  3. Yes I am “Twemoji” ©Twitter, Inc and other contributors (Licensed

    under CC-BY 4.0) https://twemoji.twitter.com/
  4. 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/
  5. Code Review Challenge Review “Bad Code” to be a “Good

    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/
  6. Code Review Challenge Review “Bad Code” to be a “Good

    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/
  7. Code Review Challenge Review “Bad Code” to be a “Good

    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/
  8. Code Review Challenge Review “Bad Code” to be a “Good

    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/
  9. Code Review Challenge Review “Bad Code” to be a “Good

    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/
  10. Code Review Challenge Review “Bad Code” to be a “Good

    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/
  11. Code Review Challenge Review “Bad Code” to be a “Good

    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/
  12. Code Review Challenge Review “Bad Code” to be a “Good

    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/
  13. Code Review Challenge Review “Bad Code” to be a “Good

    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/
  14. 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
  15. 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 }
  16. 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 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) } }
  17. Tip of the Ice-func An object which have a function

    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() }
  18. Major issues 1. OutputStream is not handled properly 2. Signatures

    that can be used incorrectly 3. Structure of functions with poor readability
  19. 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() }
  20. 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() }
  21. 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() }
  22. 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()) } }
  23. 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()) } }
  24. 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 } }
  25. 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 } }
  26. Major issues 1. Might not be called OutputStream.close() 2. Signatures

    that can be used incorrectly 3. Structure of functions with poor readability
  27. 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 }
  28. 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 } enum class BackupTarget { IMAGE, VIDEO, MESSAGE }
  29. 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 } `since` is not affected for image/video. But interface doesn’t indicate this. enum class BackupTarget { IMAGE, VIDEO, MESSAGE }
  30. Tip of the Ice-func Issue: Signatures that can be used

    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 }
  31. Tip of the Ice-func Issue: Signatures that can be used

    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 }
  32. 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() }
  33. 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() }
  34. Major issues 1. Might not be called OutputStream.close() 2. Signatures

    that can be used incorrectly 3. Structure of functions with poor readability
  35. 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 }
  36. 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 }
  37. 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) } }
  38. 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) } }
  39. 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
  40. 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/
  41. 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 ) } }
  42. A Composable for everything, and everything in its Composable A

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

    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 } }
  44. 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
  45. 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 ) } }
  46. 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 ) } }
  47. A Composable for everything, and everything in its Composable Issue:

    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 }
  48. A Composable for everything, and everything in its Composable Issue:

    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 }
  49. 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 ) }
  50. 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 ) }
  51. 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
  52. A Composable for everything, and everything in its Composable Issue:

    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" ) } }
  53. A Composable for everything, and everything in its Composable Issue:

    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" ) } }
  54. A Composable for everything, and everything in its Composable Issue:

    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" ) } }
  55. A Composable for everything, and everything in its Composable 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, modifier: Modifier = Modifier ) { Button( modifier = modifier.fillMaxWidth(), onClick = onClick ) { Text(text = "Login") } }
  56. A Composable for everything, and everything in its Composable 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, modifier: Modifier = Modifier ) { Button( modifier = modifier.fillMaxWidth(), onClick = onClick ) { Text(text = "Login") } }
  57. 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
  58. A Composable for everything, and everything in its Composable Issue:

    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" ) } }
  59. A Composable for everything, and everything in its Composable Issue:

    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" ) } }
  60. A Composable for everything, and everything in its Composable Solution:

    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") } }
  61. A Composable for everything, and everything in its Composable Solution:

    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") } }
  62. A Composable for everything, and everything in its Composable Solution:

    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 }
  63. 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
  64. 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/
  65. 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 */ } }
  66. Slip Through My Async A repository class which handles shop

    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 */ } }
  67. Slip Through My Async A repository class which handles shop

    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 } }
  68. Slip Through My Async A repository class which handles shop

    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 } }
  69. Major issues 1. Cause the app to crash 2. Hard

    to understand what this function does 3. Race condition occurs
  70. 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 } }
  71. 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 } }
  72. 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() }
  73. 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() }
  74. Major issues 1. Cause the app to crash 2. Hard

    to understand what this function does 3. Race condition occurs
  75. 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 } }
  76. 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 } }
  77. 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 } }
  78. 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() }
  79. 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() }
  80. 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() }
  81. 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
  82. 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/
  83. 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) } }
  84. 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) } }
  85. 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" } }
  86. Major issues 1. Do not use `runCatching` 2. Do not

    overload functions 3. Data class has value that doesn’t have implementation of equals
  87. 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) } }
  88. 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) } }
  89. 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) }
  90. 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
  91. 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) } This is System Error. Don’t catch! Throwable Exception Error RuntimeException IOException NullPointerException CancellationException
  92. 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) } Don’t catch! This is used for coroutine cancellation. Don’t catch! Throwable Exception Error RuntimeException IOException NullPointerException CancellationException
  93. 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) } Don’t catch! Usually, this is caused by bugs. Don’t catch! Don’t catch! Throwable Exception Error RuntimeException IOException NullPointerException CancellationException
  94. 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) } Don’t catch! Don’t catch! Don’t catch! You should catch this! Throwable Exception Error RuntimeException IOException NullPointerException CancellationException
  95. 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) } }
  96. 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 } }
  97. 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 } }
  98. Major issues 1. Do not use `runCatching` 2. Do not

    overload functions 3. Data class has value that doesn’t have implementation of equals
  99. 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 } }
  100. 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 } }
  101. 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? }
  102. 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? }
  103. 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? }
  104. 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? } 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 } });
  105. 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? } 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
  106. 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? }
  107. 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 } } 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? }
  108. 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? }
  109. Major issues 1. Do not use `runCatching` 2. Do not

    overload functions 3. Data class has value that doesn’t have implementation of equals
  110. 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 )
  111. 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 { } }
  112. 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 { } }
  113. Caught on Running Solution: Use ordinal class class UserData( val

    id: UserId, val userName: UserName, val userType: UserType, val profileImage: Bitmap )
  114. 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 )
  115. 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
  116. 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
  117. 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) } }
  118. 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'" } }
  119. 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) }
  120. Major issues 1. Unintentional synchronous execution of async functions 2.

    OS bug regarding Parcelable that occurs only in Android Tiramisu
  121. 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'" } }
  122. 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'" } }
  123. 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'" } }
  124. 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'" } }
  125. 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'" } }
  126. Major issues 1. Unintentional synchronous execution of async functions 2.

    OS bug regarding Parcelable that occurs only in Android Tiramisu
  127. 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) } }
  128. 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) } }
  129. 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) }
  130. 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) }
  131. 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
  132. 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
  133. 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
  134. 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)
  135. 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
  136. 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
  137. 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
  138. 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
  139. 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
  140. 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
  141. 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
  142. 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
  143. 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
  144. 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
  145. 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
  146. 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
  147. 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” class Ugly1 { fun dirtyFun1() { /* */ } } Questions
  148. 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() { /* */ } }
  149. 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 Ugly1 { fun dirtyFun1() { /* */ } } class Ugly2 { fun dirtyFun2() { /* */ } } class Ugly3 { fun dirtyFun3() { /* */ } }
  150. 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 Ugly1 { fun dirtyFun1() { /* */ } } class Ugly2 { fun dirtyFun2() { /* */ } } class Ugly3 { fun dirtyFun3() { /* */ } } class Ugly4 { fun dirtyFun4() { /* */ } }
  151. 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 Ugly1 { fun dirtyFun1() { /* */ } } class Ugly2 { fun dirtyFun2() { /* */ } } class Ugly3 { fun dirtyFun3() { /* */ } } class Ugly4 { fun dirtyFun4() { /* */ } } class Ugly5 { fun dirtyFun5() { /* */ } }
  152. How to make the question? Apply the weekly report in

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