« [ブログ紹介] VS 内でNUnitのテストを実行する「Visual Nunit 2010」 | トップページ | [コラム] リファクタリングはどっちへ進むべきだろう? ~ 機械に優しく? 人間に優しく? »

2010年8月22日 (日)

[イベント] TDD 道場 第4回 @わんくま名古屋勉強会#14 ~ リファクタリングやってみよう! Step by Step

2010/8/21 に開催された 「わんくま同盟 名古屋勉強会 #14」 で、 4回目になる TDD 道場をやりました。 今回は、 リファクタリングをテーマにしました。
ペアプロしてくれた方々、 また、 会場の参加者の方々、 どうもありがとうございました。

TDD 道場とは、 TDD をやってみることに主眼を置いたコーディング道場です。
今回、 説明に使ったスライドと開始時点のソースコードはこちらにあります。 ⇒ 勉強会などで使った資料: わんくま勉強会 名古屋#14
お題は、 VC# 2010 Express Edition + NUnit 2.5 で 「CSV 読み書きプログラムのリファクタリング」 をやりました。 詳しくは、 上記のスライドをご覧ください。

実際にやってもらって、 会場のみなさんの様子を見ていて感じたのは、 やはりリファクタリングは知られていない (テストファースト以上に!) ということでした。 広めるのにもっと力を入れなけりゃイカンなぁ、 とは思ったものの、 テストファーストより遥かに説明することが多いし…。

ともあれ、 今回のお題では、 メソッドの戻り値のキャッシングと、 メソッドの括り出しをするだけで、 けっこう見られるコードになります。 その後で、 (知らない人には出来ないので、 ちょっと卑怯かなという気はしますが) LINQ を使って幾つかループを消してしまいます。

お題にしたメソッドは、 CSV ファイルの 1行を 1文字列として読み出して作られた文字列の配列を、 カンマで区切って DataTable に格納するというもの。 CSV の一行をカンマで区切って文字列配列にバラすメソッド Split() は、 別に用意してあります。

とりあえず動くようにテストファーストしたら、 次のようなメソッドが出来ました。 (…という想定です)

public static DataTable ToDataTable(IEnumerable<string> lines)
{
DataTable dt = new DataTable();

// テーブルに、必要な分だけのカラムを用意する
int columnCount = 0;
foreach (string s in lines)
{
IList<string> items = Split(s);
columnCount = Math.Max(columnCount, items.Count);
}
for (int i = 0; i < columnCount; i++)
dt.Columns.Add(new DataColumn());

// 一行をカンマで切り分けて、テーブルの行として追加していく
foreach (string s in lines)
{
DataRow r = dt.NewRow();
r.ItemArray = Split(s).ToArray();
dt.Rows.Add(r);
}

dt.AcceptChanges();
return dt;
}

これをリファクタリングしてみよう、 というわけです。
まず気になることはこのくらいかな?
・ 同形の foreach が 2つある。
・ Split() の呼び出しが 2回ある。

まずは、 Split() の呼び出しを 1回にしてみましょうか。 これは、 会場でやってもらいました。

最初に、 Split() の結果をまとめてキャッシュしておくためのローカル変数を追加します。
List<List<string>> csvLines = new List<List<string>>();

そして、 csvLines にデータを格納するため、 foreach ループをもうひとつ追加します。
foreach (string s in lines)
{
    IList<string> items = Split(s);
    csvLines.Add(items.ToList());
}

ループも減らしたいと思っているのに増やすのか!? …今は、 csvLines を導入しようとしています。 そのために増えたループは、 後からなんとかすればいいのです。 あまり悩まないでください。 (で、 ダメだったら、 ソースコードリポジトリから戻せばいい。)

すると、 foreach (string s in lines) を foreach (List<string> items in csvLines) に変えて、 そのループ内での Split() 呼び出しは無くすことができます。

