Code Review Challenge 1問目解説

Kazuki Chigita 2022-12-09

LINEコミュニケーションサービス開発チーム

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

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

出題タイトル : Everything is something

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

sealed class ContactType {
    object Friend : ContactType()
    object Bot : ContactType()
}
  
class friend_item_Presenter(
    private val friendNameProvider: FriendNameProvider,
    private val coroutineScope: CoroutineScope
) {
    fun updateViews(itemId: String) {
        coroutineScope.launch {
            nameTextView.text = friendNameProvider.getName(itemId, ContactType.Friend)
            // update other view component
        }
    }
}
  
class BotItemPresenter(
    private val nameProvider: NameProvider,
    private val coroutineScope: CoroutineScope
) {
    fun updateViews(itemId: String) {
        coroutineScope.launch {
            val name = nameProvider.getName(itemId, ContactType.Bot) ?: return@launch
            val displayName = name.ifBlank {
                "No name Bot"
            }
            nameTextView.text = displayName
            // update other view component
        }
    }
}
  
interface NameProvider {
    suspend fun getName(uuid: String, contactType: ContactType): String?
}
  
class FriendNameProvider(
    val friendRepository: FriendRepository
) : NameProvider {
    override suspend fun getName(uuid: String, contactType: ContactType): String {
        if (contactType == ContactType.Friend) {
            val name = friendRepository.getSuperCoolName(uuid)
            return name
        }
        return ""
    }
}
  
class BotNameProvider(
    private val botRepository: BotRepository
) : NameProvider {
    override suspend fun getName(uuid: String, contactType: ContactType): String? {
        if (contactType == ContactType.Friend) {
            return null
        }
        val name = botRepository.getSuperCoolBotName(uuid)
        return name
    }
}

こちらのコードは、連絡先 (Contact) の名前表示を行っている View を更新するクラス群です。 Contact として Friend / Bot の 2 種類が用意されており、これらは `sealed class ContactType` として定義されています。それぞれの Contact に対応して、Presenter class が用意されており、クラスに定義されている `updateViews` 関数から View の更新を行います。

View を更新するために必要なデータは、 NameProvider を介して取得されます。 NameProvider の実装は ContactType 毎に異なっており、それぞれ `FriendNameProvider`、`BotNameProvider`と命名され実装が行われています。

このコードには主に可読性・頑健性の観点で大きく以下の 3 つの問題があります。

1. 暗黙の前提条件に基づいたフィルタ処理

2. 過度な抽象化による実装エラー

3. エラー処理を必要とする引数

それぞれの問題について解説をします。

1. 暗黙の前提条件に基づいたフィルタ処理

`BotNameProvider` の実装を確認してみましょう。引数として渡される `ContactType` を確認し、`Friend` の場合には、`null`を返却。そうでない場合には `BotRepository` から name を取得し、返却するというロジックが記述されています。このコードは `ContactType` が `Friend` / `Bot` しか存在し得ないという暗黙の前提条件に基づいてロジックが記述されています。

override suspend fun getName(uuid: String, contactType: ContactType): String? {
    if (contactType == ContactType.Friend) {
        return null
    }
    val name = botRepository.getSuperCoolBotName(uuid) // This line requests only ContactType.BOT as hidden condition.
    return name
}

例えば、`ContactType` に新たに、`Anonymized` (匿名)というタイプが追加されたとしましょう。

sealed class ContactType {
    object Friend : ContactType()
    object Bot : ContactType()
    object Anonymized : ContactType() // <- Added new type.
}

本来 `BotNameProvider.getName` では ContactType として `ContactType.BOT` 以外を指定した際には `null` を返却して欲しいのですが、`Anonymized` の際に意図しない行を通過してしまいます。

override suspend fun getName(uuid: String, contactType: ContactType): String? {
    if (contactType == ContactType.Friend) {
        return null
    }
    val name = botRepository.getSuperCoolBotName(uuid) // ContactType.Anonymized reaches here ...
    return name
}

この問題は、`BotNameProvider` と Bot に関しての処理を集めるはずが、`Friend` という本来このクラスの責務の範囲外の ContactType を用いてフィルタの条件を組み立てようとした点に問題があります。解決策としては次の2つのアプローチがあります。

A. `ContactType.Bot` を中心に条件式を組み立てる方法
B. `when` を用いる方法

A. `ContactType.Bot` を中心に条件式を組み立てる方法

今回の例では、着目したい `ContactType.Bot` 以外を early return することで、意図しない `ContactType`でロジックが実行されることを防ぐことができます。

override suspend fun getName(uuid: String, contactType: ContactType): String? {
    if (contactType != ContactType.Bot) {
        return null
    }
    val name = botRepository.getSuperCoolBotName(uuid)
    return name
}

B. `when` を用いる方法

`when` を用いることで、ContactType が増えた際には、コンパイルエラーとなります。そのため、実行時ではなくコンパイル時にロジック修正の必要性に気がつくことができます。

override suspend fun getName(uuid: String, contactType: ContactType): String? = when (contactType) {
    ContactType.Bot -> botRepository.getSuperCoolBotName(uuid)
    ContactType.Anonymized,
    ContactType.Friend -> null
}

