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

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

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

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

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

#9 A place for every function, and every function in its place

9問目は、JSON として与えられたデータを Map に変換するコードについてです。ただし、この変換は単純ではありません。JSON のキーの文字列 ("message_id-value_type")を MessageId と ValueType に分解し、ネストされたマップ Map<MessageId, Map<ValueType, JsonElement>> を作ります。

具体的な手順としては以下の 3 つのステップに分かれます。

JSON の各アトリビュートを MessageId・ValueType・JsonElement の 3 つ組 KeyParsedElement に変換する
KeyParsedElement を MessageId でグールプ化する
グループ化した KeyParsedElement を Map に変換する (中間状態として Accumulator という MutableMap を使う)

KeyParsedElement と Accumulator の定義は以下の通りです。

private typealias KeyParsedElement = Triple<MessageId, ValueType, JsonElement>
private typealias Accumulator = MutableMap<ValueType, JsonElement>

実装としては、以下の 2 つを提示しました。

コード#9-A

fun parseAsMessageAttributeMap(element: JsonElement): Map<MessageId, Map<ValueType, JsonElement>> =
    element.jsonObject.asSequence()
        .mapNotNull { (key, element) -> parseJsonKey(key, element) }
        .groupingBy { (messageId, _, _) -> messageId }
        .aggregateToMap()
  
private fun parseJsonKey(key: String, element: JsonElement):KeyParsedElement? {
    val (messageId, valueType) = ...
        ?: return null
    return Triple(messageId, valueType, element)
}
  
private fun Grouping<KeyParsedElement, MessageId>.aggregateToMap():
        Map<MessageId, Map<ValueType, JsonElement>> =
    aggregate<KeyParsedElement, MessageId, Accumulator> {
        _, nullableMap, (_, valueType, jsonElement), _ ->
  
        val map = nullableMap ?: mutableMapOf()
        map[valueType] = jsonElement
        map
    }

コード#9-B

fun parseAsMessageAttributeMap(element: JsonElement): Map<MessageId, Map<ValueType, JsonElement>> {
    fun parseJsonKey(key: String, element: JsonElement): KeyParsedElement? {
        val (messageId, valueType) = ...
            ?: return null
        return Triple(messageId, valueType, element)
    }
  
    fun Grouping<KeyParsedElement, MessageId>.aggregateToMap():
            Map<MessageId, Map<ValueType, JsonElement>> =
        aggregate<KeyParsedElement, MessageId, Accumulator> {
            _, nullableMap, (_, valueType, jsonElement), _ ->
  
            val map = nullableMap ?: mutableMapOf()
            map[valueType] = jsonElement
            map
        }
  
    return element.jsonObject.asSequence()
        .mapNotNull { (key, element) -> parseJsonKey(key, element) }
        .groupingBy { (messageId, _, _) -> messageId }
        .aggregateToMap()
}

これら 2 つの違いを端的に言うと、ステップ 1 と 3 のロジックを非ローカルな private 関数として定義するか、ローカル関数として定義するかです。

#9-Aと#9-Bのどちらが良いかのコメントは、おおよそ 2 : 1 の割合で#9-Aのほうが多かったです。

#9-Aが良い

処理の流れが追いやすい (上から順に追える)
階層が浅い
必ずしも細かい処理を読まなくても理解できる

#9-Bが良い

ローカル関数が他で使われないことを保証できる
美しい

解説

この問題では、補助的な関数を切り出したときに、同じ「スコープを限定したい」という目的であっても、視点によって結果が異なるように設定しました。

#9-Aのメリット: 補助的な関数 *が* 参照するスコープを限定できる (補助的な関数がローカル変数を参照していないことを明示しやすい)。関数の流れの概要が把握しやすくなる。
#9-Bのメリット: 補助的な関数 *を* 参照するスコープを限定できる (他の関数が補助的な関数を使っていないことを明示しやすい)。

「スコープを限定する」と言ったときは、その要素 *を* 使うコードを限定することに意識が向きがちですが、それと同じぐらい、要素 *が* 使うコードを限定することも重要です。そのどちらを優先するかについては、好みによっても分かれるでしょう。

ただし、関数を構成するときはスコープの厳密性だけでなく、関数の流れがトップダウンに把握しやすいかどうかも重要です。この点に関しては多くのコメントがあった通り、#9-Aのほうがより好ましいです。もし、「補助的な関数を参照するスコープ」も限定したいならば、#9-Aのコードに加え、3 つの関数をまとめるような object (か class) を作ることで、両方のメリットを達成できます。特に拡張関数を定義する場合は注意が必要で、ローカル変数だけでなくプロパティの参照もないことを明示できたほうが無難です。

#10 Responsibility for nothing

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

コード#10-A

class ProfileRepository {
    fun hasPremiumProfile(): Boolean = TODO()
}
  
class ProfileNavigator(
    private val repository: ProfileRepository
) {
    fun openProfileScreen() {
        if (repository.hasPremiumProfile()) {
            openPremiumProfileScreen()
        } else {
            openNormalProfileScreen()
        }
    }
    ...
}
  
val navigator = if (isProfileFeatureSupported) {
    ProfileNavigator(ProfileRepository())
} else {
    null
}
navigator?.openProfileScreen()

コード#10-B

class ProfileRepository {
    fun hasPremiumProfile(): Boolean = TODO()
}
  
class ProfileNavigator(
    private val repository: ProfileRepository?
) {
    fun maybeOpenProfileScreen() {
        if (repository == null) return
  
        if (repository.hasPremiumProfile()) {
            openPremiumProfileScreen()
        } else {
            openNormalProfileScreen()
        }
    }
    ...
}
  