// 変更前
public static DataTable ToDataTable(IEnumerable<string> lines)
{
DataTable dt = new DataTable();








// テーブルに、必要な分だけのカラムを用意する
int columnCount = 0;

foreach (string s in lines)
{
IList<string> items = Split(s);
columnCount = Math.Max(columnCount, items.Count);
}
for (int i = 0; i < columnCount; i++)
dt.Columns.Add(new DataColumn());

// 一行をカンマで切り分けて、テーブルの行として追加していく

foreach (string s in lines)
{
DataRow r = dt.NewRow();
r.ItemArray = Split(s).ToArray();

dt.Rows.Add(r);
}

dt.AcceptChanges();
return dt;
}
// 変更後
public static DataTable ToDataTable(IEnumerable<string> lines)
{
DataTable dt = new DataTable();

List<List<string>> csvLines = new List<List<string>>();
foreach (string s in lines)
{
IList<string> items = Split(s);
csvLines.Add(items.ToList());
}


// テーブルに、必要な分だけのカラムを用意する
int columnCount = 0;
//foreach (string s in lines)
foreach (List<string> items in csvLines)
{
//IList<string> items = Split(s);
columnCount = Math.Max(columnCount, items.Count);
}
for (int i = 0; i < columnCount; i++)
dt.Columns.Add(new DataColumn());

// 一行をカンマで切り分けて、テーブルの行として追加していく
//foreach (string s in lines)
foreach (List<string> items in csvLines)
{
DataRow r = dt.NewRow();
//r.ItemArray = Split(s).ToArray();
r.ItemArray = items.ToArray();
dt.Rows.Add(r);
}

dt.AcceptChanges();
return dt;
}

ここで、 ユニットテストを流して、 GREEN のままであることを確認します。

次に、 今追加した csvLines を作っている部分を、 メソッドに切り出しましょう。
※ C# 2010 EE でも、 メソッドの抽出リファクタリングの機能は装備されています。

// 変更前
public static DataTable ToDataTable(IEnumerable<string> lines)
{
DataTable dt = new DataTable();

List<List<string>> csvLines = new List<List<string>>();
foreach (string s in lines)
{
IList<string> items = Split(s);
csvLines.Add(items.ToList());
}



// テーブルに、必要な分だけのカラムを用意する
// …以下略
}
// 変更後
public static DataTable ToDataTable(IEnumerable<string> lines)
{
DataTable dt = new DataTable();

//List<List<string>> csvLines = new List<List<string>>();
//foreach (string s in lines)
//{
// IList<string> items = Split(s);
// csvLines.Add(items.ToList());
//}

List<List<string>> csvLines = ToListList(lines);

// テーブルに、必要な分だけのカラムを用意する
// …以下略
}

private static List<List<string>> ToListList(IEnumerable<string> lines)
{
List<List<string>> csvLines = new List<List<string>>();
foreach (string s in lines)
{
IList<string> items = Split(s);
csvLines.Add(items.ToList());
}
return csvLines;
}

再び、 ユニットテストを流して、 GREEN のままであることを確認します。

今度は、 columnCount を求めるためのループ部分を、 メソッドに切り出してみましょう。 ついでに、 dt の宣言位置が前過ぎるので、 少し後ろに下げます。

// 変更前
public static DataTable ToDataTable(IEnumerable<string> lines)
{
DataTable dt = new DataTable();

List<List<string>> csvLines = ToListList(lines);

// テーブルに、必要な分だけのカラムを用意する
int columnCount = 0;
foreach (List<string> items in csvLines)
{
columnCount = Math.Max(columnCount, items.Count);
}


for (int i = 0; i < columnCount; i++)
dt.Columns.Add(new DataColumn());

// 一行をカンマで切り分けて、テーブルの行として追加していく
// …以下略
}

