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

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

出題タイトル: Bitter Sweets

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

class MainActivity : AppCompatActivity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContentView(R.layout.activity_main)
        val request: Request = if (Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU) {
            @Suppress("DEPRECATION")
            intent.getParcelableExtra("request")
        } else {
            intent.getParcelableExtra("request", Request::class.java)
        } ?: return
        val viewModel = MainViewModel()
        lifecycleScope.launch {
            viewModel.createDataFlow(request).collect(::updateView)
        }
    }
 
    private fun updateView(viewData: String) {
        // TODO: Update Views by [viewData].
    }
 
    companion object {
        fun createIntent(context: Context, request: Request): Intent =
            Intent(context, MainActivity::class.java)
                .putExtra("request", request)
    }
}
 
class MainViewModel : ViewModel() {
    suspend fun createDataFlow(request: Request): Flow<String> {
        val localCacheDeferred = loadDataAsync(request, true)
        val remoteCacheDeferred = loadDataAsync(request, false)
        return flow {
            emit(localCacheDeferred.await())
            emit(remoteCacheDeferred.await())
        }
    }
 
    private suspend fun loadDataAsync(request: Request, local: Boolean): Deferred<String> =
        coroutineScope { async { loadData(request, local) } }
 
    private suspend fun loadData(request: Request, local: Boolean): String {
        // TODO: This is dummy. Load data from local cache or remote.
        delay(if (local) 1000L else 5000L)
        return "result of '$request'"
    }
}
 
data class Request(val word: String, val page: Int) : Parcelable {
    override fun writeToParcel(parcel: Parcel, flags: Int) {
        parcel.writeString(word)
        parcel.writeInt(page)
    }
 
    override fun describeContents(): Int = 0
 
    companion object {
        @JvmField
        val CREATOR = RequestCreator
    }
}
 
object RequestCreator : Parcelable.Creator<Request> {
    override fun createFromParcel(parcel: Parcel): Request =
        Request(checkNotNull(parcel.readString()), parcel.readInt())
 
    override fun newArray(size: Int): Array<Request?> = arrayOfNulls(size)
}

このコードでは、引数として受け取った Request オブジェクトを用いて、その詳細なデータを表示する Activity を実装しています。 Activity に表示されるデータは MainViewModel クラスの createDataFlow 関数が Flow<String> の型で提供しています。 この Flow には、1つ目の要素としてローカルストレージ内のキャッシュデータ、2つ目の要素としてリモートサーバーから取得した最新のデータが流れてくるような仕様になっており、「ローカルのデータの表示をとにかく真っ先に行い、その後最新のデータで上書きする」というユースケースを想定しています。

問題点と解決策

このコードには複数の問題が含まれていますが、意図しない挙動やクラッシュを引き起こすといった、影響が比較的大きい以下の2つを重点的に解説します。

1. Android Tiramisu で追加された新しい getParcelableExtra API に関する問題
2. loadDataAsync 関数と CoroutineScope の構造の問題

1. Android Tiramisu で追加された新しい getParcelableExtra API に関する問題

Request クラスのオブジェクトが MainActivity のIntent Extraとして格納されており、onCreate 関数でそのオブジェクトを取得しています。

val request: Request = if (Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU) {
    @Suppress("DEPRECATION")
    intent.getParcelableExtra("request")
} else {
    intent.getParcelableExtra("request", Request::class.java)
}

ここでは、Android Tiramisuで新しく導入されたタイプセーフな getParcelableExtra APIを適切に使用するために、Android Tiramisu以上では新しいAPI、それ以外では従来のAPIを使用するような実装になっています。

しかし、この新しいgetParcelableExtra APIにはAndroid Tiramisu のみでバグがあります。 https://issuetracker.google.com/issues/240585930

症状としては、「Parcelable の CREATOR が Parcelable のクラスの入れ子クラスでない場合に NullPointerException が発生する」というものです。

今回のコードでは、Request クラスの Parcelable に関する実装を自前で行っており、さらにその実装の仕方でわざとこのバグを満たすような作りにしています。

data class Request(val word: String, val page: Int) : Parcelable {
    override fun writeToParcel(parcel: Parcel, flags: Int) {
        parcel.writeString(word)
        parcel.writeInt(page)
    }
 
