複数のクラスにうまく分割できない

Question

連投になってしまってすみません。
どうしても気になることがあり、連投させていただきました。

私は今、Visual Basic 2008 でプログラムの改修作業を行っています。
以前に書かれたソースコードに機能を追加したり、バグを修正したりする作業です。
今は ActiveReports for .NET 3.0J を使用した帳票出力のプログラムの改修作業を行っています。

そのソースコードは、一つのフォームクラス内ですべての処理を行っています。

• フォームに入力された値のチェック
• 入力ミスの部分のスタイルを変え（背景色を目立つ色にする）、ステータスバーにエラーメッセージを表示する
• SQL文の生成、パラメータの値をバインド、クエリの発行、結果の取得
• 結果を帳票出力形式にフォーマット（日付を"yyyy/MM/dd"形式にしたりなど）

このようなソースコードに新たに機能を追加したりしようとしていますが、私はまずこのソースコードをきれいに分割したいと考えています。
（この判断が正しいかどうかも知りたいです…）

次のようなクラスに分割したいと考えています。

• フォーム用クラス
• 入力値チェック用クラス
• Data Access Object…SQL文の作成、結果となるDataTableオブジェクトを返す
• ActiveReports のクラス…帳票出力形式への変換はこのクラスで行う

しかし、うまく分割することができません。
例えば、SQL文を作成するとき、フォーム内のチェックボックスにチェックが入っているかどうかでSQL文が異なります。
このようなとき、チェックボックスが一つか二つ程度ならいいのですが、五つ以上あったりすると、それらすべてを引数としてData Access Objectに渡すのも憚られます。
フォームの状態によって幾通りものSQL文が生成されるとき、どのように設計すれば綺麗に解決するかがわかりません。

「入力値チェック用クラス」というのもうまく分割できません。

どのように設計すれば、この問題をうまく解決できるのでしょうか？何か適用できるデザインパターンがあるのでしょうか。
何か知っている方がいましたら、教えていただけると嬉しいです。

Answer 1

このようなソースコードに新たに機能を追加したりしようとしていますが、私はまずこのソースコードをきれいに分割したいと考えています。
（この判断が正しいかどうかも知りたいです…）

書き捨てのプログラムは別にして、良くも悪くもソースコードは会社の資産です
見た目には無駄に見えても、どこでどう依存しているかわかりません
膨大なリソースがつぎ込まれている秘伝のタレと言っても良いでしょう
大げさかも知れませんが、それに手を入れるということは相当準備をしておいた方がいいです
最低限、テストを書いて十分吟味したうえでやりましょう
特に帳票系は社外に出ていくものもありますので、やらかした時の被害は甚大になる場合がありますので..
うちでも経営陣鳴り物入りの「意識高い系」の人が入ってきてソースをいじりまわして、手に負えなくなり逃げるように出て行ったということを経験しました

このようなとき、チェックボックスが一つか二つ程度ならいいのですが、五つ以上あったりすると、それらすべてを引数としてData Access Objectに渡すのも憚られます。
フォームの状態によって幾通りものSQL文が生成されるとき、どのように設計すれば綺麗に解決するかがわかりません。

こういう処理はメソッドチェーンで処理できるように書くのが綺麗だと個人的には思います

「入力値チェック用クラス」というのもうまく分割できません。

こういうのはメタデータクラスでやらせるとよいかもしれません

個人的にはソースコードに手を入れる時「書いたら負け」、「1ヶ月前の自分は他人」ということをできるだけ頭に置いて仕事するように心がけています
書く前によく吟味して、プラットフォームなりフレームワークなりが持つ機能を使うことができれば、できるだけ書かず済みますし、後から見てもわかりやすいです

Comments

[penguinshunya]

回答ありがとうございます。

やっぱりそうですよね…。誰にも報告せずに会社のコードを自分好みのものに変えるのはさすがにまずいですよね。とりあえず、今日の改修作業では「できるだけコードを書かない」ことを意識しました。一から改修しようと考えていた頃に比べたらずっと楽でした。