private static List<List<string>> ToListList(IEnumerable<string> lines)
{
// …略…
}
// 変更後
public static DataTable ToDataTable(IEnumerable<string> lines)
{
List<List<string>> csvLines = ToListList(lines);

DataTable dt = new DataTable();


// テーブルに、必要な分だけのカラムを用意する
//int columnCount = 0;
//foreach (List<string> items in csvLines)
//{
// columnCount = Math.Max(columnCount, items.Count);
//}

int columnCount = GetMaxItemsCount(csvLines);
for (int i = 0; i < columnCount; i++)
dt.Columns.Add(new DataColumn());

// 一行をカンマで切り分けて、テーブルの行として追加していく
// …以下略
}

private static List<List<string>> ToListList(IEnumerable<string> lines)
{
// …略…
}

private static int GetMaxItemsCount(List<List<string>> csvLines)
{
int columnCount = 0;
foreach (List<string> items in csvLines)
{
columnCount = Math.Max(columnCount, items.Count);
}
return columnCount;
}

またユニットテストを流して、 GREEN のままであることを確認します。

次はどうしましょうか? columnCount のスコープって小さいですよね。 直後の for ループの終わりまでみたいです。
このコードでは、 末尾まで目で見てもたいしたことはないので、 やる必要は無いのですが、 長いメソッドでローカル変数のスコープを確認するときには、 中括弧をつけてビルドしてみます。

public static DataTable ToDataTable(IEnumerable<string> lines)
{
List<List<string>> csvLines = ToListList(lines);

DataTable dt = new DataTable();

// テーブルに、必要な分だけのカラムを用意する
{
int columnCount = GetMaxItemsCount(csvLines);
for (int i = 0; i < columnCount; i++)
dt.Columns.Add(new DataColumn());
}

// 一行をカンマで切り分けて、テーブルの行として追加していく
// …以下略
}

ビルドOKです!
今、 カッコで括った部分をメソッドに切り出しましょう。 また、 メソッド名 PrepareColumns によって処理の概要が分かるようになりますから、 コメント 「// テーブルに、必要な分だけのカラムを用意する」 は、 もはや不要になりますね。

// 変更前
public static DataTable ToDataTable(IEnumerable<string> lines)
{
List<List<string>> csvLines = ToListList(lines);

DataTable dt = new DataTable();

// テーブルに、必要な分だけのカラムを用意する
{
int columnCount = GetMaxItemsCount(csvLines);
for (int i = 0; i < columnCount; i++)
dt.Columns.Add(new DataColumn());
}



// 一行をカンマで切り分けて、テーブルの行として追加していく
foreach (List<string> items in csvLines)
{
DataRow r = dt.NewRow();
r.ItemArray = items.ToArray();
dt.Rows.Add(r);
}

dt.AcceptChanges();
return dt;
}
// 変更後
public static DataTable ToDataTable(IEnumerable<string> lines)
{
List<List<string>> csvLines = ToListList(lines);

DataTable dt = new DataTable();

//// テーブルに、必要な分だけのカラムを用意する
//{
// int columnCount = GetMaxItemsCount(csvLines);
// for (int i = 0; i < columnCount; i++)
// dt.Columns.Add(new DataColumn());
//}

PrepareColumns(csvLines, dt);

// 一行をカンマで切り分けて、テーブルの行として追加していく
foreach (List<string> items in csvLines)
{
DataRow r = dt.NewRow();
r.ItemArray = items.ToArray();
dt.Rows.Add(r);
}

dt.AcceptChanges();
return dt;
}

private static void PrepareColumns(List<List<string>> csvLines, DataTable dt)
{
int columnCount = GetMaxItemsCount(csvLines);
for (int i = 0; i < columnCount; i++)
dt.Columns.Add(new DataColumn());
}

ここでも、 ユニットテストを流して、 GREEN のままであることを確認します。

さて、 次は…
あれ? dt.Rows.Add(params object[] values) なんていうオーバーロードがあるぞ!?

これを使うと、 テーブルの行として追加していく foreach ループの中身が短くなるよね。