    override fun describeContents(): Int = 0
 
    companion object {
        @JvmField
        val CREATOR = RequestCreator
    }
}

object RequestCreator : Parcelable.Creator<Request> {
    override fun createFromParcel(parcel: Parcel): Request =
        Request(checkNotNull(parcel.readString()), parcel.readInt())
 
    override fun newArray(size: Int): Array<Request?> = arrayOfNulls(size)
}

この問題を回避するための一番簡単な方法は、androidx-coreライブラリに含まれる IntentCompat.getParcelableExtra を使用することです。 このAPIの内部では、Android Tiramisuにおいても古いgetParcelableExtra APIを使用するような実装になっており、この問題を回避することが出来ます。

2. loadDataAsync 関数と CoroutineScope の構造の問題

MainViewModel クラスの createDataFlow 関数では、「ローカルのデータの表示をとにかく真っ先に行い、その後最新のデータで上書きする」というユースケースを実現するために、2つの非同期関数を呼び出し、その結果を Deferred を用いて取得して flow に流すという実装になっています。

createDataFlow 関数の中で使用している loadDataAsync 関数は名前からすると非同期関数なので、この内部の処理の完了は待たずに値が返ってきて、その後の await で内部の処理を待つことを期待する実装になっています。 初めにローカルからのデータを await して Flow に emit し、その後にリモートのデータを await して Flow に emit するという順番のため、値がやってくる順番としても期待通りの正しい順番となります。

一見すると問題なさそうに見えますが、この loadDataAsync 関数には問題があり、想定通りの挙動をしません。

loadDataAsync 関数の内部では、async 関数を呼び出すために新しく CoroutineScope を coroutineScope 関数を使って作っています。

private suspend fun loadDataAsync(request: Request, local: Boolean): Deferred<String> =
    coroutineScope { async { loadData(request, local) } }

async 関数はレシーバーが CoroutineScope になっているため、「とりあえず coroutineScope でくくった」という感じでコードを書いてしまった想定です。

ここで問題になるのは coroutineScope は内部で起動されたコルーチンを待機してしまうという点です。 async 関数により新しいコルーチンが起動し、そのコルーチン内で loadData 関数が呼ばれます。

すると、結果的に coroutineScope 自身が内部で起動されたコルーチンに含まれる loadData 関数の完了を待ってしまいます。 そうなってしまうと、 loadDataAsync 関数は結局非同期関数でありながら同期的に全ての処理を待ってしまい、createDataFlow 関数で最初の emit が呼ばれるまでにデータの取得が全て終わってしまっているような状態になってしまいます。

今回の問題のコードでは、loadData 関数はローカルからのデータの取得は1秒、リモートからのデータの取得は5秒という擬似コードで実装されています。

そのため、実際の挙動としては上の図のように「6秒後にローカルのデータ、それと同時にリモートのデータが Flow に流れてくる」という感じなってしまっています。

期待する挙動としては上の図のように、「1秒後にローカルのデータ、その4秒後にリモートのデータが Flow に流れてくる」というものです。

このように修正するためには、coroutineScope の位置を正しくするだけでなく、Flow の作り方にも気を配る必要があります。

Flow はそれ自身が非同期で値が流れてくるようなオブジェクトなので、基本的に Flow を返す関数はsuspend関数ではなく、Flow をすぐに返し、値の生成に必要な時間個かかる処理はFlow Builderの中で行うべきです。

createDataFlow 関数のように、Flow を返しながらsuspend関数になっているようなコードをレビューなどで見つけたときには注意してみると良いでしょう。 同様に、loadDataAsync のような非同期関数もsuspend関数になっている場合は何かしら変なことをしてしまっている可能性が高いので、そちらも注意しましょう。

以下のコードのように emit や await まで含めて coroutineScope に含めることで期待通りの動作をさせることが出来ます。

fun createDataFlow(request: Request): Flow<String> = flow {
    coroutineScope {
        val localCacheDeferred = loadDataAsync(request, true)
        val remoteCacheDeferred = loadDataAsync(request, false)
        emit(localCacheDeferred.await())
        emit(remoteCacheDeferred.await())
    }
}

