ブログトップ 記事一覧 ログイン 無料ブログ開設

Strategic Choice

2014-10-31

[]一度に一つ:領域に分ける

どういうこと?

ロジックが達成しようとしている複数のタスクは、それぞれタスクごとの領域に分割します。

どうして?

一つの意味のあるタスクが、バラバラの領域にあると、コードが読みにくくなります。

ウェブクローリングシステムがあるとします。「UpdateCounts()」は、さまざまな統計値を更新する関数です。Webページをダウンロードしたあとに毎回呼ばれることになっています。

void UpdateCounts(HttpDownload hd) {
	counts["Exit State" ][hd.exit_state()]++; // 例: "SUCCESS" or "FAILURE"
	counts["Http Response"][hd.http_response()]++; // 例: "404 NOT FOUND"
	counts["Content-Type" ][hd.content_type()]++; // 例: "text/html"
}

まだ、「こんなふうになったらいいな」と思っている段階です。

実際のHttpDownload オブジェクトには、上記のようなメソッドはありません。HttpDownload は巨大で複雑なクラスになっていて、ネストしたクラスもたくさん入っています。

必要な値は自分で探さなければなりません。しかも、値が消えてしまっていることもあります。そういう場合は、デフォルト値に「unknown」を使うことにします。これを入れると、すごく汚いコードになってしまいました。

// 警告:このコードを長時間直視しないでください。
void UpdateCounts(HttpDownload hd) {
	// 可能であればExit State を見つける。
	if (!hd.has_event_log() || !hd.event_log().has_exit_state()) {
		counts["Exit State"]["unknown"]++;
	} else {
		string state_str = ExitStateTypeName(hd.event_log().exit_state());
		counts["Exit State"][state_str]++;
	}

	// HTTP ヘッダがなければ、残りの要素に"unknown" を設定する。
	if (!hd.has_http_headers()) {
		counts["Http Response"]["unknown"]++;
		counts["Content-Type"]["unknown"]++;
		return;
	}

	// HTTP レスポンスをログに記録する。なければ"unknown" と記録する。
	HttpHeaders headers = hd.http_headers();
	if (!headers.has_response_code()) {
		counts["Http Response"]["unknown"]++;
	} else {
		string code = StringPrintf("%d", headers.response_code());
		counts["Http Response"][code]++;
	}

	// Content-Type をログに記録する。なければ"unknown" と記録する。
	if (!headers.has_content_type()) {
		counts["Content-Type"]["unknown"]++;
	} else {
		string content_type = ContentTypeMime(headers.content_type());
		counts["Content-Type"][content_type]++;
	}
}

大量のコードです。ロジックもたくさんあります。重複コードもあります。ただ、最大のポイントは、このコードが、「複数のタスクを交互に切り替えている」ことです。タスクには以下のようなものがあります。

  1. キーのデフォルト値に「unknown」を使う。
  2. HttpDownload のメンバがあるかどうかを確認する。
  3. 値を抽出して文字列に変換する。
  4. counts[] を更新する。

これらのタスクが、バラバラの領域にあるので、読みにくいのです。領域に分けておくと、それぞれのタスクが分離され、ある領域にいるときは、他の領域のことは考えなくて済むようになります。

どうすれば?

タスクごとに、別々の領域に分割します。

上述例であれば、以下のようにします。

void UpdateCounts(HttpDownload hd) {
	// タスク:抽出したい値にデフォルト値を設定する。
	string exit_state = "unknown";
	string http_response = "unknown";
	string content_type = "unknown";

	// タスク:HttpDownload から値を1つずつ抽出する。
	if (hd.has_event_log() && hd.event_log().has_exit_state()) {
		exit_state = ExitStateTypeName(hd.event_log().exit_state());
	}
	if (hd.has_http_headers() && hd.http_headers().has_response_code()) {
		http_response = StringPrintf("%d", hd.http_headers().response_code());
	}
	if (hd.has_http_headers() && hd.http_headers().has_content_type()) {
		content_type = ContentTypeMime(hd.http_headers().content_type());
	}

	// タスク:counts[] を更新する。
	counts["Exit State"][exit_state]++;
	counts["Http Response"][http_response]++;
	counts["Content-Type"][content_type]++;
}

