どちらのコードが好ましい？6−8問目（Kotlin Fest 2024 Pocket Code Battle）

こんにちは。AndroidアプリエンジニアのMoriとIshikawaです。

先日開催されたKotlin Fest 2024のLINEヤフー企業ブースでは、「Pocket Code Battle」を実施しました。「Pocket Code Battle」とは、同じ動作をする複数の異なる実装方法のコードを比較し、どちらが好ましいかを考えてもらう企画です。

ブースに来ていただいた方には意見を書いた付箋を貼ってもらい、その意見を元にLINEヤフーの開発者と交流しました。

本記事では、今回出題した「Pocket Code Battle」の6問目-8問目の解説をします。

#6 A structure is worth a thousand variables?

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

コード#6-A

sealed interface NewsListScreenState {
    data object Initialized: NewsListScreenState

    data object Loading: NewsListScreenState

    data class Loaded(
        val newsList: List<NewsContent>,
        val pagingProgress: PagingProgress
    ): NewsListScreenState

    data class PullRefreshing(val newsList: List<NewsContent>): NewsListScreenState

    data class Error(val error: HttpException)

    data class ErrorWithContent(
        val error: HttpException,
        val newsList: List<NewsContent>,
        val pagingProgress: PagingProgress 
    )

    sealed interface PagingProgress {
        data class HasMore(val nextToken: String): PagingProgress
        data object Loading: PagingProgress
        data object ReachedEndPage: PagingProgress
        data class Error(val error: HttpException)
    }
}

コード#6-B

data class NewsListScreenState(
    val newsList: List<NewsContent>,
    val loadingStatus: LoadingStatus,
    val error: HttpException?,
    val nextToken: String?
) {
    val hasMore: Boolean = nextToken != null

    enum class LoadingStatus {
        IDLE,
        INITIAL_LOADING,
        PAGING_LOADING,
        PULL_REFRESHING
    }
}

ここでは、ニュースのリストを表示する画面を表現するためのモデルを作ろうとしています。

この画面は、以下のような挙動を持つことを想定しています。

• 初期ローディング中は真ん中にローディングアイコンを表示する
• 下スクロールで追加ページを読み込む
  • 次のコンテンツがある場合は一番下にローディングアイコンを表示する
• 引っ張って更新（pull to refresh）ができる
  • 更新中は専用のローディングインディケータが出る
  • 更新中も以前のリストを維持する
• ネットワークエラー発生時にエラーを表示する
  • 初期ロード、ページング、引っ張って更新時のいずれのタイミングでもエラーが起こり得る
  • すでに表示できるコンテンツがある場合はそれを表示し続けたい
• 引っ張って更新とページングは同時に行えない
  • 引っ張って更新が優先される

コメント

ブースでは #6-A のほうが良いという意見と、#6-B のほうが良い意見の両方がありましたが、わずかに #6-A のほうが人気があるようでした。

以下はそのコメントの一部です。

#6-A が良い

• 複雑だがPaging機能つきのリスト画面の状態を正しく表現できる
• 状態に応じてどのプロパティが必要かわかるのでsealed classのほうが好み
• 制約が明確
• 多数の人が使うのであれば不正な状態のない1つ目が良さそう
• UI側で間違いを起こしにくいモデリング
• whenで条件を網羅できる

#6-B が良い

• シンプルで短いコードで良い
• 表示することを考えると2つ目で十分
• 1つ目はリファクタが大変そう

解説

このクラスを使う側の要求によって、どのようなモデリングが適切かは異なります。

#6-A のsealed classを使うことによって、各状態を厳密に定義できます。例えば、初期ロード中なのに nextToken が存在する、といった不整合な状態を、#6-A のコードでは完全に排除できます。ページングの処理といった、状態に応じた処理を行うのであれば、このような厳密な管理が役に立つでしょう。

一方で、利用側に複雑なロジックを持たない場合、例えば各UI要素の表示だけを行うのであれば、#6-B のパターンでも十分かもしれません。以下はJetpack ComposeでUIを記述してみた例ですが、そこまで複雑な状態管理はいらないことがわかると思います。（以下のコードではNewsListScreenStateからnextToken等、いらなかったパラメータは削除しています）

data class NewsListScreenState(
    val newsList: List<NewsContent>,
    val loadingIndicator: LoadingIndicator,
    val errorMessage: String?,
    val hasMore: Boolean
) {
    enum class LoadingIndicator {
        IDLE,
        INITIAL_LOADING,
        PULL_REFRESHING
    }
}