// 変更前
public static DataTable ToDataTable(IEnumerable<string> lines)
{
List<List<string>> csvLines = ToListList(lines);

DataTable dt = new DataTable();
PrepareColumns(csvLines, dt);

// 一行をカンマで切り分けて、テーブルの行として追加していく
foreach (List<string> items in csvLines)
{
DataRow r = dt.NewRow();
r.ItemArray = items.ToArray();
dt.Rows.Add(r);


}

dt.AcceptChanges();
return dt;
}
// 変更後
public static DataTable ToDataTable(IEnumerable<string> lines)
{
List<List<string>> csvLines = ToListList(lines);

DataTable dt = new DataTable();
PrepareColumns(csvLines, dt);

// 一行をカンマで切り分けて、テーブルの行として追加していく
foreach (List<string> items in csvLines)
//{
// DataRow r = dt.NewRow();
//
 r.ItemArray = items.ToArray();
//
 dt.Rows.Add(r);
dt.Rows.Add(items.ToArray());
//}

dt.AcceptChanges();
return dt;
}

またまた、 ユニットテストを流して、 GREEN のままであることを確認します。

これで、 メソッドの切り出しはおしまいかな。
いったんリファクタリング完了、 ということにしましょう。
※ どこまでもリファクタリング出来ますからね、 どこかで妥協して、 まぁこのへんで、 ということにしないと終わりません。

あらためて最初と、 この時点のコードを並べておきます。

// 最初
public static DataTable ToDataTable(IEnumerable<string> lines)
{
DataTable dt = new DataTable();

// テーブルに、必要な分だけのカラムを用意する
int columnCount = 0;
foreach (string s in lines)
{
IList<string> items = Split(s);
columnCount = Math.Max(columnCount, items.Count);
}
for (int i = 0; i < columnCount; i++)
dt.Columns.Add(new DataColumn());

// 一行をカンマで切り分けて、テーブルの行として追加していく
foreach (string s in lines)
{
DataRow r = dt.NewRow();
r.ItemArray = Split(s).ToArray();
dt.Rows.Add(r);
}

dt.AcceptChanges();
return dt;
}
// 変更後
public static DataTable ToDataTable(IEnumerable<string> lines)
{
List<List<string>> csvLines = ToListList(lines);

DataTable dt = new DataTable();
PrepareColumns(csvLines, dt);
foreach (List<string> items in csvLines)
dt.Rows.Add(items.ToArray());

dt.AcceptChanges();
return dt;
}

private static void PrepareColumns(List<List<string>> csvLines, DataTable dt)
{
int columnCount = GetMaxItemsCount(csvLines);
for (int i = 0; i < columnCount; i++)
dt.Columns.Add(new DataColumn());
}

private static int GetMaxItemsCount(List<List<string>> csvLines)
{
int columnCount = 0;
foreach (List<string> items in csvLines)
{
columnCount = Math.Max(columnCount, items.Count);
}
return columnCount;
}

private static List<List<string>> ToListList(IEnumerable<string> lines)
{
List<List<string>> csvLines = new List<List<string>>();
foreach (string s in lines)
{
IList<string> items = Split(s);
csvLines.Add(items.ToList());
}
return csvLines;
}

いかがでしょうか?
元のコードより長くなっています。 理解しやすさはどうでしょう? 読み比べて、 判断してください。

さて、 今の .NET Framework は LINQ が使えます。
それを使って、 さらにリファクタリングしてみましょう。

まずは、 GetMaxItemsCount() メソッド。 コレクションの要素の中から最大値を求めるには、 LINQ の拡張メソッド Max<>() が使えそうです。
※ 自信が無ければ、 あるいは、 やってみて想定外のエラーが出たときには、 GetMaxItemsCount() メソッドのテストを新たに書くようにします。 (ここでは書きません。)

// 変更前
private static int GetMaxItemsCount(List<List<string>> csvLines)
{
int columnCount = 0;
foreach (List<string> items in csvLines)
{
columnCount = Math.Max(columnCount, items.Count);
}
return columnCount;


}
// 変更後
private static int GetMaxItemsCount(List<List<string>> csvLines)
{
//int columnCount = 0;
//foreach (List<string> items in csvLines)
//{
//    columnCount = Math.Max(columnCount, items.Count);
//}
//return columnCount;

return csvLines.Max(items => items.Count());
}