private fun CoroutineScope.loadCacheDataAsync(request: Request, local: Boolean): Deferred<String> =
    async { loadData(request, local) }

同時に、flow {} の中に loadDataAsync の呼び出しが移動するため、createDataFlow 関数も非suspend関数にすることが出来ます。

その他の問題

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

1. Activityの lifecycleScope でFlowを collect する場合、STOPPED でも collect が動き続けてしまう

問題のコードでは、Flowを lifecycleScope の中で直接 collect しています。

lifecycleScope.launch {
    viewModel.createDataFlow(request).collect(::updateView)
}

この場合、collect はLifecycleが DESTROYED にならないとキャンセルされず、STOPPED でも動き続けてしまうため、無駄なリソースを使用することになります。

repeatOnLifecycle や flowWithLifecycle を使用して STOPPED になったら updateView が呼ばれないようにすると良いでしょう。

lifecycleScope.launch {
    lifecycle.repeatOnLifecycle(Lifecycle.State.STARTED) {
        viewModel.createDataFlow(request).collect(::updateView)
    }
}

lifecycleScope.launch {
    viewModel
        .createDataFlow(request)
        .flowWithLifecycle(lifecycle, Lifecycle.State.STARTED)
        .collect(::updateView)
}

2. SavedStateHandle を使用していない

問題のコードでは、Intent に渡された Request オブジェクトを Activity で取得し、ViewModel の関数の引数として渡していました。 しかし、このように ViewModel で直接使用する場合には SavedStateHandle を使用し、値を ViewModel 側で直接引数を受け取るようにするとよいでしょう。

class MainViewModel(savedStateHandle: SavedStateHandle) : ViewModel() {
    private val request: Request = checkNotNull(savedStateHandle[KEY_REQUEST])

    // snip
}

3. Intent の Extra の key が繰り返し直値で書かれている

問題のコードでは、Intent の Extra に渡す Request の put および get のタイミングで使用する key の値として "request" が使用されていますが、この文字列がリテラルとして3回書かれています。 このような書き方では、万が一1箇所のみ別の文字列になっていたとしてもコンパイル時にエラーかどうかの検証ができません。

定数値として定義し、その定数を用いるようにしましょう。

4. ViewModel クラスを直接インスタンス化しているため、画面回転時などにデータが破棄される

MainActivity の onCreate 関数内で MainViewModel を直接インスタンス化してしまっています。

val viewModel = MainViewModel()

このようにしてしまうと、画面回転やダークモードの切りかえといったConfiguration変更などにより Activity が破棄された後、再度 Activity が作られた際に以前のデータを復元できなくなってしまいます。 by viewModel() などのAPIを用いて、正しい方法で ViewModel を利用するようにしましょう。

val viewModel: MainViewModel by viewModel()

5. onCreate 内での return により、Activity が中途半端な状態で初期化される可能性がある

onCreate の処理の途中でreturnをしてしまうと、中途半端な状態で Activity が初期化されてしまう可能性があります。 特に、lateinit なフィールドに代入するよりも前に return してしまい、onDestroy で未初期化の lateinit フィールドにアクセスしてしまうというケースが非常に起きやすいです。

onCreate 内での return はできる限り避け、もし本当に必要な場合はそのような挙動をする場合のテストを自動テストとして実行するといいでしょう。

6. ViewModel がリポジトリのような働きをしている

ケースバイケースにはなりますが、複数のデータソースを持つデータを読み込む場合はリポジトリパターンを用いて、各データソース特有の処理などを隠蔽することで頑健性やテスト可能性が担保できます。

7. loadDataAsync や loadData の引数 local

loadData 関数は、引数 local がtrueの時にローカルからデータを取得、falseの時にリモートからデータを取得というように挙動が変わる関数です。 このように引数で関数の挙動が大きく変わってしまうと、挙動自体を理解するのにコードを深く読まなければいけなかったり、特定の条件のみで使用する引数が増えてしまったりしやすくなります。

挙動を大きく変える引数を追加する代わりに、関数自体を分け、関数名にその関数で行うことを詳しく記述することでコードの可読性を上げることができます。

8. Parcelable クラスの実装方法

