Moonmile Solutions Blog

さて、リファクリング前とリファクタリング後のソースコードがありますが、これがどの程度「安全」にコーディングができるのかを検証していきます。
検証するためには、何らかの定量的な手法が必要です。
# ちなみに「定性的な手法」は「ああ、コードが短くなったね」という感想が根拠になりますｗ

コードの複雑度を測定するのに、if 文などの制御文や使われている変数の数を勘定する方法がありますが、ここではもうちょっと簡単に、「変更できる箇所」を数え上げていきます。変更できる箇所というのは、コーディング時に間違えやすいところ/間違える可能性があるところで、≒テストが必要なところ、と言えます。

具体的には

・制御文でカウント
・比較文でカウント
・変数の宣言でカウント
・メソッド呼び出しでカウント
・メソッドの変数でカウント

のように、ちまちまと数えていきます。

で、数え上げたのがこれ。

▼リファクタリング前

▼リファクタリング後

赤い丸の箇所が、変更可能な箇所、いわゆる「自由度」になります。

数を勘定すると、
リファクタリング前は、59箇所
リファクタリング後は、36箇所

ざっと言うと、このコードだと 36/59 の差分で 40% 程度改善されている、といえます。

ちなみに、行数を更に短くした src.Select を使ったパターンを見てみると

▼過剰なリファクタリング？

この赤丸は31箇所あります。リファクタリング後のコードとほぼ同じですね。
一見、行数が短くて簡単になっているように思えますが、自由度としてはほぼ同じなので、どちらも使っても同じ不具合の発生率となる、と言えます。

こんな風に自由度をカウントしていくと、コードの複雑度が定量的に計算できるので、コードの複雑度を低くするようにコーディングをしていけば、不具合の発生率が下がって、作業時間が減り、よってプロジェクトの成功確率は上がる、という流れになるんですけどね。
このあたりは、感覚で勘定しても良いので、数万行のコードの中から複雑すぎるメソッドを抽出する、というようなツールができるといいかなぁとか。

前回の記事
コードを短くすると単体テストが楽になる、の実装編 | Moonmile Solutions Blog
http://www.moonmile.net/blog/archives/2160

で実装上の間違いが発覚したので、やり直し。

■根本的に何をやりたいかを考え直す

のところで、全ての列をコピーするために dest.Rows.Add( r ) なんてことをしていましたが、この変数 r は既に src のテーブルで使っているので Add できないんですよねぇ…というバグが。
ちょっと、動作確認（というか xUnit）していないのがバレバレです。

そんな訳で、

DataTable src ; // 他で設定済み
DataTable dest = new DataTable();
foreach ( DataRow r in src.Rows )
{
	int age = (int)r.Item("age");
	if ( age >= 20 ) {
		DataRow row = dest.NewRow();
		foreach ( DataColumn col in src.Columns ) {
			string name = col.ColumnName;
			row[ name ] = r[ name ];
		}
		dest.Rows.Add( row );
	}
}

のコードが冗長なので、

DataTable src ; // 他で設定済み
DataTable dest = new DataTable();
foreach ( DataRow r in src.Rows )
{
	int age = (int)r.Item("age");
	if ( age >= 20 ) {
		dest.Rows.Add( r );
	}
}

のように「全ての列をコピーしたい」というところから再スタートします。

■根本的に何をやりたいのかを考える。

さて、

		DataRow row = dest.NewRow();
		foreach ( DataColumn col in src.Columns ) {
			string name = col.ColumnName;
			row[ name ] = r[ name ];
		}
		dest.Rows.Add( row );

のところは、全ての列の値をコピーしているところなので、かなり冗長です。
やりたいことは、コピーをとる（あるいは、検索した列だけ抽出したい）ということなので、この部分の意図としては、次のコードなのです。

		dest.Rows.Add( r );

ですが、これを動かそうとすると動きません。実装上の制約ですね。
だからといって、元の冗長な形に残しておくのは適切ではありません。

コードを短くする意図としては「何をしたいか明確にする」ということも含まれているので、そのままのコードでは何をしているのか明確ではないのですよね。

■分かり易い名前でメソッドを作る

コピーしていることが分かるように、メソッドで括り出します。
これは、汎用的にする意味ではなく（結果的に汎用的になるでしょうけど）、意図したことを短く記述するという基準からメソッドを作ります。

private void DataRowCopy( DataRow src, DataRow dest ) 
{
	foreach ( DataColumn col in src.Columns ) {
		string name = col.ColumnName;
		dest[ name ] = src[ name ];
	}
}
...

DataTable src ; // 他で設定済み
DataTable dest = new DataTable();
foreach ( DataRow r in src.Rows )
{
	int age = (int)r.Item("age");
	if ( age >= 20 ) {
		DataRow row = dest.NewRow();
		DataRowCopy( r, row );
		dest.Rows.Add( row );
	}
}

