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

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

出題タイトル: Split by object, not condition

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

class PlayerPresenter(
    private val loadingView: View,
    private val playButton: View,
    private val pauseButton: View,
    private val progressView: TextView,
    private val player: Player,
    private val lifecycleScope: CoroutineScope
) {
    private var isPlaying: Boolean = false
  
    init {
        showLoadingView()
        lifecycleScope.launch {
            withContext(Dispatchers.IO) {
                player.prepare()
            }
            showPausingView()
        }
    }
  
    private fun showLoadingView() {
        loadingView.isVisible = true
        playButton.isVisible = false
        pauseButton.isVisible = false
        progressView.isVisible = false
    }
  
    private fun showPausingView() {
        loadingView.isVisible = false
        playButton.isVisible = true
        playButton.setOnClickListener {
            player.play()
            showPLayingView()
        }
        pauseButton.isVisible = false
        progressView.isVisible = true
        isPlaying = false
    }
  
    private fun showPLayingView() {
        loadingView.isVisible = false
        playButton.isVisible = false
        pauseButton.isVisible = true
        pauseButton.setOnClickListener {
            player.pause()
            showPausingView()
        }
        progressView.isVisible = true
        isPlaying = true
        lifecycleScope.launch {
            while (isPlaying) {
                progressView.text = "${player.currentPosition / 1000}/${player.duration / 1000}"
                delay(100)
            }
        }
    }
}

こちらの `PlayerPresenter` は、音声や動画といったメディアをコントロールし、その再生状態を表示するクラスです。`Player` プロパティのロード中、停止中、再生中という 3 種類のステータスを、`loadingView`、`playButton`、`pauseButton`、`progressView`の 4 つの View の表示状態に反映させるクラスです。

状態の遷移として、 `PlayerPresenter` インスタンスの生成時にロード中となり、ロードが完了すると、停止中となります。再生ボタン、停止ボタンの押下により、再生中、停止中と状態がトグルします。
また、再生中には再生時間が `progressView` に更新され続けます。

このクラスは可読性、メンテナンス性、頑健性、動作の安定性などに対するいくつかの問題を抱えています。大きな問題として以下の 3 点に着目して解説します。

• 状態が条件ごとに反映されている
• 信頼できる状態のデータを反映していない
• コルーチンのキャンセル処理が正しくなされていない

状態が条件ごとに反映されている

`PlayerPresenter` は、状態を各 View に反映させるための 3 つの private 関数を持っています。それぞれ反映させたい状態に対応して、 `showLoadingView`, `showPausingView`, `showPLayingView` となります。それぞれの関数は、 `loadingView` や `playButton` といった共通の View を更新しています。

これらの関数の定義の仕方による問題はなんでしょうか。

最大の問題は拡張時のメンテナンス性にあります。仮に今、ロードされているメディアのタイトルを表す `titleView` を追加することを考えます。この場合、以下のように再生中だけではなく、ロード中、停止中に対応する関数も忘れずに修正する必要があります。

private fun showLoadingView() {
    ...
    titleView.isVisible = false
}
 
private fun showPausingView() {
    ...
    titleView.isVisible = true
}
 
private fun showPausingView() {
    ...
    titleView.isVisible = true
}

同種の問題として、例えば再生終了時に関連したメディアを表示するような追加のステータスが増えた時に `showLoadingView`, `showPausingView`, `showPLayingView` といったすべての関数で `relativeMediaView` を更新する必要がありますが、どれか 1 箇所でも追加を忘れると、バグを発生させてしまいます。

private fun showFinishedView() {
    // Don't forget to control all views
    loadingView.isVisible = false
    playButton.isVisible = false
    pauseButton.isVisible = false
    progressView.isVisible = false
    // Control new view
    relativeMediaView.isVisible = true
}

また、このコードには可読性の問題もあります。関心のある View が結局どの状態でどのような表示になるのかを知るためには、その View を更新している全ての関数を把握する必要があるため、非常に困難です。

この問題を改善するには、以下の例のように View の更新を行う関数を表示の状態によって分けるのではなく、 まずは更新対象である View によって分けるのがよいでしょう。その際に View を ViewStatus として列挙します。

private enum class ViewStatus { LOADING, PAUSING, PLAYING }
 
private fun bindViewStatus(viewStatus: ViewStatus) {
    loadingView.isVisible = viewState == ViewState.LOADING
    playButton.isVisible = viewState == ViewStatus.PAUSING
    pauseButton.isVisible = viewState == ViewStatus.PLAYING 
    progressView.isVisible = viewState == ViewStatus.PAUSING || viewState == ViewStatus.PLAYING
}

