前回の記事
コードを短くすると単体テストが楽になる、の実装編 | 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 ;
}
さあ、次はこのコードがどの位、複雑度が減っているか検証していきます。
