コード品質向上のテクニック：第54回沈黙は金メッキかも

こんにちは。コミュニケーションアプリ「LINE」のモバイルクライアントを開発している石川です。

この記事は、毎週木曜の定期連載 "Weekly Report" 共有の第 54 回です。 LINEヤフー社内には、高い開発生産性を維持するための Review Committee という活動があります。ここで集まった知見を、Weekly Report と称して毎週社内に共有しており、その一部を本ブログ上でも公開しています。（Weekly Report の詳細については、過去の記事一覧を参照してください）

以下のような API 呼び出しの結果を意味するデータモデルがあるとします。

package ...

import ...ApiErrorType

/** ... */
sealed interface ApiResult<T> {
    class Success<T>(val value: T) : ApiResult<T>
    class Failed(val errorType: ApiErrorType) : ApiResult<Nothing>
}

このような「失敗するかもしれない」結果を意味するデータモデルに、map や flatMap といった関数を定義すると、操作をチェーンできるようになり、便利なことが多いです。

以下の例では、ApiResult に mapSuccess と flatMap を追加しています。どちらも「成功時には次の操作を行う」というコードを、メソッドチェーンで書けるようにすることを意図しています。

@file:Suppress("warnings")
package ...

import ...ApiErrorType

/** ... */
sealed interface ApiResult<T> {
    class Success<T>(val value: T) : ApiResult<T>
    class Failed(val errorType: ApiErrorType) : ApiResult<Nothing>

    /** ... */
    fun <S> mapSuccess(transform: (T) -> S): ApiResult<S> = when (this) {
        is Success -> Success(transform(value))
        is Failed -> this as ApiResult<S>
    }

    /** ... */
    fun <S> flatMap(transform: (T) -> ApiResult<S>): ApiResult<S> = when (this) {
        is Success -> transform(value)
        is Failed -> this as ApiResult<S>
    }
}

このコードになにか問題はありますか？

良薬は口に苦し

このコードでは、 @file:Suppress("warnings") によってファイル全体のすべての警告を抑制しています。ただし、実際に抑制したい警告は as ApiResult<S> で発生する "unchecked cast" だけです。ファイル全体のすべての警告を抑制してしまうと、将来発生する警告も意図せず抑制してしまい、バグを見過ごしてしまう可能性があります。例えば、ライブラリの更新などで、使っている関数が Deprecated になったことを見落とす可能性もあるでしょう。

警告については、無視しない、過剰に抑制しない よう気をつける必要があります。警告が発生した場合は、以下のどちらかを実施する必要があります。(ただし、1 が優先されるべきです)

警告の原因を解決する
抑制の範囲を小さくとどめ、抑制の理由をコメントで説明する

1: 警告の原因を解決する

可能であれば警告を抑制せず、原因を根本から解決するのが望ましいです。今回の例では、タイプパラメータ T を共変 (covariant) にし、ダウンキャストを削除することで警告を解決できます。Kotlin では、以下のように out モディファイアを使うことで、T を共変にできます。

/** ... */
sealed interface ApiResult<out T> {
    class Success<T>(val value: T) : ApiResult<T>
    class Failed(val errorType: ApiErrorType) : ApiResult<Nothing>

    /** ... */
    fun <S> mapSuccess(transform: (T) -> S): ApiResult<S> = when (this) {
        is Success -> Success(transform(value))
        is Failed -> this
    }

    /** ... */
    fun <S> flatMap(transform: (T) -> ApiResult<S>): ApiResult<S> = when (this) {
        is Success -> transform(value)
        is Failed -> this
    }
}

仮に T を共変にすべきでない (不変を維持するべき) という状況であるならば、Failed のタイプパラメータを Nothing から T に変え、map や flatMap が呼ばれる度に Failed インスタンスを作り直すという方法もあります。

sealed interface ApiResult<T> {
    class Success<T>(val value: T) : ApiResult<T>
    class Failed<T>(val errorType: ApiErrorType) : ApiResult<T>

    /** ... */
    fun <S> mapSuccess(transform: (T) -> S): ApiResult<S> = when (this) {
        is Success -> Success(transform(value))
        is Failed -> Failed(errorType)
    }

    /** ... */
    fun <S> flatMap(transform: (T) -> ApiResult<S>): ApiResult<S> = when (this) {
        is Success -> transform(value)
        is Failed -> Failed(errorType)
    }
}

2: 抑制の範囲を小さくとどめ、抑制の理由をコメントで説明する

警告の原因を解決できないときは、抑制せざるを得ないこともあります。警告の原因が外部のライブラリやフレームワークに起因する状況などが、その代表例です。他にも、解決案を思いつかなかったという状況もありえます。そのような場合は、以下の 3 点に気をつけてください。

抑制のコード範囲を限定する: ファイルレベルの抑制よりも、クラス、関数、ステートメントレベルの抑制を選ぶ
抑制の対象となる警告を限定する: "warnings" や "all" といったすべての警告を抑制するのではなく、"UNCHECKED_CAST" のように警告の種類を特定する
コメントで理由を書く: なぜ抑制が必要なのか、原因の解決ができないのかをコメントで説明する

コードを書いているときに警告の原因を解決できなかったとしても、コメントで抑制の理由を明確にしておくことで、コードレビューアや他の開発者が解決案を思いつく可能性があります。ApiResult の例では、以下のように "UNCHECKED_CAST" の理由を明記しておくことで、誰かが共変の利用やインスタンス再作成のような解決案を思いつけるかもしれません。

    @Suppress("UNCHECKED_CAST") // To cast `Failed` from `ApiResult<T>` to `<S>` to reuse the instance.    
    fun <S> flatMap(transform: (T) -> ApiResult<S>): ApiResult<S> = when(this) {
        is Success -> transform(value)
        is Failed -> this as ApiResult<S>
    }

一言まとめ

警告はまず原因の解決を試み、抑制が必要なら範囲を限定した上でコメントを書く。

キーワード: warning, suppression, comment

コード品質向上のテクニックの他の記事を読む

コード品質向上のテクニックの記事一覧