【DroidKaigi 2023】Code Review Challenge 1問目解説

Yuta Takanashi2023-09-25

LINEアプリ基盤機能開発チーム

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

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

出題タイトル: Tip of the Ice-func

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

object BackupCreator {
    private val localDataSource = LocalDataSource()
    private val remoteDataSource = RemoteDataSource()
    private lateinit var progressDialog: ProgressDialogForBackup

    @WorkerThread
    fun createBackup(target: BackupTarget, since: Date) {
        showProgressDialog()
        uploadBackup(target, since)
        hideProgressDialog()
    }

    private fun uploadBackup(target: BackupTarget, since: Date) {
        compressBackupData(target, since)
        remoteDataSource.upload(File("path/compressed"))
    }

    private fun compressBackupData(target: BackupTarget, since: Date) {
        collectBackupData(target, since)
        compress(source = File("path/raw"), destination = File("path/compressed"))
    }

    private fun collectBackupData(target: BackupTarget, since: Date) {
        val data = when (target) {
            BackupTarget.IMAGE -> localDataSource.selectImageData()
            BackupTarget.VIDEO -> localDataSource.selectVideoData()
            BackupTarget.MESSAGE -> localDataSource.selectMessageData(since)
        }
        writeToFile(File("path/raw"), data)
    }

    private fun showProgressDialog() {
        progressDialog = ProgressDialogForBackup()
        progressDialog.showOnMainThread()
    }

    private fun hideProgressDialog() {
        progressDialog.hideOnMainThread()
    }

    private fun writeToFile(file: File, data: BackupData) {
        val outputStream = file.outputStream()
        try {
            outputStream.write(data.toByteArray())
        } catch (e: IOException) {
            // Ignore
        }
        outputStream.close()
    }

    private fun compress(source: File, destination: File): Unit = // Throws [IOException] if any I/O error occurs.
        TODO("Write data from `source` to `destination` with compression.")
}

enum class BackupTarget { IMAGE, VIDEO, MESSAGE }

class RemoteDataSource {
    fun upload(file: File): Unit = TODO("Upload file to server.") // Throws [IOException] if any I/O error occurs.
}

class LocalDataSource {
    fun selectImageData(): BackupData = TODO("Select image data.")
    fun selectVideoData(): BackupData = TODO("Select video data.")
    fun selectMessageData(since: Date): BackupData = TODO("Select message data created after `since`")
}

これはユーザーのデータをバックアップするためのクラスです。 BackupCreatorは、ローカル(端末上)のデータを集めて一つのファイルに書き出し、それを圧縮してからバックアップとしてサーバーにアップロードします。データの収集にはLocalDataSource、圧縮したデータのアップロードにはRemoteDataSource、また、バックアップの処理中にダイアログを出すためにProgressDialogForBackupを利用しています。バックアップ対象のデータは3種類あり、BackupTargetを利用して呼び出し側から指定することができます。

このコードには複数の問題が含まれていますが、問題の影響が比較的大きい以下の3つを重点的に解説します。

不十分なリソースや例外の扱い
間違った使い方が可能なシグネチャ
見通しの悪い関数の構造

1.不十分なリソースや例外の扱い

バックアップ用のデータをファイルに書き出すwriteToFile()を見てみましょう。

private fun writeToFile(file: File, data: BackupData) {
    val outputStream = file.outputStream()
    try {
        outputStream.write(data.toByteArray())
    } catch (e: IOException) {
        // Ignore
    }
    outputStream.close()
}

try-catchを使って例外を捕捉していますが、それを無視して処理が続行されます。問題が起きている状態で処理を継続しても、後続の処理が成功する可能性は低いです。さらには、中途半端にファイルに書き出されたデータのせいで、現状よりも深刻な問題が発生してしまう可能性もあります。バックアップ処理自体を中断するのが賢明です。

また、close()を使ったリソースの開放も不十分です。close()もIOExceptionを投げる可能性があるので、それも処理する必要があります。 try-catchを追加するよりも、use()(Javaでのtry-with-resourcesに相当します)を使うと良いです。自動で安全にclose()を呼び出してくれるので、リソースの開放忘れも防ぐことができます。

/**
 * Writes [data] to [file] in binary format.
 *
 * Throws [IOException] if any I/O error occurs.
 */
