【DroidKaigi 2023】Code Review Challenge 4問目解説

Yuki Ando2023-09-27

iOS/Androidエクスペリエンス開発チーム

こんにちは。コミュニケーションアプリLINEのクライアントを開発している安藤です。

この記事では、DroidKaigi2023の企業ブースで行ったCode Review Challengeの4問目の解説をします。 Code Review Challengeについてはこちらを参照してください。

出題タイトル: Caught on Running

4問目では、以下のコードが出題されました。

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

interface UserDataApi {
    suspend fun findUser(userId: UserId): UserData

    suspend fun findUser(userName: UserName): UserData
}

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

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

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 Logger {
    fun sendLog() {
        TODO("Send log to remote server")
    }
}

これはユーザの情報を表示する画面のためのActivityと関連するRepositoryクラスです。 UserDataRepositoryはUserDataApi(とその実装UserDataApiImpl)を用いてリモートサーバからユーザの情報を取得します。
SomeActivityはUserDataRepositoryを用いてユーザの情報を表示します。この際、成功時には取得したユーザの情報を、失敗時にはToastを表示します。取得するユーザの情報はUserDataとして定義されています。

問題点と解決策

このコードには複数の問題が含まれていますが、問題の影響が比較的大きい以下の3つを重点的に解説します。

runCatchingを使用している問題
関数のオーバーロードをしている問題
data classのequalsの実装の問題

1. runCatchingを使用している問題

UserDataRepositoryが持つ2つの関数ではUserDataApiのsuspend関数を呼び出すためにrunCathingを使用しています。

    suspend fun getUserData(userId: UserId): Result<UserData> =
        runCatching { userDataApi.findUser(userId) }

    suspend fun getUserData(userName: UserName): Result<UserData> =
        runCatching { userDataApi.findUser(userName) }

リモートサーバから情報を取得するUserDataApiはネットワークの問題を始めとするIOExceptionを発生させる可能性があります。これらのハンドリングを行うためにrunCatchingを使用して、呼び出し側にて成功時と失敗時の処理を記述していますが、runCatchingを使用してエラーハンドリングを行っているのが問題です。
runCathingを使用した場合のコードは次のコードと等価です。

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

    suspend fun getUserData(userName: UserName): Result<UserData> = try {
        Result.success(userDataApi.findUser(userName))
    } catch (e: Throwable) {
        Result.failure(e)
    }

UserDataApiが発生させる例外は限られたものになるはずです。しかし、runCathingを使用することでその他の例外も全てキャッチしてしまいます。今回の場合ではネットワーク関連のトラブルを示すIOExceptionのみをハンドリングすれば十分です。

    suspend fun getUserData(userId: UserId): UserData? = try {
        // ...snip
    } catch (e: IOException) {
        null
    }

    suspend fun getUserData(userName: UserName): UserData? = try {
        // ...snip
    } catch (e: IOException) {
        null
    }

不必要に広い範囲の例外をキャッチすることは、バグの早期発見の妨げになります。また、リカバリー不可能なエラーをキャッチしてしまうことで更に深刻な問題へと発展してしまう可能性もあります。
例えば、 NullPointerException は実装の問題から生じるものです。そのためコードを修正する必要がありますが、 runCatching を使っていると発見が難しくなってしまいます。
また、CancellationExceptionはCoroutineが正しく動作するために必要な例外です。こちらをキャッチしてしまうと正常にキャンセルが動作しません。そのため必要最小限の例外をキャッチするよう心がけるべきでしょう。

2. 関数のオーバーロードをしている問題

UserDataRepositoryが持つ2つの関数はどちらも同じgetUserDataという名前を持っています。引数として各関数はvalue classのUserId,UserNameを取ります。

    suspend fun getUserData(userId: UserId): Result<UserData> =
        runCatching { userDataApi.findUser(userId) }

    suspend fun getUserData(userName: UserName): Result<UserData> =
        runCatching { userDataApi.findUser(userName) }

一見すると大きな問題を持っていないように見えますが、このUserDataRepositoryがJavaから参照されるケースで非常に大きな問題となります。これはKotlinにおけるvalue classのJava上での振る舞いによって発生する問題です。

value classは値を1つだけ持ったクラスで、値に制限をかける、意味のある名前をつけ型安全に取り扱いたい際に使用します。例えば今回のUserIdとUserNameはどちらもStringの値を持ちますが、それぞれ別の型として扱われます。そのため、UserIdの変数にUserNameを代入するといったStringではできてしまうことを制限することができます。

しかし、このvalue classをJavaの上で扱おうとした場合、内部に持っている1つの値の型(今回の場合ではどちらもString)として扱われます。 Java上からUserDataRepositoryを使用する場合、次のように認識されてしまいます。

public @Nullable UserData getUserData(String userId);

public @Nullable UserData getUserData(String userName);

その結果同一のシグネチャをもつ関数が2つあると認識されてしまい、呼び出そうとするJavaのコードにてコンパイルエラーが発生します。