この変更により、`showLoadingView`, `showPausingView`, `showPLayingView` が 1 つの関数に統合され、各 View がどの状態でどのように表示されるのかが一目で分かるようになりました。

特定の状態でしか表示しない `progressView` で表示する時間についても `ViewStatus` と深く関係があります。よって、表示する時間についても下記のように ViewStatus で表現することで View の更新を `bindViewStatus` に集約でき、View の表示についての一覧性がさらにあがります。

private sealed class ViewStatus(val currentPosition: Int?) {
    object Loading : ViewStatus(null)
    class Playing(currentPosition: Int) : ViewStatus(currentPosition)
    class Pausing(currentPosition: Int) : ViewStatus(currentPosition)
}
 
private fun bindViewStatus(viewStatus: ViewStatus) {
    loadingView.isVisible = viewStatus is ViewStatus.Loading
    playButton.isVisible = viewStatus is ViewStatus.Playing
    pauseButton.isVisible = viewStatus is ViewStatus.Pausing
    progressView.isVisible = viewStatus.currentPosition != null
    progressView.text = "${viewStatus.currentPosition / 1000}/${player.duration / 1000}"
}

信頼できる状態のデータを反映していない

`PlayerPresenter` には現在表示している View の状態がプロパティなどに保持されていません。では、現在表示されている View の状態はなにかというと、ボタンの押下などによるイベント時によって最後に呼び出された View を更新する関数、ということになります。

今回、最も信頼すべき View の状態を表すデータはどこにあるのでしょうか。それは `player` プロパティが保持しているはずです。現在のコードでは再生が本当に始まったのか、など `player` の実際の状態を知ることができません。

playButton.setOnClickListener {
    player.play()
    // Cannot know actual player status, but update views with playing status anyway
    showPLayingView()
}

実データと View の状態の同期が非常に困難で、頑健性が非常に低い状態と言えるでしょう。実際に今回のクラスではメディアを再生し終わった場合でも `player` の状態を更新できないため `PlayButton` が可視のまま残る、という問題にもつながっています。

これを改善するには `player` の状態を監視し、状態の変更の都度 `bindViewStatus` を呼び View に状態を反映させるような構造にするとよいでしょう。例えば、 `player` がその状態を Flow で公開していた場合は、以下のようになります。

init {
    ...
    lifecycleScope.launch {
        player.status.collect { newStatus ->
            bindViewStatus(convertToViewStatus(newStatus))
        }
    }
}

もし `player` が監視可能な状態を提供していない場合は、状態を監視できるようにするためのラッパークラスを作るのも手段の一つとして考えられます。

コルーチンのキャンセル処理が正しくなされていない

今回のコードでは再生時間を更新するため、再生中に 100ms のディレイをかけながら while ループが実行されています。また、ループの制御のために `isPlaying` が利用されています。

lifecycleScope.launch {
    while (isPlaying) {
        progressView.text = ...
        delay(100)
    }
}

このコードにはバグがあり、停止ボタン、再生ボタンが 100ms 以内に 2 回押下された場合 `isPlaying` フラグは true のままになり、この while ループは停止されません。結果として、複数のループが同時に実行される可能性があります。
これを改善するには、 `launch` したときの Job を保存しておき `launch` する前にひとつ前の Job をキャンセルできるようにすると良いでしょう。

progressUpdateJob?.cancel()
progressUpdateJob = lifecycleScope.launch {
    while (isPlaying) {
        progressView.text = ...
        delay(100)
    }
}

また `PlayerPresenter` の中で while ループで都度更新するのではなく、 再生位置についても View の状態と同様に `player` から監視可能な状態にしておき、再生位置の更新の都度、 `bindViewStatus` を呼び出すことでさらに頑健な構造になるでしょう。

その他の問題点

以上の 3 点が大きな問題でしたが、他にも細かい改善ポイントがあります。

例えば `ClickListener` が View の状態を更新するたびに設定されていますが、クリック時のロジックは状態により変わりません。この場合は `init` の中で設定することができます。

他にもメディアのロードの開始が、プレゼンタークラスの初期化に隠されていることも可読性の問題を生みそうです。

また View に表示されるテキストがハードコードされています。

progressView.text = "${viewStatus.currentPosition / 1000}/${player.duration / 1000}"