ちょっとドキドキしながら、 ユニットテストを流して、 GREEN のままであることを確認します。 OK です!
※ テストが通らなかったら、 あきらめて元に戻すか、 本腰を据えてユニットテストを書き始めるか、 決断することになります。

GetMaxItemsCount() メソッドの中身は 1行だけになってしまいました。 また、 呼び出しているのは、 一箇所だけです。 そこで、 このメソッドはインライン化することにします。
※ GetMaxItemsCount() メソッドは、 リファクタリングの一環として切り出してきたメソッドです。 このように、 リファクタリングが進むと、 再び元のメソッドの中に戻したりすることもあります。

// 変更前
private static void PrepareColumns(List<List<string>> csvLines, DataTable dt)
{
int columnCount = GetMaxItemsCount(csvLines);

for (int i = 0; i < columnCount; i++)
dt.Columns.Add(new DataColumn());
}

private static int GetMaxItemsCount(List<List<string>> csvLines)
{
return csvLines.Max(items => items.Count());
}
// 変更後
private static void PrepareColumns(List<List<string>> csvLines, DataTable dt)
{
//int columnCount = GetMaxItemsCount(csvLines);
int columnCount = csvLines.Max(items => items.Count());
for (int i = 0; i < columnCount; i++)
dt.Columns.Add(new DataColumn());
}

//private static int GetMaxItemsCount(List<List<string>> csvLines)
//{
// return csvLines.Max(items => items.Count());
//}

ここで、 ユニットテストを流して、 GREEN のままであることを確認します。

次に、 ToListList() メソッドの中を見てみます。 コレクションの中身を取り出して、 加工して、 新たなコレクションに入れています。 これは、 LINQ 拡張メソッドの Select<>() が使えます。 新たなコレクションを csvLines に追加するには、 ループでいちいち Add() しなくても、 まとめて AddRange() すればいいです。

// 変更前
private static List<List<string>> ToListList(IEnumerable<string> lines)
{
List<List<string>> csvLines = new List<List<string>>();
foreach (string s in lines)
{
IList<string> items = Split(s);
csvLines.Add(items.ToList());
}




return csvLines;
}
// 変更後
private static List<List<string>> ToListList(IEnumerable<string> lines)
{
List<List<string>> csvLines = new List<List<string>>();
//foreach (string s in lines)
//{
//    IList<string> items = Split(s);
//    csvLines.Add(items.ToList());
//}

IEnumerable<List<string>> l = lines.Select(s => Split(s).ToList());
csvLines.AddRange(l);


return csvLines;
}

毎度ながら、 ユニットテストを流して、 GREEN のままであることを確認します。

ところで、 IEnumerable<List<string>> は、ToList() で List<List<string>> になります。 csvLines を用意しておいて AddRange() しなくても、 ToList() したものをそのままリターンできます。
また、 変数名 l も直しておきましょう。

// 変更前
private static List<List<string>> ToListList(IEnumerable<string> lines)
{
List<List<string>> csvLines = new List<List<string>>();
IEnumerable<List<string>> l = lines.Select(s => Split(s).ToList());
csvLines.AddRange(l);
return csvLines;

}
// 変更後
private static List<List<string>> ToListList(IEnumerable<string> lines)
{
//List<List<string>> csvLines = new List<List<string>>();
IEnumerable<List<string>> csvCollection = lines.Select(s => Split(s).ToList());
//csvLines.AddRange(l);
//return csvLines;

return csvCollection.ToList();
}

いつものように、 ユニットテストを流して、 GREEN のままであることを確認します。

これで、 LINQ を使ったリファクタリングも、 ひととおりおしまいです。
最後にもうひとつやっておきましょうか…

PrepareColumns() メソッドの第1引数に注目します。 このメソッドが欲しがっているものは List<List<string>> じゃなくて、 用意すべきカラム数です。 第1引数を List<List<string>> から int に変えましょう。 そのためには、 columnCount を求める部分を、呼び出し元に引っ越す必要があります。