問題のコードでは、Request クラスの Parcelable の実装を全て自前で書いています。 昔のAndroidアプリではよく見た光景ですが、今の時代であれば Parcelize プラグインを使用し、シリアライズ・デシリアライズに関するコードは自動生成にすることで間違いを減らせ、ボイラープレートコードを減らすことが出来ます。

9. Parcelable クラスと難読化

minifyEnabled オプションを有効にすると、R8による難読化により、Request のクラス名が置き換わります。 多くの場合は難読化によるクラス名の変更で問題は起きません。 しかし、シリアライズ可能なクラスの名前がアプリバージョンによって異なるものになってしまったとき、正しくデシリアライズできなくなります。

また、アプリのアップデート後でもRecent Appなどからアプリが起動されると古いアプリで作成された Intent を元にアプリが起動されることがあります。 この2つの状況が重なってしまうと、別の名前に難読化された Parcelable によるクラッシュが発生してしまいます。

これを防ぐためには、@Keep アノテーションやProGuardルールファイルを使用して Parcelable クラスに関しては難読化をしない設定にすることで回避が可能です。

想定解答

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

class MainActivity : AppCompatActivity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        val viewModel: MainViewModel by viewModels { MainViewModel.Factory }
        lifecycleScope.launch {
            repeatOnLifecycle(Lifecycle.State.STARTED) {
                viewModel.viewDataFlow.collect(::updateView)
            }
        }
    }
    private fun updateView(viewData: String) {
        // TODO: Update Views by [viewData].
    }
    companion object {
        fun createIntent(context: Context, request: Request): Intent =
            Intent(context, MainActivity::class.java)
                .putExtra(MainViewModel.KEY_REQUEST, request)
    }
}

class MainViewModel(
    savedStateHandle: SavedStateHandle,
    private val repository: MainRepository
) : ViewModel() {
    private val request: Request = checkNotNull(savedStateHandle[KEY_REQUEST])
    val viewDataFlow: Flow<String> by lazy { repository.createDataFlow(request) }
    object Factory : ViewModelProvider.Factory {
        override fun <T : ViewModel> create(modelClass: Class<T>, extras: CreationExtras): T {
            @Suppress("UNCHECKED_CAST")
            return MainViewModel(extras.createSavedStateHandle(), MainRepository()) as T
        }
    }
    companion object {
        const val KEY_REQUEST = "request"
    }
}

class MainRepository {
    fun createDataFlow(request: Request): Flow<String> = flow {
        coroutineScope {
            val localCacheDeferred = loadCacheDataAsync(request)
            val remoteCacheDeferred = fetchRemoteDataAsync(request)
            emit(localCacheDeferred.await())
            emit(remoteCacheDeferred.await())
        }
    }

    private fun CoroutineScope.loadCacheDataAsync(request: Request): Deferred<String> =
        async { loadCacheData(request) }

    private fun CoroutineScope.fetchRemoteDataAsync(request: Request): Deferred<String> =
        async { fetchRemoteData(request) }

    private suspend fun loadCacheData(request: Request): String {
        // TODO: Load data from local cache.
        delay(1000)
        return "local cache result of '$request'"
    }
    private suspend fun fetchRemoteData(request: Request): String {
        // TODO: Fetch data from remote.
        delay(5000)
        return "remote result of '$request'"
    }
}

@Parcelize
@Keep
data class Request(val word: String, val page: Int)

まとめ

この記事では、Code Review Challengeの5問目の解説をおこないました。

Android OSに関連したバグや、頭の中でデバッグしにくい挙動など、比較的難易度の高いポイントを詰め込んだ問題になります。 これらに関しては、最新の情報をキャッチアップすることやユニットテストをきちんと書くことで気づくことが出来るでしょう。

また、それ以外にも「そういえばそういう書き方じゃなくてこっちの書き方いつもしているなぁ」という点もいくつか取り入れていました。 コードレビューをおこなうときに斜め読みをしてしまうと見落としてしまいやすい箇所ですが、アプリのクオリティを下げてしまう要因になるのでレビュワーとしてもしっかり細部までコードを見て、「本当にその書き方で問題無いのか」という部分に少し目を向けられるとレビュー力が上がると思います。