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

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

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

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

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

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

そんな訳で、

1
2
3
4
5
6
7
8
9
10
11
12
13
14
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 );
    }
}

のコードが冗長なので、

1
2
3
4
5
6
7
8
9
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 );
    }
}

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

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

さて、

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

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

1
dest.Rows.Add( r );

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

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

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

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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
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 を改良します。

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
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() などのメインへの記述がなくなるようにします。

1
dest.Rows.Add( r );

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

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

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

1
2
3
4
5
6
7
8
9
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 );
    }
}

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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
// 指定年齢以上のカラムを取り出す
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 ;
}

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

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