// 変更前
public static DataTable ToDataTable(IEnumerable<string> lines)
{
List<List<string>> csvLines = ToListList(lines);

DataTable dt = new DataTable();

PrepareColumns(csvLines, dt);
foreach (List<string> items in csvLines)
dt.Rows.Add(items.ToArray());

dt.AcceptChanges();
return dt;
}

private static void PrepareColumns(List<List<string>> csvLines, DataTable dt)
{
int columnCount = csvLines.Max(items => items.Count());
for (int i = 0; i < columnCount; i++)
dt.Columns.Add(new DataColumn());
}
// 変更後
public static DataTable ToDataTable(IEnumerable<string> lines)
{
List<List<string>> csvLines = ToListList(lines);

DataTable dt = new DataTable();
int columnCount = csvLines.Max(items => items.Count());
PrepareColumns(columnCount, dt);
foreach (List<string> items in csvLines)
dt.Rows.Add(items.ToArray());

dt.AcceptChanges();
return dt;
}

private static void PrepareColumns(int columnCount, DataTable dt)
{
//int columnCount = csvLines.Max(items => items.Count());
for (int i = 0; i < columnCount; i++)
dt.Columns.Add(new DataColumn());
}

それでは、 ユニットテストを流して、 GREEN のままであることを確認して、 リファクタリングを終わりにします。

最初と最後のコードを並べておきます。

// 最初
public static DataTable ToDataTable(IEnumerable<string> lines)
{
DataTable dt = new DataTable();

// テーブルに、必要な分だけのカラムを用意する
int columnCount = 0;
foreach (string s in lines)
{
IList<string> items = Split(s);
columnCount = Math.Max(columnCount, items.Count);
}
for (int i = 0; i < columnCount; i++)
dt.Columns.Add(new DataColumn());

// 一行をカンマで切り分けて、テーブルの行として追加していく
foreach (string s in lines)
{
DataRow r = dt.NewRow();
r.ItemArray = Split(s).ToArray();
dt.Rows.Add(r);
}

dt.AcceptChanges();
return dt;
}
// 最後
public static DataTable ToDataTable(IEnumerable<string> lines)
{
List<List<string>> csvLines = ToListList(lines);

DataTable dt = new DataTable();
int columnCount = csvLines.Max(items => items.Count());
PrepareColumns(columnCount, dt);

foreach (List<string> items in csvLines)
dt.Rows.Add(items.ToArray());

dt.AcceptChanges();
return dt;
}

private static void PrepareColumns(int columnCount, DataTable dt)
{
for (int i = 0; i < columnCount; i++)
dt.Columns.Add(new DataColumn());
}

private static List<List<string>> ToListList(IEnumerable<string> lines)
{
IEnumerable<List<string>> csvCollection = lines.Select(s => Split(s).ToList());
return csvCollection.ToList();
}

|

« [ブログ紹介] VS 内でNUnitのテストを実行する「Visual Nunit 2010」 | トップページ | [コラム] リファクタリングはどっちへ進むべきだろう? ~ 機械に優しく? 人間に優しく? »

*イベント」カテゴリの記事

<NUnit>」カテゴリの記事

コメント

コメントを書く



(ウェブ上には掲載しません)




トラックバック

この記事のトラックバックURL:
http://app.cocolog-nifty.com/t/trackback/209349/49219571

この記事へのトラックバック一覧です: [イベント] TDD 道場 第4回 @わんくま名古屋勉強会#14 ~ リファクタリングやってみよう! Step by Step:

» Twitter Trackbacks []
[続きを読む]

受信: 2010年8月23日 (月) 09時19分

« [ブログ紹介] VS 内でNUnitのテストを実行する「Visual Nunit 2010」 | トップページ | [コラム] リファクタリングはどっちへ進むべきだろう? ~ 機械に優しく? 人間に優しく? »