前回は理論的なところを説明したのですが、では具体的どうするのか？ということで。
コードを短くすると単体テストが楽になる、の証明 | 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以上のカラムを抽出する。
・そのカラムを抽出する
という要件に絞り込んで、さっくりとメソッドで括りだすことぐらいはしないとね > 自分、といったところで（苦笑）