コードレビューの際には、暗黙の前提条件に基づいて条件式が記述されていないか？の観点でレビューを行うことを意識してみてはいかがでしょうか？

2. 過度な抽象化による実装エラー

続いて、`BotItemPresenter` のコンストラクタを確認してみましょう。

class BotItemPresenter(
    private val nameProvider: NameProvider,
    private val coroutineScope: CoroutineScope
) { /* snip... */ }

`nameProvider` の型として `NameProvider` が指定されています。つまり `nameProvider` には `NameProvider` interface を実装しているクラスやオブジェクトであればどんなものでも引数として渡すことができます。
そのため、例えば `nameProvider` として `FriendNameProvider` のインスタンスを渡すことができてしまいます。
これは実装者が意図した挙動ではないでしょう。
解決方法は、`NameProvider` を `BotNameProvider` とすることです。

class BotItemPresenter(
    private val nameProvider: BotNameProvider,
    private val coroutineScope: CoroutineScope
) { /* snip... */ }

さて、この問題の根本原因はどこにあるのでしょうか？`friend_item_Presenter` は `FriendNameProvider` と、`BotItemPresenter` は `BotNameProvider`とそれぞれ依存関係があり、Friend と Bot は独立していると言えます。
ロジック上では依存関係のなかった両者が、`NameProvider` という interface を持つことで、不必要に関係を複雑にしています。
コードを見ても、`NameProvider` interface を定義している理由は、単に共通のメソッド (`getMethod`) があるからに過ぎません。
現在の依存関係を図で表すと以下の図1のようになります。

図1. 既存のコードの依存関係

そこで、`NameProvider` を削除しましょう。すると、以下の図2ような依存関係となります。

図2. NameProvider を削除した後の依存関係

実装者が意図していた依存関係を表現できていることが依存関係の図からもわかります。

過度な抽象化は、本来意図したより依存関係を複雑にし、間違った実装を誘発します。一方で、`NameProvider` interface を用意することで依存の逆転を行い、`friend_item_Presenter`と `FriendNameProvider` の実装の依存を切り分けていると考えることもできます。
コードレビューの際には「なぜ抽象化のレイヤが必要なのか？」の観点でレビューを行うことで、より深い議論へと発展させることができるでしょう。

3. エラー処理を必要とする引数

2. 過度な抽象化による実装エラーにおいても一部触れましたが、`Friend` と `Bot` はどちらも ContactType に関連していますが、presenter のレイヤでは独立しています。
そのため、`FriendNameProvider` には `ContactType.Friend` 以外が引数として渡されても異常系として処理されます。`BotNameProvider` も同様です。
Presenter と NameProvider は依存関係で既に制約がある状態と言えるでしょう。
そのため、`ContactType`を改めて引数で渡し、その関係をラベル的に明示する必要はありません。
`FriendNameProvider.getName` および `BotNameProvider.getName` の引数から `ContactType` を削除します。

class FriendNameProvider(private val friendRepository: FriendRepository) {
    suspend fun getName(uuid: String): String = friendRepository.getSuperCoolName(uuid)
}

class BotNameProvider(private val botRepository: BotRepository) {
    suspend fun getName(uuid: String): String = botRepository.getSuperCoolBotName(uuid)
}

`ContactType` を削除することで、複雑になっていた NameProvider の実装がよりシンプルになりました。
またこの変更を行うことで、`BotNameProvider` の `getName` の戻り値が `String?` からより厳密な `String` となっています。

その他の問題

以上の 3 点についてをメインのトピックとしていましたが、他には以下のような問題点がありました。

`friend_item_Presenter` の命名は一般的な Kotlin のクラスの命名のルールから外れている。
`FriendNameProvider`のプロパティ `friendRepository` は本来意図しているよりも広いスコープから参照可能になっている。
`ContentType` を継承しているのは `object` だけのため、`sealed class` を使うのは過剰で、`enum class` で十分。
`uuid` について扱っている変数の型を `String`ではなく `java.util.UUID` 等にする。

まとめ

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

暗黙の前提条件が埋め込まれていないか注意する。
enum / sealed class等のタイプが増えた時の動作に着目する。
過度な抽象化が起きていないか確認する。
意図しないインスタンスが注入されないようにより厳密な型を選択する。

特に暗黙の前提条件は、実装を行った時点ではバグではないためスルーされがちですが、後々の機能改修やリファクタリングの際に時限爆弾のようにバグが発生します。時限爆弾を防ぐためにも「暗黙の前提条件がないか？」の視点を持ってコードレビューに臨んでみてください！

第2問目は池永（https://engineering.linecorp.com/ja/blog/code_review_challenge_2）による解説です。

Blog

Code Review Challenge 1問目解説

出題タイトル : Everything is something

1. 暗黙の前提条件に基づいたフィルタ処理

A. `ContactType.Bot` を中心に条件式を組み立てる方法

B. `when` を用いる方法

2. 過度な抽象化による実装エラー

3. エラー処理を必要とする引数

その他の問題

まとめ