@OptIn(ExperimentalMaterial3Api::class)
@Composable
private fun NewsListScreen(
    state: NewsListScreenState,
    onPullToRefresh: () -> Unit,
    onLoadMore: () -> Unit,
    modifier: Modifier = Modifier
) {
    val context = LocalContext.current
    LaunchedEffect(state.errorMessage) {
        if (state.errorMessage != null) {
            Toast.makeText(context, state.errorMessage, Toast.LENGTH_SHORT).show()
        }
    }

    PullToRefreshBox(
        isRefreshing = state.loadingIndicator == NewsListScreenState.LoadingIndicator.PULL_REFRESHING,
        onRefresh = onPullToRefresh,
        modifier = modifier
    ) {
        LazyColumn {
            items(state.newsList) { news ->
                NewsItem(news)
            }
            if (state.hasMore) {
                item {
                    LaunchedEffect(Unit) { onLoadMore() }
                    LoadMoreIndicator()
                }
            }
        }
        if (state.loadingIndicator == NewsListScreenState.LoadingIndicator.INITIAL_LOADING) {
            LoadingIndicator()
        }
    }
}

このように、何かのモデリングを行うときは、それを使う側でどのような情報が必要なのか、どれくらいの厳密さが必要なのかをもとに、その状況にあった構造を検討すると良いでしょう。

#7 Peeling back the elements of the model

7 問目では、ファイルダウンロードなどの長時間かかるタスクがあることを前提とし、それを実行する関数 doHeavyTask() が定義されているとしています。そして、タスクの実行状態を戻り値 Flow<ExecutionState<...>> で表現します。

suspend fun doHeavyTask() : Flow<ExecutionState<Value>> { ... }

タスクの実行状態として「実行中」、「完了」、「失敗」の 3 つがあるとした場合、それをどのように表現する方法にはいくつかの選択肢があります。

1 つ目の選択肢は、sealed class に直接 3 つの状態を並べて表現することです。

コード#7-A

sealed interface ExecutionState<out T> {
    class Ongoing(val progressRatio: Float) : ExecutionState<Nothing>
 
    class Finished<T>(val result: T) : ExecutionState<T>
 
    class Failed(val errorType: ErrorType) : ExecutionState<Nothing>
}

2 つ目の選択肢は、まず「実行中」と「終了」で大きく分け、「終了」の中に「成功」と「失敗」を定義する方法です。

コード#7-B

sealed interface ExecutionState<out T> {
    class Ongoing(val progressRatio: Float) : ExecutionState<Nothing>
 
    sealed interface Finished<out T> : ExecutionState<T> {
        class Success<T>(val result: T) : Finished<T>
 
        class Error(errorType: ErrorType) : Finished<Nothing>
    }
}

コメント

このどちらが良いかについては、おおよそ半々に意見が分かれました。

#7-A が良い

• シンプルでわかりやすい
• when が書きやすい
• ネストがないほうが好ましい
• 今後の変更に対応しやすい

#7-B が良い

• 実行中と完了時で処理を分けやすい（is Finished で分岐しやすい、「終わった」ことがわかりやすい）
• 状態が増えたときに気づきやすい
• Kotlinっぽい

特徴的だったのは、同じ「変更を加えやすい」というコメントが両方にあった点です。編集の簡単さと間違いの気づきやすさのどちらを重視するかで意見が分かれたようです。

解説

こちらも 6 問目と同様、このコードを使う側の仕様によって、適切な形は異なります。

#7-A が有効なケースは、すべての状態で共通のロジックを使う場合が挙げられます。例えば、以下のように状態に応じてテキストを表示するといったものが考えられます。

val messageText = when(state) {
    is ExecutionState.Ongoing ->
        "ongoing... (${state.progressRatio * 100}%)"
    is ExecutionState.Finished ->
        "Finished. Congrats!"
    is ExecutionState.Failed ->
        "Oops. An error ${errorType.displayName} happened"
}
showMessage(messageText)

一方で、実行中とそれ以外で大きくロジックが異なる場合は、#7-B のほうが良いでしょう。実行中はローディングモーダルを表示し、成功・完了時にメッセージを表示する場合が当てはまります。

when(state) {
    is ExecutionState.Ongoing -> {
        progressModal.showWithRatio(state.progressRatio)
    }
    is ExecutionState.Finished -> {
        progressModal.hide()
        showMessage(state.toMessageText())
   }
}

fun ExecutionState.Finished.toMessageText(): String = when(this) {
    ...
}

この他にも、「失敗」とそれ以外で大きくロジックを分けたいという状況もあります。実行中や完了時は状態を表示し続ける一方で、失敗時にダイアログを表示し、その後画面を閉じるというケースが考えられます。その場合は、Ongoing と (Successfully)Finished を 1 つの interface として括るべきでしょう。

このように、データモデルの構造として何が適切かは、とり得る状態（変域）だけでは決まりません。設計一般に言えることですが、そのデータやコードがどのように使われるかによって、適切な形は変わり得ます。設計の議論をするときは、まず要求を明確にすることが重要です。