これを解決するための一番簡単な方法は関数のオーバーロードをやめ、それぞれに別の名前をつけることです。

    suspend fun getUserDataById(userId: UserId): UserData? = try { /* snip */ }

    suspend fun getUserDataByName(userName: UserName): UserData? = try { /* snip */ }

@JvmNameアノテーションを用いて名前を宣言することで回避することもできますが、特に理由がなければ関数のオーバーロードをしない前述の方法を用いたほうがよいでしょう。関数を作成する場合は、その関数が何をする関数なのかを的確に表す関数名にすべきです。もし、似たような関数が複数ある場合は引数に関する言及を関数名に含めることも検討しましょう。

3. data classにequalsの実装がない値を含めている

ApiやUserDataRepositoryで使用しているUserDataはdata classとして次のように定義されています。

    data class UserData(
        val id: UserId,
        val userName: UserName,
        val userType: UserType,
        val profileImage: Bitmap
    )

ユーザの情報を表示するための情報をまとめているUserDataクラスですが、data classであることに起因して問題があります。

Kotlinのdata classでは、toString(),hashCode(),equals(),copy(), componentN()といった関数が自動で実装されます。

Bitmap のequalsは同一であるかの確認をし、同値であるかの確認にはsameAsを使います。一方、data classのequalsではすべてのプロパティのequalsを確認するため、プロパティの詳細を見ない限りは同値性を確認していることを期待しやすいです。もし、data classの一部として Bitmapが含まれていた場合は、誤解を招く可能性があります。例えば、UserDataの変更をdistinctUntilChangedを用いて検知する場合を考えます。
distinctUntilChangedの内部ではequalsを用いて変更を検知しているため、UserDataに対して使用するとBitmapの同一性を確認してしまいます。結果としてBitmapは同一であるか、それ以外の値は同値であるかを確認するような挙動となってしまい、非常に理解するのが難しい状況になります。

この問題の解決方法ですが、一番簡単なのはdata classではなく通常のclassを使用することです。

    class UserData(
        val id: UserId,
        val userName: UserName,
        val userType: UserType,
        val profileImage: Bitmap
    )

通常のclassとして定義することで誤解を招きづらい実装とすることができます。

data class を使う必要がある場合は、equalsの実装をオーバーライドすることも可能です。 bitmapでは前述したようにsameAs関数を使用することで同値であるかの確認を行うことができます。こちらを用いてequalsを正しくオーバーライドすることで想定通りに動くコードを作成することができます。 equalsをオーバーライドする場合にはhashCodeについても正しくオーバーライドする必要があるので注意してください。

data classを使用する他の方法としてBitmapではなく画像を示すUrlやリソースIDなどを変わりに持たせることでもdata classを正しく使うことができます。

    data class UserData(
        val id: UserId,
        val userName: UserName,
        val userType: UserType,
        val profileImageUrl: String
    )

data classは非常に便利に使えるケースが多いため気軽に使用するケースが多いと思いますが、使用すべきでないケースで使用してしまうことで思わぬ実装ミスに繋がってしまう場合もあるため注意が必要です。

その他の問題

この他にも様々な問題が含まれていました。

命名について Kotlinのconventionに則っていない変数、関数の命名や分かりづらい、間違った名前がついた変数などが多数存在していました。それぞれの変数・関数に対して適切に名前を付ける必要があります。

a. 関数名がUpdateUserDataになっている - updateUserDataとする b. sealed classの子クラスの名前がenumの命名規則に従っている - ADMIN, NORMALではなく、Admin, Normalとする c. Intentを取得するためのkeyの値がuser_nameなのに対し、変数名がUSER_IDとなっている - keyの値と変数名を揃える d. ユーザの情報を表示するActivityの名前が不適切 - SomeActivityではなく、UserDataActivityなど、実態に即した名前にする

不必要なsealed classの定義ユーザの属性を示すUserTypeをsealed classとして定義しています。しかし、UserTypeの小クラスを見ると、全てobjectとして定義されています。今回の場合enum classを用いた表現で十分です。
value classの定義にアノテーションが付与されていない現在value classを定義する場合には@JvmInlineのアノテーションを付与することが必須ですが、問題のコードには含まれていません。こちらのアノテーションを追記する必要があります。
Intentから取得した値の取り扱いによってバグが発生する IntentからUSER_IDを用いて値を取得することを試みますが、失敗した場合にfailedという値を代入しています。また、updateUserData関数にて値がfalled以外の場合にのみUserDataRepositoryでの処理を行うという実装をしています。今回の実装ではuserIdがfalledの場合にはUserDataRepositoryの処理が行われず、userIdを正常に取得できない場合にはfailedというuserIdがあるとして実行されてしまいます。基本的にエラーの値は明確にエラーであることがわかるように実装すべきでしょう。今回の場合はnullをエラーの値として取り扱うことでバグを埋め込む可能性を極力排除することができます。

さらに今回はStringからvalue classへの変換も行う必要がありました。変数名と実際の値の食い違いによってvalue classへの変換も間違った形で実装してしまっています。 value classは内部の値が同じ型である場合には簡単に変換ミスを起こすことができてしまうのでこちらも注意する必要があります。

