こんにちは。コミュニケーションアプリ「LINE」のモバイルクライアントを開発している石川です。
この記事は、毎週木曜の定期連載 "Weekly Report" 共有の第 36 回です。 LINEヤフー社内には、高い開発生産性を維持するための Review Committee という活動があります。ここで集まった知見を、Weekly Report と称して毎週社内に共有しており、その一部を本ブログ上でも公開しています。（Weekly Report の詳細については、過去の記事一覧を参照してください）
「トートロジーはトートロジー」はトートロジー
とあるグループウェアを実装していることを仮定しましょう。このグループウェアには、タスクを管理する機能と、ユーザ同士でメッセージを送り合う機能があるとします。また、何かタスクに更新がある時や、新しいメッセージを受信した時は、通知ドット (notificationDot) でお知らせする機能もあるとします。この通知ドットの実装は以下の通りです。ただし、(any の計算コストも含め) このロジックとデータモデルには問題がないとします。
notificationDot.isVisible = channelModels.any(ChannelModel::hasUnreadMessage) ||
directMessageRoomModels.any(DirectMessageRoomModel::hasUnreadMessage) ||
taskListStatus.hasNewlyAssignedTask ||
taskListStatus.hasNewlyReopenedTask ||
taskListStatus.hasUpdateOnAssignedTask
...isVisible の計算ロジックは少し長く、見通しがあまり良くないです。そこで、この計算ロジックを以下のようにプライベート関数 shouldShowNotificationDot として抽出しました。
notificationDot.isVisible = shouldShowNotificationDot(
channelModels,
directMessageRoomModels,
taskListStatus
)
...
}
private fun shouldShowNotificationDot(
channelModels: List<ChannelModel>,
directMessageRoomModels: List<DirectMessageRoomModel>,
taskListStatus: TaskListStatus
): Boolean = channelModels.any(ChannelModel::hasUnreadMessage) ||
directMessageRoomModels.any(DirectMessageRoomModel::hasUnreadMessage) ||
taskListStatus.hasNewlyAssignedTask ||
taskListStatus.hasNewlyReopenedTask ||
taskListStatus.hasUpdateOnAssignedTaskこのようにして、呼び出し元を単純にすることができました。しかし、これは読みやすさの観点で最善とは言いにくいです。より良い方法何かあるでしょうか？
意味のあるフレーズにする
notificationDot.isVisible = shouldShowNotificationDot( の行を自然言語として読み下すと、「notificationDot を表示するべきときに、notificationDot を表示する」となります。これでは、コードを読む人に与える情報は限定的です。
このコードが何をするのかを知りたい場合は、= の時点で読むのを中断しても構いません。notificationDot.isVisible = を見ただけで、「このコードは notificationDot の表示状態を更新する」ということを理解できます。裏を返せば、= の右側 (や継続行) を読みたいということは、「どういう条件のときに notificationDot が表示されるか」を知りたいのだと言えます。しかし、実際に = の右側と継続行を読んでも、「notificationDot の表示状態は channelModels などに依存する」としかわかりません。実際の条件を理解するためには、shouldShowNotificationDot の定義を読む必要があります。しかしこれでは、= の右側と継続行の持つ情報量はあまりないと言えるでしょう。
コードから多くの情報を読み取れるようにする (コードの情報量を増やす) ためにも、抽出したコードに与える名前を少し詳細にする ことは良い選択肢です。以下のコードでは、 notificationDot.isVisible = で読むのを中断しても、「何をするのか」を知る ことができます。一方で、条件の概要を理解する必要があるならば、...isVisible = の後ろを読めば十分で、「未読メッセージか未読タスクがある」ことだと分かります。
notificationDot.isVisible =
hasAnyUnreadMessage(channelModels, directMessageRoomModels) ||
hasAnyUnreadTask(taskListStatus)
}
private fun hasAnyUnreadMessage(
channelModels: List<ChannelModel>,
directMessageRoomModels: List<DirectMessageRoomModel>,
) = channelModels.any(ChannelModel::hasUnreadMessage) ||
directMessageRoomModels.any(DirectMessageRoomModel::hasUnreadMessage)
private fun hasAnyUnreadTask(taskListStatus: TaskListStatus): Boolean =
taskListStatus.hasNewlyAssignedTask ||
taskListStatus.hasNewlyReopenedTask ||
taskListStatus.hasUpdateOnAssignedTaskこの例では右辺値で || の演算を行っていますが、このような計算や操作は必ずしも必須ではありません。単純な変数の代入だったとしても、同様の考え方を適用できます。以下の 2 つの代入を見比べてください。
// having little information
notificationDot.isVisible = shouldShowDot
// having more information
notificationDot.isVisible = hasUnreadMessageもちろん、この考え方は代入だけではなく、様々なケースに応用できます。以下のコードは、条件分岐に適用した例です。
// having little information
if (shouldShowDot) {
notificationDot.show()
}
// having more information
if (hasUnreadMessage) {
notificationDot.show()
}ただし、この「抽出したコードに与える名前を少し詳細にする」という考え方は、常に適用するべきという訳ではありません。API やライブラリを作る場合や、コードをモジュール化・レイヤ分けする場合など、依存先の詳細を隠蔽するべき場合は、「依存元が何をするべきか」に基づいて名前をつけることもあります。他にも、汎用的な UI 要素のイベント処理インターフェース (例: onClicked) など、動作の詳細が決まっていない場合も同様です。この場合、イベント処理のコードとしては、「クリックが発生したときに onClicked を呼び出す」というコードにせざるを得ません。少し詳細な名前を与えるというアイディアは、あくまでも選択肢の 1 つに過ぎないということに留意してください。
一言まとめ
コードを抽出したときは、情報量が増えるような名前を与えることを考慮する。
キー ワード: naming, abstraction, extraction