List<StringBuilder>に大量の文字列データを入れて扱うやりかたは妥当か？

Question

メンテナンスすることになった他人（他社）が作ったC#ソース（.NET Framework 3.5、コンソールアプリ）で気になる構文があったので質問させてください。

• DBから数百、数千行のデータを取り出す。
• 結果を整形しながらテキストファイルに吐き出す。

という、データエクスポート的な処理のロジックで、テキストファイルに吐き出す前の整形データを下記のようなStrinBuilderのListに格納していました。

// 事前にDBから取ったデータをDataTableに格納しています

List<StringBuilder> exportList = new List<StringBuilder>();

foreach (DataRow row in selectedDataTable)
{
    StringBuilder sb = new StringBuilder();
    // 各列の内容を整形しながら繋いでいく
    sb.Append(row["col1"]).ToString());
    sb.Append(row["col2"]).ToString());
                 :
    sb.Append(row["col10"]).ToString());
    // 1行分のデータをStringBuilderごとListに入れる
    exportList.Add(sb);
}
// このあとListの内容をテキストファイルに吐き出す

各列の内容をStringBuilderで繋げて、その結果をstringではなくStringBuilderのままListに追加していっています。上記コードでは省きましたが、最後にまたListをループして、テキストファイルに内容を吐き出して終わりです。列の内容を繋げる部分は実際にはデータの整形（パディングや年月日の書式変更、数値を所定のルールで文字に変換するコード変換など）を行っていますが、上記サンプルでは省いています。

上記のようにListにStringBuilderのインスタンスを大量に突っ込んで使う方法は一般的なのでしょうか？下記2点について情報や御意見をお聞かせください。

• 上記のような用法の問題点
• 逆に、メリットや使い処

自分は、このやりかたはデータが数千件になることを考えると非効率なので、そのままstringをListに突っ込んだ方が断然速いし良いと考え、このコードやプログラム全体の品質について懐疑的になっています。しかし、もしかしたら一般的なやり方で、何か意図や明確なメリットがあるのかもしれない、と思って質問させていただきました。

なお、本件について元の制作者達とは連絡ができず、基本設計レベルの資料よりも深い、詳細設計や開発実装指針、フレームワークの用法、設計意図についてのドキュメント類は皆無という状況です（できあがり品から実装や設計意図を汲み取るしかない……）。

Comments

[m.ts10806]

現状でデータ増やすと目に見えるような問題起きてますか？

[m.ts10806]

ただ、ひとつ念頭に置かなければならないのは、現状で目に見える問題があったとして、すべての影響範囲を網羅した上で対応するほどのメリットが生み出せるかどうかです

[ozwk]

> 自分は、このやりかたはデータが数千件になることを考えると非効率なので

なぜ非効率と考えましたか?

[fana]

> stringをListに突っ込んだ方が…

ファイルへの出力処理が，単に，Listの先頭から順番に吐き出すだけなのであれば，
Listに貯め込まずとも，ループ内処理末尾の exportList.Add(sb);　の箇所で単にファイルに書き出せば良いように思える．

[退会済みユーザー]

何か設計意図があってStringBuilderにしているかどうかは作った人じゃないと判らないので何ともいえません。どうせファイルに書き出すだけなら、そこまで違いはないと思いますが。機能は満たしているんですよね？
ただ、あえてStringBuilder使うなら、Listにせずにむしろ全部一つのStringBuilderに突っ込めばいいのでは感はあります。

[退会済みユーザー]

Listに入れた後になんか加工やら判定やらするのでなければ、私もfanaさんの方法を取りますね。

[Zuishin]

Twitter を見ると、StringBuilder のような大きなオブジェクトをいくつも作っていいのか、のようなことを心配されているようですが、そこは大丈夫です。string を使っても大して変わりません。
それよりも、ため込む必要のないものをリストにため込んでいることの方が私も気になります。

特に困っていないなら改修する必要はありませんが、もしメモリを圧迫してパフォーマンスを上げる必要があるなら、リストを使わず直接出力するよう手を入れていいと思います。関数内で完結していて、他に影響を与えない処理のように見えますから。しかし手を入れる場合はテストを一つ作るべきでしょう。(手を入れない場合も作るのがベターですね)

[backyard]

参考にさせていただきます。ありがとうございます＞みなさん。

パフォーマンスその他については厳密に調べたりしているわけではなく、実際に問題が起きているわけではないのですが、顧客の業務用語で書かれた基本設計書があるだけで、100近くのプログラムがソースとしてあるという状況のため、初期実装者の考え方や指針を汲み取ろうとしたのですが。なかなか……。