結果的に、このコードには、目的を持った3つの領域ができました。

  1. 3つのキーのデフォルト値を定義する。
  2. 可能であれば、キーの値を抽出して文字列に変換する。
  3. キーのcounts[] を更新する。

タスクは4つでしたが、領域は3つです。しかし、これはこれで構いません。最初に挙げたタスクは出発点にすぎないのです。いくつかに分割さえできれば、それでいいのです。

2014-10-30

[]一度に一つ:条件の圧縮

どういうこと?

同じ条件文が分散していたら、一つの条件文にまとめられないか検討します。

どうして?

if文が連続していると、すべての場合分けを注意深く読まなければいけません。

例えば、以下のコードは、国が「USA」と「非USA」の場合分けが、分散してしまっています。

var second_half = "Planet Earth";
if (country) {
	second_half = country;
}
if (state && country === "USA") {
	second_half = state;
}
var first_half = "Middle-of-Nowhere";
if (state && country !== "USA") {
	first_half = state;
}
if (city) {
	first_half = city;
}
if (town) {
	first_half = town;
}
return first_half + ", " + second_half;

どうすれば?

まとめられる条件の処理は一つにまとめて、コードを簡潔にします。

上述例だと、「USA」「非USA」をまとめて別々の処理として書くことができます。

var first_half, second_half;
if (country === "USA") {
	first_half = town || city || "Middle-of-Nowhere";
	second_half = state || "USA";
} else {
	first_half = town || city || state || "Middle-of-Nowhere";
	second_half = country || "Planet Earth";
}
return first_half + ", " + second_half;

これで検査や更新の処理*1が簡潔になりました。if 文が少なくなって、ビジネスロジックを扱うコードも短くなりました。

*1:「a || b || c」は最初に「真」と評価できる値(ここでは定義された空ではない文字列)を取り出すJavaScriptのイディオムです。

2014-10-29

[]一度に一つ:コードのデフラグ

どういうこと?

コードを「デフラグ」して、1つずつタスクを行うようにします。

f:id:asakichy:20140507173331p:image

手順は、以下の通りです。

  1. コードが行っている「タスク」をすべて列挙する。
  2. タスクをできるだけ異なる関数に分割する。少なくとも異なる領域に分割する。

どうして?

一度に複数のことをするコードは理解しにくくなります。

たとえば、オブジェクトを生成して、データをキレイにして、入力をパースして、ビジネスロジックを適用しているようなコードです。これらのコードがすべて絡み合っていると、「タスク」が個別に完結しているコードよりも理解するのが難しいのです。

具体例として、ユーザの所在地を読みやすい文字列(「都市, 国」)に整形するコードを見てみます。「Santa Monica, USA」や「Paris, France」のような感じです。「location_info」ディクショナリに構造化された情報があって、そこからすべてのフィールドの「都市」と「国」を選んで連結する仕組みになっています。

以下は、入出力の例を示したものです。

変数整形後
LocalityName“Santa Monica”“Santa Monica, USA”
SubAdministrativeAreaName“Los Angeles”
AdministrativeAreaName“California”
CountryName“USA”

ここまでは簡単ですが、ややこしいのは、この4つの値のいずれかが(あるいはすべてが)存在しない可能性があることです。以下、対応策をを考えます。

  • 「都市」を選ぶときには、「LocalityName」(市や町)→「SubAdministrativeAreaName」(都市や郡)→「AdministrativeAreaName」(州や地域)の順番で使用可能なものを使う。
  • 以上の3 つすべてが使えない場合は、「都市」に「Middle-of-Nowhere」(何でもない場所)というデフォルト値を設定する。
  • 「国」に「CountryName」(国名)が使えない場合は、「Planet Earth」(地球)というデフォルト値を設定する。

以下の図は、値がないときの対処法の例です。