val navigator = if (isProfileFeatureSupported) {
    ProfileNavigator(ProfileRepository())
} else {
    ProfileNavigator(null)
}
navigator.maybeOpenProfileScreen()

これはProfileNavigatorを利用してプロフィール画面を表示するためのコードです。isProfileFeatureSupportedはプロフィール画面がサポートされているかを表すBooleanで、falseの場合にはプロフィール画面を表示することができません。

どちらのコードもisProfileFeatureSupportedがfalseの場合に、依存しているクラスの参照をnullで持つことで、画面遷移が不可能なことを表現しています。ただし、#10-AはProfileNavigator自体をnullableとして扱い、 #10-BはProfileNavigatorが依存しているProfileRepositoryをnullableとして扱っています。

ブースで集まったコメントは以下の通りで、#10-Aを好む人が非常に多かったです。

#10-Aが良い

呼び出し側で画面が開かれるかがわかりやすい。
機能自体がサポートされているかどうかの判断はリポジトリの責務ではない。
ProfileNavigatorという名前ならば、必ずプロフィール画面を開くほうが良い。
nullを深い場所で扱いたくない。

#10-Bが良い

プロフィール画面がサポートされていない場合に、別の画面を出すような将来の変更をしやすい。

解説

コメントで既に指摘されているように、呼び出し側(最後の行)で画面が開かれるかがわかりやすいことが#10-Aの大きなメリットです。#10-BのmaybeOpenProfileScreen()から、画面が開かれないケースが存在すると予想することはできますが、どのような条件で開けないかを知るためには関数の中身を読む必要があります。

また、ProfileRepositoryに依存するProfileNavigatorの全ての関数の先頭にearly returnを書かなければならない点が、#10-Bのデメリットとして挙げられます。

#10-Bでは将来的な変更をProfileNavigator内部に閉じられる可能性もありますが、YAGNIやその他のデメリットを考慮すると、現時点の仕様では#10-Aのほうが望ましいと言えます。

#11 In Search of Lost Event

11問目ではCoroutines Flowの中でもStateFlow, SharedFlowを使った問題を出題しました。

コード#11-A

// Flow definitions
private val _openNextScreenRequest = MutableStateFlow<Unit?>(null)
val openNextScreenRequest = _openNextScreenRequest.asStateFlow()
fun requestToOpenNextScreen() {
    _openNextScreenRequest.value = Unit
}
 
fun consumeOpenNextScreenRequest() {
    _openNextScreenRequest.value = null
}
// Caller implementation
viewModel.openNextScreenRequestStateFlow.filterNotNull()
    .collect {
        viewModel.consumeOpenNextScreenRequest()
        openNextScreen()
    }

コード#11-B

// Flow definition
private val _openNextScreenRequest = MutableSharedFlow<Unit>(extraBufferCapacity = 1)
val openNextScreenRequest = _openNextScreenRequest.asSharedFlow()
fun requestToOpenNextScreen() {
    _openNextScreenRequest.tryEmit(Unit)
}
 
// Caller implementation
viewModel.openNextScreenRequestStateFlow.collect {
    openNextScreen()
}

「次の画面を開く」のような、1度だけ実行したいイベント(one-shot event)をViewModelからViewレイヤーまで伝播させるようなケースを考えます。 Listenerパターンを使用するとViewModelでViewのオブジェクトへの参照を持ってしまうことになり、メモリリークを発生させる原因となってしまうため、Observableな仕組みを用いてイベントをView側で監視する形式が一般的になっていると思います。その中でも、KotlinのCoroutinesに含まれるFlowを使用したイベントの伝搬の仕組みは、他のsuspend関数との連携のしやすさや、ライフサイクル周りでの安全性の高さ、そしてComposeにおいてもStateへと変換しやすいことから近年よく使用されていると思います。 #11-AではStateFlowを用いた実装、#11-BではSharedFlowを用いた実装を示しています。

ブースで集まったコメントはとしては、大きな偏りはなく両方に良い点悪い点がしっかりと書かれており、Bのほうが良いというコメントが少しだけ多いかなという感じでした。

#11-Aが良い

bufferが詰まることがなさそう
StateFlowなので状態が確定している

#11-Bが良い

nullを使用していない
consumeがセットになっていないのでわかりやすい

解説

StateFlowとSharedFlowは似ていますが、大きく以下の点で異なります。

StateFlowは初期値と現在の値を持つ。SharedFlowは現在の値という概念ではなく、repeatで指定された数の要素を保持する。
StateFlowは値が等価だった場合はsubscriberに通知しない(distinctUntilChanged)

#11-Aでは、StateFlowを使用してone-shot eventを実現するために、subscriber側でイベントを処理するタイミングでStateFlowの現在地をnull(初期状態)にする consumeOpenNextScreenRequest を呼んでいます。 #11-Bでは、subscriber側でイベントが処理されたタイミングで自動的にSharedFlowから値が消える(repeatが0なので保持されない)ため、consumeに相当する関数を準備する必要はありません。

一見すると#11-Bのほうがシンプルにかけそうですが、気をつけなければいけない点があります。それが、collectが行われていないときにemitされたイベントを失ってしまうという点です。 collectが実行される前や、ライフサイクルによってcollectがキャンセルされている間にViewModel側からemitされた場合、その値はバッファリングされずに破棄されてしまいます。このようなケースが発生しない、もしくは無視できる場合はSharedFlowを使い、それ以外のケースではStateFlowを使用するとよいでしょう。

おわりに

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

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

Pocket Code Battleの問題解説シリーズはこれで最後になります。

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

またLINEヤフー企業ブースでお会いしましょう！