非効率、無駄かな？と考えたのが「StringBuilderでなくてもいいのではないか？」という点です。他の方の意見にもあるとおり、StringBuilderの役割がその先もあるのであれば判りますが、単に「文字列を入れる入れ物」として使っているのであれば過剰包装かな、という。
ただ、それが「少々無駄でも今はそういう風にやるものです」みたいなものだったりするならば見習う必要もあるでしょうけれど、そこがちょっと判らなかったので意見を伺いたいと思った次第です。

問題が起きてなければ触らない方がよい、はその通りだと思います。なので、似たような処理をゼロから組む必要があれば、もうすこしシンプルな方法も盛り込んでみるように実装担当の人には指示してみようと思います。

Answer 1

問題点
オーバーヘッドが大きくなって利用するメモリが肥大化する（しっかり確認してみないとわからない）
メリット
加工がしやすい（元の開発者が、このやり方に慣れていた）

個人的な見解
業務で使っているコンソールアプリだと思いますが、鉄則として”気になるから”で治すと痛い目に合います。現状、動いているプログラムで、仕様変更（機能追加）や問題（速度が遅いや、メモリが足りなくなっている）が出ていない限りは、触らない。

一般的なやり方というのでしたら、私は独自クラスを作って配列化します。
初めてやった方法が、多分、その開発者にとっての”一般的”なことで、”普通”になるので、議論の方向としては、違った側面を持ってきたほうがいいと思います。

メモリを減らす方法とか、速度を上げる方法とか、実運用で困りそうなポイントなら議論になると思います。

Answer 2

Stringはimmutableなオブジェクトで、+演算を使うとコピーが発生し、メモリ効率が下がることがあります。(最適化によってそうでない場合もありますが)そのため、StringBuilder自体を使うこと自体はパフォーマンスの改善には繋がる可能性があると思います

Stringのコピーに伴うメモリ消費 > StringBuilderのnewするコスト

ただ、そもそもやりたいこととして、カラムをつないだ文字列をファイルに吐き出すだけっぽいので

StringBuilder sb = new StringBuilder();
foreach (DataRow row in selectedDataTable)
{
    // 各列の内容を整形しながら繋いでいく
    sb.Append(row["col1"]).ToString());
    sb.Append(row["col2"]).ToString());
                 :
    sb.Append(row["col10"]).ToString());
    sb.AppendLine(); // 改行
}

で良くないですか？

Answer 3

一般的ではないと思います。

気になるところ

• StringBuilderを保持するということはexportList[0].Append("aaa");みたいに後から文字を追加できたりしてしまいますが、おそらくexportListはそんな使われ方を想定していないと思います。「DB取得結果を出力する仕様」ではなくなってしまいますし。
• StringBuilderをループ毎にnewしていますが、リストをList<string>に変えてsb.ToString()をリストにつっこめば、StringBuilderは使い回せますよね。データが数千件あるなら数千回newすることになるので、無駄です。
• DataTableの列にデータがあるので、列名/列数は固定のはず。それならこんな感じに書けばListやStringBuilderがいらないですよね。.Net Framework 3.5ならyield return も使えるので、文字列生成を別メソッドに切り出してもいいですし。

using (StreamWriter sw = new StreamWriter(ファイル))
{
  foreach (DataRow row in selectedDataTable)
  {
    sw.Write(row["col1"].ToString());
    sw.Write(row["col2"].ToString());
    ... 以下、全列出力
    sw.Write(row["col10"].ToString());
    sw.WriteLine("");
  }
}

使い所

DBの取得結果に対して、取得処理後にあえて後からデータを追加したいときとか・・・？

Answer 4

質問の趣旨からは外れますが、このまま改修するのは良くないと思いますので指摘します。
別の質問でも似たようなものが最近ありましたが、妥当かどうか、一般的なやり方かどうかというのは
改修する理由にはなりません。
他社が作成したプログラムをメンテナンスすることになったということですが、
契約や見積もりに今回の箇所の改修・テスト工数は含まれていない限りあえて
触れるような内容ではないかと。

改修を加えるということは当然その改修した箇所の動作を保証する為に
テストを行う必要性が発生するということをお忘れなく。

Comments

[backyard]

そうですね。そこは作業担当する人には徹底させたいところです。
どちらかというと、こうしたロジックを書く「意図」が全体から読めず、同じような処理を新規追加する際に我々もこのやり方を踏襲すべきかどうかの判断がよく分からなくなってきたので「良いコードではないけど既存の部分はそっとしておこう……」か「見習ってガンガン使うべき」かを知りたいと思った次第でした。
アドバイス、おっしゃる通りだとおもいますので参考にさせていただきます。

[kaina]

プロフィールを拝見したところ、経験豊富な方で釈迦に説法になったようで恐縮です。
中々プログラマー視点だと分からない部分ではありますので、
上手くコントロールしたいところですね。