変数整形後
LocalityName(undefined)“Middle-of-Nowhere, Canada”
SubAdministrativeAreaName(undefined)
AdministrativeAreaName(undefined)
CountryName“Canada”
変数整形後
LocalityName(undefined)“Washington, DC, USA”
SubAdministrativeAreaName“Washington, DC”
AdministrativeAreaName(undefined)
CountryName“USA”

このタスクを実装したコードが以下になります。

var place = location_info["LocalityName"]; // 例: "Santa Monica"
if (!place) {
	place = location_info["SubAdministrativeAreaName"]; // 例: "Los Angeles"
}
if (!place) {
	place = location_info["AdministrativeAreaName"]; // 例: "California"
}
if (!place) {
	place = "Middle-of-Nowhere";
}
if (location_info["CountryName"]) {
	place += ", " + location_info["CountryName"]; // 例: "USA"
} else {
	place += ", Planet Earth";
}
return place;

読みにくいコードですが、これでもちゃんと動きます。ただ、数日後、機能を追加することになったら、話は別です。

例えば、アメリカの場合は、国名ではなく(可能であれば)州名を表示する、と機能が望まれるとします。すると、「Santa Monica, USA」は「Santa Monica,California」のようになります。

先ほどのコードにこの機能を追加したら、いうまでもなく、もっと読みにくいコードになります。

どうすれば?

コードをデフラグして、「一度に1つのタスク」を適用します。

上述例のコードを分析すると、このコードが一度に複数のタスクを行っていることに気づきます。

  1. location_info ディクショナリから値を抽出する。
  2. 「都市」の優先順位を調べる。何も見つからなかったら、デフォルトで「Middle-of-Nowhere」にする。
  3. 「国」を取得する。なければ「Planet Earth」にする。
  4. place を更新する。

これらのタスクを個別に解決するようなコードに書き換えます。

最初のタスク(location_info から値を抽出)は、そのままで問題なさそうです。

var town = location_info["LocalityName"]; // 例: "Santa Monica"
var city = location_info["SubAdministrativeAreaName"]; // 例: "Los Angeles"
var state = location_info["AdministrativeAreaName"]; // 例: "CA"
var country = location_info["CountryName"]; // 例: "USA"

これでlocation_info がなくなって、長くて直感的ではないキーを覚える必要がなくなりました。これからは4つの簡潔な変数を扱えばよくなりました。

次に、戻り値の「後半」を見つけます。

// 先にデフォルト値を設定して、値が見つかったら書き換える。
var second_half = "Planet Earth";
if (country) {
	second_half = country;
}
if (state && country === "USA") {
	second_half = state;
}

同じようにして「前半」を見つけることができます。

var first_half = "Middle-of-Nowhere";
if (state && country !== "USA") {
	first_half = state;
}
if (city) {
	first_half = city;
}
if (town) {
	first_half = town;
}

最後に情報をつなぎ合わせます。

return first_half + ", " + second_half;

この修正後のコードは、元のコードを「デフラグ」したものになっています。

f:id:asakichy:20140507173142p:image

修正後のコードでは、4つのタスクが別々の領域にデフラグされています。

2014-10-28

[]一度に一つ:タスクは小さく

どういうこと?

一度に一つのタスクしか行わないようにします。そのため、タスクはそれぞれ小さくします。

どうして?

一度に複数のことをするコードは理解しにくくなります。

たとえば、ブログに設置する投票用のWidgetがあるとします。ユーザは「アップ(賛成)」と「ダウン(反対)」を投票します。すべての投票を合計したものを「score」とします。

「アップ」は1点で、「ダウン」は-1点です。ユーザの投票には3つの状態があります。これらが「score」に与える影響を見てみます。

元の点投票投票後の点
total score = 5vote = “”total score = 5
total score = 5vote = “Up”total score = 6
total score = 5vote = “Down”total score = 4

ユーザが(投票や更新のために)ボタンをクリックしたら、以下の関数が呼び出されます。

vote_changed(old_vote, new_vote); // 投票は"Up"・"Down"・"" のいずれか

関数の処理では、「old_vote」と「new_vote」のすべての組み合わせを考慮して、scoreを更新している。

