コードを短くすると単体テストが楽になる、の実装編

投稿日: 2011/04/11 作成者: masuda

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

コードを短くすると単体テストが楽になる、の証明 | 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以上のカラムを抽出する。
・そのカラムを抽出する

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

カテゴリー: 開発, 設計 パーマリンク

コードを短くすると単体テストが楽になる、の実装編 への2件のフィードバック

  1. masuda のコメント:
    2011/04/12 12:11 am

    実は、リファクタリング後のほう、引数の age を間違っていて後から修正しました。
    説得力がないなぁ、もう。ってな訳で、何故間違いやすいのかという話を明日にでも。
    「自由度」に絡めて、実証という話で。

  2. masuda のコメント:
    2011/04/13 12:26 am

    あ、今気づいたのだけど、dest.Add( r ); でコピーしようとすると、「r は、既にテーブルに含まれています」というエラーがでます。なので、このコードが動かない…のはサンプルとして良くないなぁ。
    ちょっと、リファクタリングのやり直しをしますか。

コメントは停止中です。