初手としては、DataRow のコピーだけを対象にしておきます。
これでも十分なのですが、前後の dest.NewRow() や dest.Rows.Add( row ) がある文だけ目的のコードよりも長くなっています。

これを目的のコードに近づけるために、DataRowCopy を改良します。

private void AddClone( DataTable dt, DataRow src ) 
{
	DataRow dest = dt.NewRow();
	foreach ( DataColumn col in src.Columns ) {
		string name = col.ColumnName;
		dest[ name ] = src[ name ];
	}
	dt.Rows.Add( dest );
}
...

DataTable src ; // 他で設定済み
DataTable dest = new DataTable();
foreach ( DataRow r in src.Rows )
{
	int age = (int)r.Item("age");
	if ( age >= 20 ) {
		AddClone( dest, r );
	}
}

AddClone というメソッドを作成して、dest.NewRow() などのメインへの記述がなくなるようにします。

		dest.Rows.Add( r );

と似たような記述になったと思います。

■その方法が適切かどうかを考え直す

ここまで DataTable を使ってきましたが、実は DataTable の機能は使っていないのでリストに直すことができます。そうすると、先に AddClone メソッドを作って DataRow のコピーを行いましたが、これがいらなくなります。

DataTable src ; // 他で設定済み
List<DataRow> dest = new List<DataRow>();
foreach ( DataRow r in src.Rows )
{
	int age = (int)r.Item("age");
	if ( age >= 20 ) {
		dest.Add( r );
	}
}

と、ここまで来て最終的なコードは前回と同じです。

// 指定年齢以上のカラムを取り出す
List<DataRow> dest = SelectMoreAge( src, 20 )
...
// 指定年齢以上のカラムを取り出すメソッド
List<DataRow> SelectMoreAge( DataTable src, int age )
{
	List<DataRow> dest = new List<DataRow>();
	foreach ( DataRow r in src.Rows )
	{
		int a = (int)r.Item("age");
		if ( a >= age ) {
			dest.Add( r );
		}
	}
	return dest ;
}

さあ、次はこのコードがどの位、複雑度が減っているか検証していきます。

前回は理論的なところを説明したのですが、では具体的どうするのか？ということで。

コードを短くすると単体テストが楽になる、の証明 | Moonmile Solutions Blog
http://www.moonmile.net/blog/archives/2157

最初に、こんなコードがあるとします。

DataTable src ; // 他で設定済み
DataTable dest = new DataTable();
for ( int i=0; i<src.Rows.Count; i++ ) 
{
	if ( (int)src.Rows[i].Item("age") >= 20 ) {
		DataRow row = dest.NewRow();
		row["id"] = src.Rows[i].Item("id");
		row["age"] = src.Rows[i].Item("age");
		row["name"] = src.Rows[i].Item("name");
		row["address"] = src.Rows[i].Item("address");
		row["telephone"] = src.Rows[i].Item("telephone");
		dest.Rows.Add( row );
	}
}

何をやっているかというと、元のデータテーブル(src)から年齢(age)が20才以上の人を、別のデータテーブル(dest)にコピーしています。
分かり易いといえば分かり易いような（事実、これを分かり易いと主張する方も多いので）、分かりにくいと言えば分かりにくいというような微妙なコードです。

なので、個人的な視点から「わかりやすい」か「わかりにくい」かというのは生産的ではないので、業務の視点や保守の視点から、このコードを変えていきます。
コードをうまく整理すると、コード上の情報が消えて（見えなくなって）、結果的にはコードの質が上がります。

■カウンター変数をやめる

最初は、変数 i で使われているカウンターをやめます。カウンターは配列やリストなどをアクセスするときに便利な手段なのですが、現在 foreach などのループ専用の構文があることを考えると「遺物」と言えます。

DataTable src ; // 他で設定済み
DataTable dest = new DataTable();
foreach ( DataRow r in src.Rows ) 
{
	if ( (int)r.Item("age") >= 20 ) {
		DataRow row = dest.NewRow();
		row["id"] = r.Item("id");
		row["age"] = r.Item("age");
		row["name"] = r.Item("name");
		row["address"] = r.Item("address");
		row["telephone"] = r.Item("telephone");
		dest.Rows.Add( row );
	}
}

変数 i は、Rows コレクションの添え字でしか使われていません。よって、foreach 文で書き換えることができます。これによって、「変数 i が途中で変更される恐れ」が無くなります。また、foreach 文の中では、変数 i による間接的な参照ではなく、変数 r による直接的な参照に切り替えられます。
これは、俗に言うセキュリティ上の変数の「汚染」を防ぐことができます。

■r.Item(“age”) を一時変数に代入する