var vote_changed = function (old_vote, new_vote) {
	var score = get_score();
	if (new_vote !== old_vote) {
		if (new_vote === 'Up') {
			score += (old_vote === 'Down' ? 2 : 1);
		} else if (new_vote === 'Down') {
			score -= (old_vote === 'Up' ? 2 : 1);
		} else if (new_vote === '') {
			score += (old_vote === 'Up' ? -1 : 1);
		}
	}
	set_score(score);
};

短いコードですが、いろんなことをやっています。難しいところがたくさんあるので、「エラー」「タイポ」「バグ」があっても一目見ただけではわかりません。

このコードはたった1つのこと(スコアを更新すること)をしているように見えて、実際には、一度に以下の2つのタスクを行っています。

  1. 「old_vote」と「new_vote」を数値に「パース」する。
  2. 「score」を更新する。

だから、理解しにくいのです。

どうすれば?

一つ一つのタスクを小さくします。

上述例であれば、2 つのタスクを別々に解決すれば、読みやすいコードになります。以下のコードは、最初のタスク(投票を数値にパースする)を解決したものです。

var vote_value = function (vote) {
	if (vote === 'Up') {
		return +1;
	}
	if (vote === 'Down') {
		return -1;
	}
	return 0;
};

次のコードは、2つめのタスク(スコアの更新)を解決しています。

var vote_changed = function (old_vote, new_vote) {
	var score = get_score();
	score -= vote_value(old_vote); // 古い値を削除する
	score += vote_value(new_vote); // 新しい値を追加する
	set_score(score);
};

新しいコードは、一目見ただけで正しく動きそうです。これが「コードを楽に理解できるようにする」ということです。

2014-10-27

[]無関係の下位の問題の抽出:やりすぎない

どういうこと?

「無関係の下位問題」は積極的に見つけて、抽出します。ただし、やりすぎには注意します。

どうして?

「無関係の下位問題」の抽出をやりすぎると、逆に読みにくいコードになる場合があります。

例えば、グルーコードを抽出した結果のコードがあります。

//グルー部分
def url_safe_encrypt(obj):
	obj_str = json.dumps(obj)
	cipher = Cipher("aes_128_cbc", key=PRIVATE_KEY, init_vector=INIT_VECTOR, op=ENCODE)
	encrypted_bytes = cipher.update(obj_str)
	encrypted_bytes += cipher.final() # 現在の128 ビットブロックをフラッシュする
	return base64.urlsafe_b64encode(encrypted_bytes)
//本質部分
user_info = { "username": "...", "password": "..." }
url = "http://example.com/?user_info=" + url_safe_encrypt(user_info)

このコードを、さらに分割すると、以下のようになります。

user_info = { "username": "...", "password": "..." }
url = "http://example.com/?user_info=" + url_safe_encrypt_obj(user_info)

def url_safe_encrypt_obj(obj):
	obj_str = json.dumps(obj)
	return url_safe_encrypt_str(obj_str)

def url_safe_encrypt_str(data):
	encrypted_bytes = encrypt(data)
	return base64.urlsafe_b64encode(encrypted_bytes)

def encrypt(data):
	cipher = make_cipher()
	encrypted_bytes = cipher.update(data)
	encrypted_bytes += cipher.final() # 残りをフラッシュする
	return encrypted_bytes

def make_cipher():
	return Cipher("aes_128_cbc", key=PRIVATE_KEY, init_vector=INIT_VECTOR, op=ENCODE)

すると、逆に読みにくくなってしまいました。小さな関数を作りすぎたため、あちこちに飛び回る実行パスを追いかけることになるからです。

どうすれば?

「無関係の下位問題」は、積極的に見つけて抽出しますが、度を越さないように注意します。

新しい関数をコードに追加すると、ごくわずかに(でも確実に)読みにくさのコストが発生します。このコストを相殺できるようなものかどうか検討します。

プロジェクトの他の部分から再利用できるのであれば、小さな関数を追加するのも意味のあることかもしれません。しかし、それまでは必要ありません。