companion object {
    const val INTENT_KEY_USER_ID = "user_id"
    
    private fun getUserIdOrNull(intent: Intent): UserId? {
        val userId = intent.getStringExtra(INTENT_KEY_USER_ID) ?: return null
        return UserId(userId)
    }
}

定義したUserIdがUserDataで使われていないせっかく定義したUserIdがUserDataの内部では使用されていませんでした。定義するのであれば正しく使用すべきでしょう。
関数の公開範囲は最小化すべき UpdateUserDataは現在SomeActivityのonCreateでのみ呼び出されています。そのため、private化すべきでしょう。
UserDataのフォーマットが正しくない UserDataクラスの定義のうち、UserTypeと次のvalの間にスペースが存在しません。これはconventionのルールに反しています。 linterを使用するなど、フォーマットルールに従うべきでしょう。
Dispatchers.Mainが必要ない updateUserData関数においてUserDataRepositoryの関数を呼び出すためにlifecycleScopeからlaunchを行っていますが、Dispatchers.Mainを使用しています。しかし、こちらの宣言は必要ありません。
Publicなsuspend関数がメインスレッドセーフでない問題行数の都合上、UserDataApiがメインスレッドセーフであるかどうかを実装から判断することができるように記述できていません。こちらでも紹介されている通り、publicなsuspend関数は基本的にメインスレッドセーフであるべきでしょう。
その場合、UserDataApiImplの内部にてIoDispatcherを使用するなどの考慮を行うのが望ましいです。

class UserDataApiImpl(private val ioDispatcher: CoroutineDispatcher) : UserDataApi {
    //...snip
    override suspend fun findUser(userId: UserId): UserData? = withContext(ioDispatcher) {
        TODO("Call remote api")
    }    
}

今回紹介した問題・改善点を解決した想定解答がこちらになります。

class UserDataActivity : AppCompatActivity() {
    private val logger: Logger = Logger()
    
    private val userDataRepository = UserDataRepository()

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContentView(R.layout.activity_user_data)
        val userId = getUserIdOrNull(intent)
        updateUserData(userId)
    }

    private fun updateUserData(userId: UserId?) {
        val textView = findViewById<TextView>(R.id.text)
        if (userId == null) {
            return
        }
        lifecycleScope.launch {
            val userData = userDataRepository.getUserDataById(userId)
            if (userData == null) {
                Toast.makeText(
                    this@GoodSomeActivity,
                    "Something wrong with it.",
                    Toast.LENGTH_SHORT
                ).show()
                logger.sendNetworkErrorLog()
                return@launch
            }
            textView.text = userData.userName.value
        }
    }

    companion object {
        const val INTENT_KEY_USER_ID = "user_id"
        
        private fun getUserIdOrNull(intent: Intent): UserId? {
            val userId = intent.getStringExtra(INTENT_KEY_USER_ID) ?: return null
            return UserId(userId)
        }
    }
}

interface UserDataApi {
    suspend fun findUserById(userId: UserId): UserData
    
    suspend fun findUserByName(userName: UserName): UserData
}

class UserDataRepository(
    private val userDataApi: UserDataApi = UserDataApiImpl(),
    private val ioDispatcher: CoroutineDispatcher = Dispatchers.IO
) {
    suspend fun getUserDataById(userId: UserId): UserData? = try {
        userDataApi.findUserById(userId)
    } catch (e: IOException) {
        null
    }
    
    suspend fun getUserDataByName(userName: UserName): UserData? = try {
        userDataApi.findUserByName(userName)
    } catch (e: IOException) {
        null
    }
}

data class UserData(
    val id: UserId, // or String userId
    val userName: UserName, // or String userName
    val userType: UserType,
    val profileImage: Bitmap
)

@JvmInline
value class UserId(val value: String)

@JvmInline
value class UserName(val value: String)

enum class UserType { ADMIN, NORMAL }

class UserDataApiImpl(private val ioDispatcher: CoroutineDispatcher) : UserDataApi {
    override suspend fun findUserById(userId: UserId): UserData = withContext(ioDispatcher) {
        TODO("Call remote api") 
    }
    
    override suspend fun findUserByName(userName: UserName): UserData = withContext(ioDispatcher) { 
        TODO("Call remote api")
    }
}

class Logger {
    fun sendNetworkErrorLog() {
        TODO("Send log to remote server")
    }
}

まとめ

この記事では、Code Review Challengeの 4 問目の解説として、以下の改善が必要なことを説明しました。

runCatchingを使用せず、適切なスコープで例外を処理する
可能な限り関数のオーバーロードをせず、適切な関数名を設定する
data classを使用する際にはequalsなどの関数がどのように動作するか考慮した上でパラメータを決定する

言語の標準機能・標準ライブラリとして用意されているものであったとしても使い方によって不安定なコードになってしまう場合があります。機能の理解はもちろんのこと、作成すべき機能はどのようなものかをしっかり考えることが重要です。

Blog