一時変数の使い方がうまくないコードがあります。

	if ( (int)r.Item("age") >= 20 ) {

この部分は、一見、int にキャストしていて、行数が短くコンパクトになっているように見えますが、保守を考えると、次のように書くほうがよいでしょう。

	int age = (int)r.Item("age");
	if ( age >= 20 ) {

1行のコードが2行になってしまい、冗長なコードに見えますが、実は実行されるときの挙動が異なります。

r.Item(“age”) の中身が int ではない場合、int のキャストに失敗するので、例外は int age = (int)r.Item(“age”); の時点で発生します。元のコードでは if 文内で発生します。
この場合、デバッグ実行をしなければいけないときに、上記のように一時変数に代入しておくと、デバッグがしやすくなります。例えば、どうも 20歳以上とは思えないデータを検出したとき、いわゆる print デバッグ（Console.WriteLine によるデバッグ）をする場合、このように書けます。

	int age = (int)r.Item("age");
	Console.WriteLine("age: {0}", age );
	if ( age >= 20 ) {

ところが、if 文の中で直接キャストしてしまうと、次のように冗長になるし、先の int のキャストを含めるとデバッグ自体をややこしくしてしまいます。

	Console.WriteLine("age: {0}", (int)r.Item("age") );
	if ( (int)r.Item("age") >= 20 ) {

# 実際は、WriteLine メソッドは、object 型の引数を取るので int のキャストを削っても良いのですが、ここでは説明のためにつけています。

DataTable src ; // 他で設定済み
DataTable dest = new DataTable();
foreach ( DataRow r in src.Rows ) 
{
	int age = (int)r.Item("age");
	if ( age >= 20 ) {
		DataRow row = dest.NewRow();
		row["id"] = r.Item("id");
		row["age"] = r.Item("age");
		row["name"] = r.Item("name");
		row["address"] = r.Item("address");
		row["telephone"] = r.Item("telephone");
		dest.Rows.Add( row );
	}
}

■同じ情報を二重に書かないようにする

更に問題なのは、id, age, name, address, telephone などの特定な文字列（マジックナンバー）が含まれていることです。この情報は一体何でしょうか？
おそらく、元のテーブルからコピーしているのですが、本当にそうなのか定かではありません。というのも
id という文字列が2回出てきてしまっているために、

・全て id, age, name… という列名をコピーしたいのか？
・変換しながら id などの列名をコピーしたいのか？

という2つの情報が交じり合っています。
なので、全ての列名をそのままにしてコピーしたい、ということを強調するために次のように書き換えます。

DataTable src ; // 他で設定済み
DataTable dest = new DataTable();
foreach ( DataRow r in src.Rows ) 
{
	int age = (int)r.Item("age");
	if ( age >= 20 ) {
		DataRow row = dest.NewRow();
		string[] names = {
			"id", "age", "name", "address", "telephone" };
		foreach ( string name in names ) {
			row[ name ] = r[ name ];
		}
		dest.Rows.Add( row );
	}
}

こんな風に列名を使った配列を定義して、コピーをします。こうすることで「同じ列名」をコピーすることを強調します。逆に言えば、同じ列名ではない場合があるときは、先のように明示的に列名を書くわけです。

ただし、この場合でも2つの情報が混在しています。
・一部の列をコピーしたいのか？
・全ての列をコピーしたいのか？

一部の場合は、この例でも良いのですが、すべての列の場合はもう一工夫しましょう。列名がコードに現れないようにして情報を減らします。

DataTable src ; // 他で設定済み
DataTable dest = new DataTable();
foreach ( DataRow r in src.Rows ) 
{
	int age = (int)r.Item("age");
	if ( age >= 20 ) {
		DataRow row = dest.NewRow();
		foreach ( DataColumn col in src.Columns ) {
			string name = col.ColumnName;
			row[ name ] = r[ name ];
		}
		dest.Rows.Add( row );
	}
}

こうすると「全ての列をコピーする」という情報が強調され、「一部をコピーする」という情報が消え去ります。

■根本的に何をやりたいかを考え直す

さて、コードから情報を減らすという方法でコードを変えてきました。こうすると、このコードの目的がはっきりしてきます。

・年齢（age）が20以上のカラムを抽出する。
・そのカラムを別のテーブルにコピーする。

とう正確なコードに対する要件です。これの要件は、最初のコードでも満たせていたのかもしれませんが、コードを短くする中で明確になってきています。

そのカラムを別のテーブルにコピーする、という段階で2つの条件が混在しています。

・コピーしたカラムは、修正前と修正後のデータを残さないといけない。
・コピーしたカラムは、修正後のデータだけ残っていればよい。

最初のコードでは、コピーをしているだけなので、この意図が判然としません。
逆に言えば、修正前のデータは残さなくてもよい（あるいは、修正前のデータに反映する必要がある）ときには、次のように元のデータを直接、参照先のテーブルに入れることができます。

DataTable src ; // 他で設定済み
DataTable dest = new DataTable();
foreach ( DataRow r in src.Rows ) 
{
	int age = (int)r.Item("age");
	if ( age >= 20 ) {
		dest.Rows.Add( r );
	}
}

一気にコードが短くなりました。コピー元の情報を取っておく場合には、NewRow メソッドで新しい行を作る必要があるのですが、条件にマッチしたデータだけがほしい場合には、書き方を一変させることで、更に情報を削ることができます。

■その方法が適切かどうかを考え直す。

ここまでは、コピー先も DataTable で用意することにしていましたが、果たして DataTable である必要があるのでしょうか？
条件にマッチした行が必要ならば、List を使ってもよいわけです。
ここでコレクションを利用するのは、DataTable まで高機能ではなくてもよい、というメモリ的な視点からでています。

DataTable src ; // 他で設定済み
List<DataRow> dest = new List<DataRow>();
foreach ( DataRow r in src.Rows ) 
{
	int age = (int)r.Item("age");
	if ( age >= 20 ) {
		dest.Add( r );
	}
}

■機能に適切な名前をつける

ここから、リファクタリングの分野になります。

・年齢（age）が20以上のカラムを抽出する。
・そのカラムを抽出する

という条件が明確になるようにメソッドとして切り出します。

// 指定年齢以上のカラムを取り出す
List<DataRow> dest = SelectMoreAge( src, 20 )

// 指定年齢以上のカラムを取り出すメソッド
List<DataRow> SelectMoreAge( DataTable src, int age )
{
	List<DataRow> dest = new List<DataRow>();
	foreach ( DataRow r in src.Rows ) 
	{
		int a = (int)r.Item("age");
		if ( a >= age ) {
			dest.Add( r );
		}
	}
	return dest ;
}

これで呼び出し側では、たったの1行で済むことになります。
しかも、

・年齢（age）が20以上のカラムを抽出する。
・そのカラムを抽出する

という条件が非常に明確になります。

■メソッド内をリファクタリングする

メソッド内のコードは、xUnit を使えば自由に書き換えることができるので、便利なライブラリがあればそれを使えばよいでしょう。高速化やメモリ効率化などは、この段階で行います。
DataTable の Select メソッドを使うと、非常に小さく書けます。

// 指定年齢以上のカラムを取り出す
List<DataRow> dest = SelectMoreAge( src, 20 )

// 指定年齢以上のカラムを取り出すメソッド
List<DataRow> SelectMoreAge( DataTable src, int age )
{
	List<DataRow> dest = new List<DataRow>();
	dest.AddRange( src.Select(string.Format("age >= {0}", age )));
	return dest ;
}

切り出したメソッドが多少読みにくくなってしまっていますが、ここは xUnut で十分テストできる箇所なので、多少複雑なっても保守性に問題はありません。
分かりづらい場合は、先のメソッドの状態で残しておいても良いと思います。

■どちらのコードが保守性が高いか？

さて、あらためて最初のコードと見比べてみましょう。

変更前がこちら

DataTable src ; // 他で設定済み
DataTable dest = new DataTable();
for ( int i=0; i<src.Rows.Count; i++ ) 
{
	if ( (int)src.Rows[i].Item("age") >= 20 ) {
		DataRow row = dest.NewRow();
		row["id"] = src.Rows[i].Item("id");
		row["age"] = src.Rows[i].Item("age");
		row["name"] = src.Rows[i].Item("name");
		row["address"] = src.Rows[i].Item("address");
		row["telephone"] = src.Rows[i].Item("telephone");
		dest.Rows.Add( row );
	}
}

変更後は１行になります。

// 指定年齢以上のカラムを取り出す
List<DataRow> dest = SelectMoreAge( src, 20 )

メソッド内のコードはこちら。

// 指定年齢以上のカラムを取り出すメソッド
List<DataRow> SelectMoreAge( DataTable src, int age )
{
	List<DataRow> dest = new List<DataRow>();
	foreach ( DataRow r in src.Rows ) 
	{
		int a = (int)r.Item("age");
		if ( a >= age ) {
			dest.Add( r );
		}
	}
	return dest ;
}

メソッドで括りだしているところが「ずるい」ように見えますが、実は業務で使うコードなのにメソッドで括りだしていないことが「危ない」と私は思っています。
ただし、作業量的に（時間的な制約が大きいので）ここまで考慮することはできないことが多いのですが、多少なりとも業務的なコードであれば、

・年齢（age）が20以上のカラムを抽出する。
・そのカラムを抽出する

という要件に絞り込んで、さっくりとメソッドで括りだすことぐらいはしないとね > 自分、といったところで（苦笑）