#8 Look at the type before you jump

8 問目では、ある API を呼び出した結果として、以下のような FooApiResult が返される状況を想定しています。

sealed interface FooApiResult {
    class Success(val value: FooModel) : FooApiResult
    class Error(val errorType: ErrorType, val message: String) : FooApiResult
}

この FooApiResult を処理するコードを書く場合に、1 つ目のように early return を使うほうが良いのか、2 つ目のように when を使うのほうが良いかを出題しました。

コード#8-A

fun syncRemoteFooData(fooId: Int) {
    val result = fooApiClient.callFooApi(fooId)
    if (result is FooApiResult.Error) {
        aggregateApiError(result.errorType, result.message)
        return
    }
 
    val fooModel = (result as FooApiResult.Success).value
    repository.saveFooModel(fooModel)
    useCase.notifyFooSynced(fooModel)
    debugLogger.log(LogLevel.INFO, "Foo model ${fooModel.id} has been synced")
}

コード#8-B

fun syncRemoteFooData(fooId: Int) =
    when (val result = fooApiClient.callFooApi(fooId)) {
        is FooApiResult.Success -> {
            val fooModel = result.value
            repository.saveFooModel(fooModel)
            useCase.notifyFooSynced(fooModel)
            debugLogger.log(LogLevel.INFO, "Foo model ${fooModel.id} has been synced")
        }
 
        is FooApiResult.Error ->
            aggregateApiError(result.errorType, result.message)
    }

コメント

この問題については、#8-A と #8-B でおおよそ 1 : 4 の割合になりました。

#8-A が良い

• 直感的
• 一目で構造がわかる

#8-B が良い

• 網羅されていることが保証できる
• as のキャストがいらない
• 処理でインデントがそろっているほうがわかりやすい
• Kotlin っぽい

解説

#8-A と #8-B、それぞれに良い点があります。

• #8-A のメリット: early return の部分を読み飛ばすことで、重要なコード（Success のケース）がどこにあるかがわかりやすくなり、関数の概要の把握がしやすい。
• #8-B のメリット: FooApiResult が変更されたとしても、コンパイル時に FooApiResult が網羅されていることが保証できる。

「網羅性を保証する」ということは sealed interface/class や enum を使う上で大きなメリットです。逆に言えば、これを活かせないのであるならば、sealed や enum を使う必要はほとんどないでしょう。したがって、#8-B の選択を行うのがより好ましいと考えられます。

しかしながら、Success のケースのロジックが大きくなった場合、#8-B の構造は読みにくくなりやすいです。その場合は #8-B の選択に加え、分岐のボディを private 関数として抽出することが選択肢に入ります。

fun syncRemoteFooData(fooId: Int) = when (val result = fooApiClient.callFooApi(fooId)) {
    is FooApiResult.Success -> saveAndNotifySyncedFoo(result.value)
    is FooApiResult.Error -> aggregateApiError(result.errorType, result.message)
}

private fun saveAndNotifySyncedFoo(fooModel: FooModel) {
    repository.saveFooModel(fooModel)
    useCase.notifyFooSynced(fooModel)
    debugLogger.log(LogLevel.INFO, "Foo model ${fooModel.id} has been synced")
}

こうすることで、#8-A と #8-B のメリットのいいとこ取りを達成できます。

なお、TypeScript の type guard では、if (result is Error)  return に相当するコードを書き、残った可能性が Success だけになる場合は、result を Success として取り扱うことができます。この状況ならば early return でも網羅性を保証しやすくなるため、#8-A の書き方も十分に選択肢に入ります。

type FooApiResult =
  | {
      kind: 'success';
      value: FooModel;
    }
  | {
      kind: 'error';
      error: ErrorType;
    };

const handleFooApiResult = (result: FooApiResult) => {
  if (result.kind === 'error') {
    return;
  }

  result.value; // kind === 'success' として取り扱える
};

Kotlin 2.0 では smart cast が強化されたものの、まだこのケースは取り扱えません。今後の機能強化にも期待したいですね。

おわりに

「Pocket Code Battle」の6問目-8問目について解説しました。

部分的なコードだけではどちらのコードが良いと断定できないケースも多くあります。この記事も参考にしつつ、そのコードの意図や使われ方まで深く考え、より良いコードを探してもらえると嬉しいです。

5問目までの解説をまだ読んでない方は、ぜひそちらも確認してみてください。

• どちらのコードが好ましい？1−4問目（Kotlin Fest 2024 Pocket Code Battle）
• どちらのコードが好ましい？5問目（Kotlin Fest 2024 Pocket Code Battle）

また、9〜11問目の解説記事を後日公開します。ぜひ最後までお楽しみください。