private fun writeToFile(file: File, data: BackupData) {
    file.outputStream().use { outputStream ->
        outputStream.write(data.toByteArray())
    }
}

改善後はこのようなコードになります。他の関数もIOExceptionを投げる可能性があるので、呼び出し元でまとめて例外処理をするようにしました。 Kotlinには検査例外が無いので、呼び出し元に例外処理を強制させるためにkotlin.Resultのようなsealed classを利用するのも良いでしょう。

2.間違った使い方が可能なシグネチャ

次に、BackupCreatorを使う側の気持ちになってみましょう。コードのpublicな部分だけを抜き出すと以下のようになります。

object BackupCreator {
    fun createBackup(target: BackupTarget, since: Date) {
        ...
    }
}

enum class BackupTarget { IMAGE, VIDEO, MESSAGE }

BackupCreator.createBackup()でバックアップを作ることができ、パラメータのtarget: BackupTargetでバックアップ対象、since: Dateでバックアップを取る期間を指定できると想像できます。そこで次のようなコードが書かれたとします。これは意図通りに動くでしょうか？

// 昨日以降のVIDEOのデータをバックアップしたい。
val date = getYesterdaysDate()
BackupCreator.createBackup(BackupTarget.VIDEO, date)

残念ながら、このコードは昨日以降だけではなく全てのVIDEOのデータをバックアップしてしまいます。sinceパラメータは呼び出し先のcollectBackupData()で、BackupTarget.MESSAGEが指定されている時のみ利用されるためです。

private fun collectBackupData(target: BackupTarget, since: Date) {
    val data = when (target) {
        BackupTarget.IMAGE -> localDataSource.selectImageData()
        BackupTarget.VIDEO -> localDataSource.selectVideoData()
        BackupTarget.MESSAGE -> localDataSource.selectMessageData(since)
    }
    writeToFile(File("path/raw"), data)
}

この問題の解決方法として、sinceパラメータをnullableにしたり、ドキュメンテーションコメントを書いたりする方法が考えられます。

/**
 * Creates backup for given [target].
 *
 * [since] is used to specify start data of the backup if [target] is [BackupTarget.MESSAGE]
 */
fun createBackup(target: BackupTarget, since: Date?) {
    ...
}

現状よりも大分マシになりましたが、sealed classを使うことで更に改善することができます。

sealed class BackupTarget {
    data object Image : BackupTarget()
    data object Video : BackupTarget()
    data class Message(val since: Date) : BackupTarget()
}

fun createBackup(target: BackupTarget) {
    ...
}

// Videoのバックアップ
BackupCreator.createBackup(BackupTarget.Video)

// Messageのバックアップ
val date = getYesterdaysDate()
BackupCreator.createBackup(BackupTarget.Message(date))

このようにすることで、ImageやVideoに対してはsinceを意識する必要すらなくなり、間違った使い方が完全にできなくなります。ドキュメンテーションを書くアプローチでも、間違った使い方をされる可能性を小さくすることができますが、0に近い可能性と不可能の間には品質上の大きな差があります。
バグが発生して原因を調査している状況を考えてみましょう。通常は、影響がありそうな範囲を狭めながら原因特定を目指すため、起こり得ない事象は調査の対象外にすることができます。しかし、ドキュメンテーションを書くといったアプローチは、間違った使い方をされている可能性を排除できないため、調査の対象外にすることはできません。 sealed classの様な直和型が表現できるクラスで、望ましく無い状態を排除することは、シンプルですが品質に対する影響力は大きいです。

3. 見通しの悪い関数の構造

createBackup()で利用しているprivate関数の呼び出しの関係は以下のようになっています。

createBackup()
└uploadBackup()
 └compressBackupData()
  └collectBackupData()

createBackup()がuploadBackup()を呼び、そのuploadBackup()は更に別の関数compressBackupData()を呼ぶ...というように関数呼び出しが連鎖しています。

この様に連鎖した関数の構造は以下の点でよくありません。

関数の呼び出しの深い部分まで読まないと、何をしているのかがわからない。
圧縮やアップロードは独立した処理のはずだが、関数の構造上は依存関係がある。
- 不必要な依存関係のせいで、その関数では直接使わないパラメータでも、バケツリレーのように渡し続ける必要がある。
関数の粒度が揃っていないので、可読性が低い。