しかし、やはり対象のソースコードに手を加えたいという思いは消えません。例えば、「データグリッドビューに重複する入力があれば、そこにフォーカスを当ててエラーメッセージを表示する」という機能を実装しているところがあったのですが、その会社では二重ループを使って実装していました。私なら「リストから重複しない要素を取り除く関数」を作成し、そこに複雑な処理を任せます。フォームクラス内には「フォーカスを当ててエラーメッセージを表示する」部分のみを実装します。そちらのほうがずっとわかりやすいと思うのです。

そんなこんな考えていると、いい案を思いつきました。「こんな関数があれば便利なのに」という関数を自宅で開発しておき、いつか自分が主体となって開発するときに備えておくというものです。これからは、そのようにやっていこうと思います。

回答ありがとうございました！

Answer 2

ソースコードを大幅に改修する場合にはリスクが伴います。
逆にバグが増える可能性もあります。
お客さんの視点からすれば『仕様が一切変わってないのにバグが増えてる』という大変な事態になる可能性があります。
メリット・デメリットをキッチリと認識した状態で、更に上司やお客さんとの話も進めておいた上で考えたほうがいいと思います。

各部分を分断していくには、基本的に入出力(特に出力から)を考えると解りやすくなりますよ。
例えば以下のようになります。

1.DBを実行するには何が必要か？
→SQL文+パラメーターのデータ（裏では接続文字列やトランザクションの管理）
→SQLとSQLパラメーターを持ったクラスを作成し、それを渡す
→コネクションやトランザクションは実行側で責任を持たせる

2.SQL文を作成するには何が必要か？
→チェックボックスの状態やテキストボックスの値が必要
→引数化(bool, string)
→数が多いのでこれをパラメータークラス化

Comments

[penguinshunya]

確かに、手を加えることによってプログラムにバグを混入させてしまっては、お客さんにもSEの方にも迷惑をかけてしまいますね。
これからは、他の人のことも考えないといけないなと思いました。

回答ありがとうございました。

Answer 3

このようなソースコードに新たに機能を追加したりしようとしていますが、私はまずこのソースコードをきれいに分割したいと考えています。
（この判断が正しいかどうかも知りたいです…）

プログラマとしてはごく自然な判断だと思います。
ただ、会社の文化によってはそれを良しとしないところもあります。今後のメンテナンスを効率よく行うために絶対必要だということを上司や同僚に理解してもらえて、なおかつ修正のコストをかけることが許されるかによって、実施できるかどうかが変わってきます。

このようなとき、チェックボックスが一つか二つ程度ならいいのですが、五つ以上あったりすると、それらすべてを引数としてData Access Objectに渡すのも憚られます。
フォームの状態によって幾通りものSQL文が生成されるとき、どのように設計すれば綺麗に解決するかがわかりません。

実際のソースを見ていないので的確かどうかわかりませんが、できるだけData Access Objectをシンプルに作った方が良いのではないかと思います。
チェックボックスなどの状態による分岐処理はフォーム側でさせて、分岐の結果を番号などでData Access Objectに渡す、Data Access Objectはその番号に対応したSQLを実行して結果を返す、というような形です。多少冗長になるでしょうが、後からメンテナンスするのに楽になるのではないかと思います。

Comments

[penguinshunya]

上司に納得してもらえるだけの根拠は持っていません…。もしかすると、ただLINQなどの真新しい技術を使いたいだけなのかもしれません（笑）仕事を甘く見すぎですね。

これからは会社のソースコードを大幅に書き換えるようなことはせず、できるだけ書き換えが少なく済むように心がけます。それと、会社の方針に沿ったソースコードが書けるようにしたいと思います。

Data Access Object についてのアドバイスを頂きありがとうございます。その方法も考えたのですが、できるだけ独自ルールがないように実装したいと考えています。分岐の結果を番号で渡すようにすると、「1のときはこのようなSQL文を作成して、2のときはこのようなSQL文を作成して…」という新たなルールが作られてしまい、あまりよくない気がします。
でもその辺りはコメントで補えばいいのかな？うーん、まだ経験が浅く、どちらがいいかが判断できません…。また考えてみようと思います。

回答ありがとうございました！