コード品質向上のテクニック: 第 11 回（関数にたこができる）

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

この記事は、毎週木曜の定期連載 “Weekly Report” 共有の第 11 回です。Weekly Report については、第 1 回の記事を参照してください。

関数にたこができる

あるサービスで、ユーザ同士が「フレンド」になるという機能を実装することを想定します。現在のユーザから見て、別のユーザがフレンドであるかを調べたり、新たにフレンドになるために、以下のような UseCase クラスを使います。

class FriendStateUseCase(
    private val currentUserId: UserId,
    ...
) {
    fun isFriend(otherUserId: UserId): Boolean {
        ...
    }

    fun markAsFriend(otherUserId: UserId) {
        ...
    }
}

呼び出し側のコードは以下のようになります。

if (!friendStateUseCase.isFriend(someUserId)) {
    friendStateUseCase.markAsFriend(someUserId)
}

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

何度も聞かない

呼び出し側のコードの構造を抜き出すと、if (receiver.a()) { receiver.b() } となります。

つまり、「現在のレシーバの状態に応じてレシーバを変更する」というコードになってしまっています。

このように「現在のレシーバの状態に応じてレシーバを変更する」場合、状態を変更する関数自身に確認を行わせる方が良いことが多いです。

friendStateUseCase.markAsFriend のケースでは、あらかじめ !isFriend を満たしていることを期待しているようにも読み取れます。フレンド状態で markAsFriend を呼ぶと例外が発生するなど、もし本当に !isFriend を確認する必要がある場合は、その確認が漏れたときにバグが発生します。これを解決するためには、 !isFriend の確認を markAsFriend の内部で行うと良いでしょう。

fun markAsFriend(userId: UserId) {
    if (isFriend(userId)) {
        return
    }

    // Actual logic to mark `userId` as a "friend".
}

すでにフレンド状態のときに markAsFriend が何もしないのは、動作として自然に見えることが多いです。ただし、「何もしない」ことを特に強調する必要がある場合は、関数の名前を markAsFriendIfNotYet としたり、ドキュメンテーションを書いたりして条件を説明する必要があります。

このように、レシーバに自身の状態を確認させることで、見せる必要のない状態を隠し、見かけの状態遷移を単純化することができます。

戻り値で聞く

以下のコードをリファクタリングすることを考えましょう。

if (!friendStateUseCase.isFriend(someUserId)) {
    friendStateUseCase.markAsFriend(someUserId)
    showEventPopup(Event.NEW_FRIEND) // To notify user the operation was successfully finished.
}

上記のコードを以下のようにリファクタリングするのは 不適当 です。

class FriendStateUseCase(...) {

    fun markAsFriend(
        userId: UserId,
        onSucceeded: () -> Unit
    ) {
        if (isFriend(userId)) {
            return
        }

        ... // Actual logic to mark `userId` as a "friend".
        onSucceeded()
    }
}

// Caller 
friendStateUseCase.markAsFriend(someUserId) {
    showEventPopup(Event.NEW_FRIEND)
}

この FriendStateUseCase.markAsFriend のリファクタリングが不適切な理由は、onSucceeded コールバックによって依存関係の不要な巡回が発生してしまっているためです。さらには、この onSucceeded が同期コールバックなのか非同期コールバックなのかが一見するとわかりにくいという点も問題です。

このような場合は markAsFriend を高階関数にするよりも、戻り値として結果を返すのが良いでしょう。今回の場合は真偽値 (Boolean) で十分です。

/** ... */
fun markAsFriend(userId: UserId): Boolean {
    if (isFriend(userId)) {
        return false
    }

    ... // Actual logic to mark `userId` as a "friend".
    return true
}

// Caller
val isNewlyMarkedAsFriend = friendStateUseCase.markAsFriend(someUserId)
if (isNewlyMarkedAsFriend) {
    showEventPopup(Event.NEW_FRIEND)
}

このようにすることでいつ showEventPopup(Event.NEW_FRIEND) が呼ばれるかが明確になります。

ただし、このように関数の名前で戻り値に言及しない場合は、ドキュメンテーションで戻り値の説明をする必要があります (詳しくは読みやすいコードの書き方 session 3 を参照)。また、必要に応じて呼び出し元に戻り値の確認を強制してください（詳しくは確認したかどうか確認した？の Option 1 を参照）。

一言まとめ: レシーバの状態を確認するコードは、そのレシーバの関数内で実行したほうが良い場合もある。

キーワード: state check logic, responsibility, implicit precondition