createBackup()が他のprivate関数を順番に呼び出す構造にするのが良いでしょう。

createBackup()
└collectBackupData()
└compressBackupData()
└uploadBackup()

このような呼び出し構造にすると、createBackup() を見るだけで「バックアップデータを集め、圧縮し、アップロードする」という流れが分かるようになります。

その他の問題

その他、以下のような問題があります。

問題	解決方法
`uploadBackup()`, `compressBackupData()`, `collectBackupData()`がfile pathで暗黙的に依存している	関数のパラメータと戻り値で関連性をわかりやすくする。
`createBackup()`がスレッドセーフではない	排他制御をするかスレッドセーフではないことをドキュメンテーションコメントで明示する。
`BackupCreator`がobject classとして実装されていて、テスタビリティが低い	通常のクラスに変更し、data souceなどの依存をコンストラクタで注入する
`createBackup()`が二回連続で呼ばれると、タイミングによっては古いdialogがhideされない。	finallyを使い確実に後処理を行う。
UI(dialog)の表示とバックアップの作成が同時に行われていて、クラスの責務がわかりにくい。	UI処理を呼び出し元に移す。
作成した一時ファイルを削除していない	finallyを使って消す。

想定回答

問題を全て解決した想定回答は以下になります。

// Manage the instance as a singleton
class BackupCreator(
    private val localDataSource: LocalDataSource,
    private val remoteDataSource: RemoteDataSource
) {
    @Synchronized
    @WorkerThread
    fun createBackup(target: BackupTarget) {
        val tmpFiles = mutableListOf<File>()
        try {
            val rawDataFile = collectBackupData(target).also(tmpFiles::add)
            val compressedDataFile = compress(rawDataFile).also(tmpFiles::add)
            remoteDataSource.upload(compressedDataFile)
        } catch (e: IOException) {
            // TODO: Handle error.
        } finally {
            tmpFiles.forEach(File::delete)
        }
    }

    /**
     * Collects backup data for given [target] and returns file that the data are written.
     *
     * Throws [IOException] if any I/O error occurs.
     */
    private fun collectBackupData(target: BackupTarget): File {
        val data = when (target) {
            BackupTarget.Image -> localDataSource.selectImageData()
            BackupTarget.Video -> localDataSource.selectVideoData()
            is BackupTarget.Message -> localDataSource.selectMessageData(target.since)
        }
        val destination = File("path/raw")
        writeToFile(destination, data)
        return destination
    }

    /**
     * Writes [data] to [file] in binary format.
     *
     * Throws [IOException] if any I/O error occurs.
     */
    private fun writeToFile(file: File, data: BackupData) {
        file.outputStream().use { outputStream ->
            outputStream.write(data.toByteArray())
        }
    }

    /**
     * Writes data from `source` to another file with compression and return the compressed file.
     *
     * Throws [IOException] if any I/O error occurs.
     */
    private fun compress(target: File): File =
        TODO("Write data from `source` to another file with compression and return the compressed file.")
}

sealed class BackupTarget {
    data object Image : BackupTarget()
    data object Video : BackupTarget()
    data class Message(val since: Date) : BackupTarget()
}

class RemoteDataSource {
    /**
     * Uploads the content of the file to the server.
     *
     * Throws [IOException] if any I/O error occurs.
     */
    fun upload(file: File): Unit = TODO("Upload file to server.")
}

class LocalDataSource {
    fun selectImageData(): BackupData = TODO("Select image data.")
    fun selectVideoData(): BackupData = TODO("Select video data.")
    fun selectMessageData(since: Date): BackupData = TODO("Select message data created after `since`")
}

まとめ

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

例外の処理やリソースの開放をuse()などを用いて正しく行う。
利用側が間違った使い方ができないようなシグネチャを設計する。sealed classで望ましくない状態を排除することができる。
関数の構造・粒度・依存関係に注意し、可読性を上げる。

コードレビュー中に"読みにくい・理解が難しいがどこが悪いのかわからない"という経験をしたことがある人は少なくないと思います。そんな時には、3つ目の改善の様に、全体の構造を広く見ることで、問題点が浮かび上がってくるかもしれません。コードレビューに関して、この記事が何か皆さんの気づきに繋がると嬉しいです。

Blog