LINEヤフー Tech Blog

LINEヤフー株式会社のサービスを支える、技術・開発文化を発信しています。

コード品質向上のテクニック:第34回 三十六計分けるに如かず

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

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

三十六計分けるに如かず

メッセージを送受信するサービスの UI レイアウトを実装しているとします。このレイアウト内では、様々な「ボタン」を実装する必要があります (例: メッセージ送信ボタン、プロフィール編集ボタン、...)。以下のコードは、メッセージ送信ボタン SendMessageButton とプロフィール編集ボタン ProfileStatusEditToggleButton の実装です。これらのボタンは、クリックされたときに onClicked が呼ばれます。

class SendMessageButton(
    private val messageTextForm: TextForm,
    private val useCase: MessageServiceUseCase
) : MessageServiceButton() {
    override fun onClicked(button: Button) {
        val message = Message(messageTextForm.text, ...)
        val action = MessageServiceAction.SendMessage(message)
        useCase.executeServiceAction(action)

        messageTextForm.clear()
    }
}

class ProfileStatusEditToggleButton(
    private val statusTextForm: TextForm,
    private val useCase: MessageServiceUseCase
) : MessageServiceButton() {
    override fun onClicked(button: Button) {
        val isEditable = !button.isChecked
        button.isChecked = isEditable 
        
        statusTextForm.isEditable = isEditable
        if (!isEditable) {
            val statusText = statusTextForm.text 
            val action = MessageServiceAction.UpdateProfileStatus(statusText)
            useCase.executeServiceAction(action)
        }
    }
}

実際にボタンがクリックされたとき動作は、MessageServiceAction によって指定され、その値によって MessageServiceUseCase が処理します。このようなクラス構成にしたのは、MessageServiceUseCaseMessageServiceAction のレイヤによってレポジトリを各 Button が直接操作させないことを意図してのことでした。

class MessageServiceUseCase(
    private val messageRepository: MessageRepository,
    private val profileRepository: ProfileRepository,
    ...
) {
    fun executeServiceAction(action: MessageServiceAction) {
        when (action) {
            is MessageServiceAction.SendMessage -> action(messageRepository)
            is MessageServiceAction.UpdateProfileStatus -> action(profileRepository)
            ...
        }
    }

}

sealed interface MessageServiceAction {
    class SendMessage(
        private val message: Message
    ) : MessageServiceAction {
        operator fun invoke(messageRepository: MessageRepository) {
            // ... actual logic
        }
    }

    class UpdateProfileStatus(
        private val statusText: String
    ) : MessageServiceAction {
        operator fun invoke(profileRepository: ProfileRepository) {
            // ... actual logic
        }
    }

    ...
}

しかし、このコードには問題点がいつくかあります。それは何でしょうか。

Strategy not to craft

大きな問題としては以下の 3 つがあります。

  • 条件の統合と再分岐: 各 ButtonMessageServiceAction を指定することで動作を定義しています。その action は一度 MessageServiceUseCase.executeServiceAction で 1 つのフローに集められているのにもかかわらず、その中で再分岐しています。このような構造により、動作の流れが追いにくくなっているのが問題です。例えば、SendMessage.invoke がどのような条件で呼ばれるかを確認するためには、すべての executeServiceAction の呼び出し元のコードを読む必要があります。
  • 不要な sealed class: sealed class はダウンキャストでの網羅性を保証できる便利な言語機能ですが、今回の場合は条件分岐を整理することで、sealed class を使わずに実装できます。他の言語においても、不要な associated value や直和型などは避けるべきでしょう。
  • 不完全なロジックのレイヤ分け: MessageServiceActionButton から Repository を隠蔽する役割を持っているのにもかかわらず、invoke を自由に呼び出せてしまいます。そのため、間違った使い方をしてしまっても、それに気が付きにくいことが問題です。

これらの問題は、不適切な Strategy pattern の使用によって引き起こされています。まず、どのロジックを使うのかが静的に決まる場合は、Strategy pattern は不要 です。今回の場合は、MessageServiceUseCase の関数を単純に分割するだけで十分でしょう。

class MessageServiceUseCase(
    private val messageRepository: MessageRepository,
    private val profileRepository: ProfileRepository,
    ...
) {
    fun sendMessage(message: Message) {
        // ... actual logic
        // Use `messageRepository` here
    }

    fun updateProfileStatus(statusText: String) {
        // ... actual logic
        // Use `profileRepository` here
    }
}

class SendMessageButton(
    ...
) : MessageServiceButton() {
    override fun onClicked(button: Button) {
        val message = Message(messageTextForm.text, ...)
        useCase.sendMessage(message)

        messageTextForm.clear()
    }
}

class ProfileStatusEditToggleButton(
    ...
) : MessageServiceButton() {
    override fun onClicked(button: Button) {
        val isEditable = !button.isChecked
        button.isChecked = isEditable 
        
        statusTextForm.isEditable = isEditable
        if (!isEditable) {
            useCase.updateProfileStatus(statusTextForm.text)
        }
    }
}

仮に Strategy pattern が必要になった場合でも、網羅性を保証する意味では Strategy クラスを sealed class にする必要はありません。Strategy pattern は動的にロジックを決める機構 (動的ディスパッチなど) を使って実装されるため、ロジックを決める分岐は必要ないはずだからです。(もちろん、新たな Strategy を外部で定義できなくするという意味は、sealed class は有効です。)

一言まとめ

どのロジックを使うのかが静的に決まる場合は、Strategy pattern の使用を避ける。

キーワード: strategy pattern, conditional branch, sealed class

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

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