公開日:2022.03.17

「その共通化、コゲ臭くない?」

テクログdevelopment

どうも!入社して3年が経とうとしているわいです!

3年間ソフトウェア開発をおこなっていく中で、経験的に身についた嗅覚があります。

コードレビューも、「そのコード後々ヤバくなりそうなので、一応関数に切り出しておいてください」と持ち前の嗅覚でおこなっております。

「なんでですか?」

「うーん、なんかコゲ臭いです」

そう。嗅覚を言語化できていないのです。散々わめくだけで全く理由を説明できません。

そんなことから、私は社内で「火災報知機」と呼ばれています。

凝集度と結合度

そんな私でしたが、WEB+DB PRESS vol.127 を読んで、凝集度と結合度について理解したことで「良いコード」についてそれなりに言語化できました。

今回は凝集度と結合度について、この「火災報知機」が学んだことを少しでもお伝えできればと思います。

すでに知っている方もいらっしゃると思うので詳細は上記参考に譲るとして、私なりに簡単にまとめてみます。

  • 凝集度
    • 関数が機能単位でしっかり分けられているかという度合(関数の役割の少なさ
    • 一般に機能単位であれば関数は細かければ細かいほど凝集度が高くて良い
  • 結合度
    • 関数でデータを変更すると他の処理に意図しない影響を与えやすい度合(関数の独立性
    • グローバル変数を一切使わず、データを引数にして渡したり関数内でローカル変数を定義していると結合度が低くて良い

「1つの関数が担う役割をなるべく少なくするだとか、グローバル変数を使わないだとか、こんなことは改めて言われなくてもわかっている、なにが言語化できただ」と思われた方も多いかと思います。

それでは、次のコードを見たときにあなたはなにを感じますか?

「DRY万歳!共通化してコードがスッキリしたぞ!」

/**
 * チャットアプリケーション
 * ※ 簡単のため、クラスを使わずに関数のみで記述
 */

// テキストメッセージの送信処理
function send_text_message(int $room_id, int $user_id, string $text): bool
{
	return send_message($room_id, $user_id, 'text', $text);
}

// スタンプメッセージの送信処理
function send_stamp_message(int $room_id, int $user_id, int $stamp_id): bool
{
	return send_message($room_id, $user_id, 'stamp', null, $stamp_id);
}

// 共通のメッセージ送信処理
function send_message(int $room_id, int $user_id, string $msg_type, ?string $text=null, ?int $stamp_id=null): bool
{
	// チャットルームに参加しているユーザであるかチェック
	if (!is_valid_user($room_id, $user_id)) return false;

	// $msg_type に応じて、メッセージを作成する
	switch ($msg_type)
	{
		case 'text':
			$msg = create_text_message_object($room_id, $user_id, $text);
			break;
		case 'stamp':
			$msg = create_stamp_message_object($room_id, $user_id, $stamp_id);
			break;
		default:
			return false;
	}

	// DBにメッセージを格納する
	$is_success = store_message_object($msg);
	if (!$is_success) return false;

	// 新規メッセージが届いたことをチャットルーム参加者に通知する
	$is_success = notify_message_object($room_id, $msg);
	return $is_success;
}

よし!これでコードの重複を回避できた!

この例では重複箇所は適切に関数化されているが、仮に数十行にもわたるコードだったとすると、スッキリした感がより強い!!

「その共通化、コゲ臭くない?」

一見問題なさそうである上記コードは、論理的凝集になっています。

論理的凝集とは、関数利用側からフラグを受け取り条件分岐によって処理を切り替えること(send_message()switch ($msg_type) 部分。もちろん if でも同じ)で、極力避けるべき凝集とされています。

論理的凝集の問題点は以下が考えられます。

  • 機能追加のたびに、条件分岐が増える
  • 不要な引数を渡す必要がある
  • 単一責任でない

平たく言うと、スッキリしたと喜んでいる本人以外には読みにくいコードであるということです。

ここで仕様変更があり、「スタンプごとに利用回数を数える」という機能を上記にコードを追加する必要が出たとします。

そうした場合、notify_message_object() をおこなった後に if ($msg_type === 'stamp') { // $stamp_id に応じてカウントアップ処理 } という感じに、テキストメッセージのときには全く必要ない処理を条件分岐とともに追加するしかありません。

こうなってしまうと、仕様変更のたびにますます読みにくくなり、誰も触りたくないコードになってしまいます。

論理的凝集の改善方法

論理的凝集を改善するには、論理的凝集となっている関数を削除してユースケースごとに関数を定義するようにします。

以下が改善後のコードです。

function send_text_message(int $room_id, int $user_id, string $text): bool
{
	if (!is_valid_user($room_id, $user_id)) return false;

	// テキストメッセージ特有の処理
	$msg = create_text_message_object($room_id, $user_id, $text);

	$is_success = store_message_object($msg);
	if (!$is_success) return false;

	$is_success = notify_message_object($room_id, $msg);
	return $is_success;
}

function send_stamp_message(int $room_id, int $user_id, int $stamp_id): bool
{
	if (!is_valid_user($room_id, $user_id)) return false;

	// スタンプメッセージ特有の処理
	$msg = create_stamp_message_object($room_id, $user_id, $stamp_id);

	$is_success = store_message_object($msg);
	if (!$is_success) return false;

	$is_success = notify_message_object($room_id, $msg);

	// $stamp_id に応じて利用回数をカウントアップする処理

	return $is_success;
}

いかがでしょうか?当たり前のように感じる方もいらっしゃるかと思います。

それでは is_valid_user() などの共通で使われている関数が、関数化されておらずそれぞれ数十行にわたるコードだったらどうでしょう?

おそらく、共通化して論理的凝集を選択してしまう人が増えるのではないでしょうか。

このように論理的凝集を選択したくなる状況は多くて、深く注意していないと簡単に論理的凝集になってしまいます。

ここでのポイントは、共通のコードを機能ごとに関数に切り出せているかだと思います。

上記コードのように機能が適切に関数化されていると、論理的凝集を選択したときのうまみ(コードの重複削除)はかなり少なくなります。そうなれば、おのずと論理的凝集を回避できるのではないでしょうか。

さいごに

論理的凝集を学んだことで、なんとなく抱いていた共通化に対するモヤモヤ感というのが私の中で言語化できました。

これからはコードレビューでも理由を説明できそうです。「火災報知器」卒業です。これからは「火災報知器 v2.0」です。

以上、わいでした。

健闘を祈る!!

この記事を書いた人

わい

入社年2019年

出身地大阪

業務内容システム開発

特技または趣味芸人のラジオを聴く、ダイビング

わいの記事一覧へ

テクログに関する記事一覧