Q&A
コメント欄に出ているクイズとは「次の1~nの(なにか)を～の順番に並べなさい」的な問題ですか？
いつもお世話になります。
雑談というか相談です。
３つのFormで、同じような処理をする、13行くらいのmethodがあります。
このmethodには11個の引数を渡していて、それを渡せば、共通のmethodにできます。
public void commonMethod(引数11個){
//それぞれの引数に対する処理。
}
ということです。
これって、共通化してsimpleになったと思えますかね？
引数が11個という時点で、simpleとはいいがたい気がして、別々に実装したほうがいいかなと悩んでます。
引数を減らすためにclassを作るのも大げさだし。
こういうときって、みなさまなら、どうします。
雑談モードでお聞かせください。
クイズはサンプルで、コード例は単なる例です。
この問題は、そういう問題ですね。
他の形式のクイズと共通化しようとしていますか？例えばただ単に名称を答えさせるとか、n択問題とか。
いまはstringの比較ですむもの３例を共通にするかなー、と悩んでいます。
ただ、今日、みなさまと楽しく討論していて、どうも今回はしないほうがよさそう、というところに落ち着きそうです。
回答8件
0
「重複するコードを括り出してはみたが、それを呼び出すコードが同じくらい複雑」現象、と一般化できますか。
一般論でお答えしますと、その現象にぶつかったときは「括り出すべきかどうか」を議論する意味はあまりなくて、もっと全体の処理フローがまずくないか、そちらを見ていく方が正しいアプローチです。
今回の話ですと、13行の処理が11個のパラメータを必要としている時点でデータの凝集度がいまいちで、オブジェクト同士が疎結合になっておらず今後もいろいろな苦しみに遭遇しそうであるなあと感じます。
とは言えあまりプロダクト設計全体の議論もここではできませんからその13行の処理についてもう一回フォーカスしてみますと、ラムダ式を渡す形にすることで引数をシンプルにできたりしませんか？
投稿2015/11/30 03:46
総合スコア5568
はい。なるほど。参考になりました。
全体のフローといっても、今回に関していうと、あまりフローは多くなくて、見直すところがあまりない感じであり、うーむ、、、と。
おっしゃるように、今後も苦しみそうな印象です。
引数にかんしては、いっそformを渡すか、と考えたのですが、そこまでしてmethodをまとめる意味あるのかな、と躊躇気味です。
引数にFormを渡すというのは最悪の設計だと思います。
何でも出来るからと言って全部を渡すと影響範囲が見えなくなります。
本来は限定するために引数で絞るはずなのですから。
え。Formごと渡すのは最悪ですか。
そういわれるとやめたくなりますね。
引数を用意するのは呼び出しを見て『それが必要なもの』と判るためです。
これを共通化のために『Form全部渡す』とすると、中でFormのどの部分を使っているのかわからなくなります。
結局メソッドの中身を全部追っていかないと影響範囲が理解できなくなります。
それくらいなら共通化しないほうが見えるだけまだマシですね。
はい。
見えるだけマシかなと。
他の回答に付いたコメントも読ませていただきました。
「あまりフローは多くなくて、見直すところがあまりない感じ」なんてとんでもない、これはかなり見直すべきコードですとも。
「リーダブルコード」はもう読まれました？ http://www.amazon.co.jp/dp/4873115655 この本です。今回の件に直接役に立つ内容も含まれていますし、どちらにしろ通して読んでおいて損はありません。
えーほんとですか。
精進します。
はい、改善したほうがいい点を端的に申しますと、このコードは
・入力のあったコントロールの識別
・フォームからの値の読み出し
・正解判定
という別々のロジックを一度に記述しています。
知識として言うと、
・Windows Formsというフレームワークの仕組みの知識
・フォームをどう構成したかという知識
・正解判定はどういうロジックで行われるかという知識
というまったく別々の分野の知識が動員されています。これが「凝集度の低い」状態です。
例えばですよ、「このアプリをWebアプリに移植したい。ASP.NETなら同じC#だから簡単でしょ？」と行くかというと、無理なんですよね。ほぼ一から書き直しです。
「少しルールの違うクイズを追加したいんだけど」でもごっそり書き直しです。
じゃあどうしたら移植や拡張に強い、凝集度の高いコードにできるか。ご紹介した本はその一助になるはずです。
0
フォームが異なっても、渡す引数が共通であるなら、シンプルになったんじゃないですか。
フォーム１では１番目から９番目の引数が使われ、フォーム２では１番目から５番目と８番目から１１番目の引数が使われ、フォーム３では３番目から９番目の引数が使われる、というように「どのフォームに使うのかを意識して引数を決めなければならない」メソッドであるなら、混乱の種を撒いただけだと思います。
質問に書かれている『同じような処理』は、何が同じで何が異なっているのか次第じゃないですか。
それを、きれいに整理できないのであれば、１つのメソッドにまとめるのは誤りです。
きれいに整理できないという事は、動作の把握が困難なきたないコードを書かざるを得ないという事ですから。
投稿2015/11/30 03:05
総合スコア6915
なるほど。参考になりました。
引数が多いので、いっそFormそれじたいをわたしてしまうか、などと考えていました。
しかし、結局、おっしゃるように、わたしたもののどれを使うかは、別々に条件を立てて考えなければならないところがあり、混乱するな、とも思い、躊躇してもいます。
内部的に使用するものがケースによって違うのなら、まずそれぞれを分断してみてはどうでしょうか？
ケースバイケースのようなコードが多数入ってる時点で共通化できてるとは言い難いと思います。
そうですね。
おおまかなコードを載せました。
ただ、問題によって回答数が変わったり、いろいろ違うこともあるので、どうしたものかなと。
0
「リファクタリング」によれば
既知のオブジェクトに問い合わせることができる場合:「メソッドによる引数の置き換え」
複数の引数が1つのオブジェクトにまとまっていれば:「オブジェクトそのものの受け渡し」
引数をオブジェクトにまとめる:「引数オブジェクトの導入」
コメント欄や追加修正欄でのやりとりを踏まえてですが、
とりあえず画面ありきでプログラミングするのをやめましょう。
(私も昔はそうしていましたし、そう言われてもいまいちよくわかっていませんでしたが。)
画面に何でも実装するのは、「分けて実装するのが基本だが、面倒なので」という上級者向け行為です。
クイズの例で言えば、作りたいのはクイズであって画面ではありません。
クイズの出題/回答をする部品(クラス)を作ってから、それを操作する画面を作りましょう。
番号を順番に並べるクイズの例であれば、順番があっているかどうか判定できればいいのですから、
引数11個も必要なく、そもそもstring1つ(か何か)で済む話です。
これを画面ありきで作ると、
・解答欄がある
・何個あるかは解らない
・じゃあ複数ある場合で共通化しよう
・引数が11個になった
という謎の構造になります。
まずすべきことは問題の出題形式のグループ化です。
(順序を答える、単一の名称を答える、n択問題、可能なかぎり列挙する、などなど)
こういう状態を指して、「もっと全体の処理フローがまずくないか検討した方がいい」
と指摘されていると思います。
例のように、複数のテキストボックスに回答を入力し、複数のチェックボックスで正誤を表示するなら、
画面がテキストボックスの入力状況から回答文字列を生成し、クイズクラスに採点をお願いし、
結果を受け取ってチェックボックスを操作するのが「シンプル」だと思います。
また、現状ですと、例えば3択問題100個つくったら
subformX.csをX=1~100作る気でいるようですが、
クイズクラスを作れば
・3択問題クラス
・3択問題回答用画面クラス
・3択問題を何らかの方法で100個生成するクラス
で済みますね。
投稿2015/11/30 07:29
編集2015/11/30 08:00総合スコア13521
おお。なるほど。
すこし目が覚めました。
おっしゃるとおり、画面ありきで設計するのは微妙だなとは思います。
ただ、言い訳するわけではないといいつつ言い訳ですが、諸事情により画面は既定なのです。
所与で、そこを変更することは、今回はむずかしいです。
わたしは設計に深くは関与しておらず、その画面制御のコアの部分は、画面と連動した外部周辺機器を操作しているので、手を加えるとしても、ミクロすぎて手出ししにくいとか。
仕様がぜんぶ決まっていないとか、納期は決まっているとか、共通化しなくてもとりあえずは動いているからとか、そういう事情です。
設計がよくないなあ、とは思うので、できるところはsimpleに、と思うのです。
現時点では、問題が出そろっていないので、
「まずすべきことは問題の出題形式のグループ化です。 」
を行えないのです。
さらに、採点クラスを作っても、そこの処理は判定だけではすまない、ということもあります。
採点したら結果を表示するとか、他のチームと順番をつけるとか、まあ、それもフローのうちですけれど。
もし集まっtailとかで雑談できれば、もうすこしお話しできますね。
設計のこと、共通にするか、しないか。画面とロジックの分離、クラスなど、みなさまのご意見をお伺いできて、とても参考になり、楽しい時間を過ごせました。
今回は変更なしになりそうですが、また次の機会には活かしていきたいと思います。
ありがとうございました。
> 現時点では、問題が出そろっていないので、
「まずすべきことは問題の出題形式のグループ化です。 」
を行えないのです。
新しいパターンができてもなるべく既存の部分を変更しないために行います。
> 画面制御のコアの部分は、画面と連動した外部周辺機器を操作しているので
> 採点クラスを作っても、そこの処理は判定だけではすまない、ということもあります。
採点したら結果を表示するとか、他のチームと順番をつけるとか
ますます画面とロジック分離した方が見通しよくなるんじゃないかと思います。
ともあれ開発頑張ってください。
ありがとうございます。
DBは使う予定だったのですが、応答速度が３秒とかで、なしになり、csvでもつ、みたいになる話をしてます。
いろいろ流動的な状況です。
わたしも分離したほうがいいとは思うし、classつくるほうがとは思います。
0
こんにちは。
程度はあると思いますが、シンプルにはなったと思いますよ。
アプローチの仕方はケースバイケースですが、Formに実装された処理であればインスタンスメソッドでしょうか。
であれば抽象化した上で同じじゃない部分だけオーバーライドさせる方法もありますし。
静的メソッドであれば引数の渡し方は多少改善は出来るかもしれません。
構造体だったりクラスを引数として渡す。処理の内容によってはフォームインスタンスを渡す場合もあると思いますし、特定のインターフェイスを実装した上で引数として扱う場合もあると思います。
全ては共通化したい処理次第です。
でも、少なくとも複雑化はしてないと思いますよ。（同じ「ような」処理ってのが気になりますが…）
投稿2015/11/30 03:11
総合スコア4791
そうですね。
同じような、というところが、実は仕様がまだ１００％でなく微妙な感じなのです。
現時点ではまとめてもよいけど、あとで引数が増えたりしそうではあり、いっそformをわたすか、などと考えいるところです。
複雑化はしてない、というのは心強いです。
むむ。
ためしに、
共通処理(this);
でformぜんぶを渡せるのかと思ったら、ダメですね。
formぜんぶを渡す方法って、ご存知ですか？
もう少し詳細な記載をして頂かないとわからないですが、それで渡せますよ。
ただし、Sideceresさんもコメントしてらっしゃいますけど、汎用的にするためにフォームインスタンスを渡すのはやめたほうが良いです。渡す場合はフォームの操作に特化したメソッドに限ったほうが良いでしょう。（※例：フォームのタイトルバーを変更する、とか）
おおまかなコードを載せました。
ざっくりいうと、クイズのプログラムで、問題ごとに回答の数が微妙にかわったりすることがあります。
コアを作って、問題のformはそのコアを継承しています。
で正解の判定のところで、入力した回答のtextboxと正解かどうかを表示するcheckbox, それに正解の文字列（１１個）を引数として渡せば、正解の判定は共通処理としてできるのではないか、と考えたわけです。
今ここだけ見たのですが・・・（全体的なコメントはこれから見ます。）
委譲じゃなくて継承にしてる理由ってありますか？
こんにちは。
わたしがあまりプログラムに詳しくなく、移譲を知らないためです。
継承も、今回初めて使いました。
0
実務では他人のソフトをメンテナンスすることが（なぜか）多い自分ですが，引数１１個，というのは失笑モノ，というレベルですね（残念ながら実装されているケースはあります…）．
書き込まれた内容と少しずれますし，今こういうふうに設変かけられるわけじゃないのは承知していますから，ここから先は遊び半分でお読みください．
（ガッツリと問題クラスを実装するほうがメリットが大きい，というのは別として）簡易実装でこんなの考えてみました．
以下の様な構造体
・問題文
・正答（配列１番目）
・誤答（配列２番目以降，たとえば最大５件までで，誤答数は不定，最後は空文字列）
これを構造体配列として問題集を作ります（フォームを表示する前とか，問題集を取り込むタイミングで生成します）．フォームへは構造体の１要素を渡し，設問を出す方のフォームで問題文を表示，解答一覧をランダムで散らす，という実装です．
これは正答数が一つの場合ではありますが，言いたいことは**引数のハンドリングは複雑になればなるほどミスしやすくなり，その結果，意図しない動作を引き起こすことにつながる，**ということです．
問題形式が複数あるんだよ，ということですが，この構造体形式が違うようにできればフォームの呼び出しメソッドをオーバーライドすることによって問題形式の多様化にも柔軟に対応できると思います（フォームのデザインはオーバーライドされている個々のメソッドで必要なパネルを引っ張りだしたりする）．
とりとめのない文章になってしまって申し訳ないのですが，ニュアンスを汲んでいただけると幸いです．
投稿2015/12/01 13:38
退会済みユーザー
総合スコア0
引数１１個は失笑、そういっていただけてよかったです。
結局これはこのままいきで進行してます。
回答がロジックとGUIがセットなので、単にロジックだけ切り出しても、シンプルにならない、というのが最大の理由です。
そうですね，プログラマの格言として「動いているものはいじらない」というのがあります．
今後は自分が実装した時に満足するコーディングができればいいんじゃないでしょうかね？
（今のコーディングを不本意と思っているのはコメントの端々から伝わってきます）
0
どんな状況下でも
(引数11個)
は、却下ですね。
シンプルになったかどうかの評価は、ハッシュやテーブルを使うなどして 引数が ４個程度になってからです。
投稿2015/11/30 21:16
総合スコア22324
おお。やっぱりそうですね。
わたしも引数を数えて、これはないわーと、みなさまのご意見をうかがってみたかったのでした。
ほんと、引数１１個はないわー。
ですね。
ありがとうございます。
結局、現状のままいじらずいこう、というほうに傾きました。
あと、補足的になりますが、今回の場合、クイズの回答ロジックの部分を共通化したいのではなく、回答にまつわるいろいろ（正解の時にピンポン、不正解のときにブブー）みたいな、がめん周りを含めて共通化したい、そこまでしないと共通化の意味（コードが減る）がない、ということがあります。
ロジックとインターフェイスをわける、というのはひとつの考え方で、それにはわたしも共感しますが、今回の場合には残念ながらロジックとインターフェイスが込みというか、ロジックだけならほんの数行、インターフェイスの共通化のほうが重要、という感じです。
このあたりをうまく説明できなかったのですが。
インターフェイスの抽象化は、継承を使ってそれなりに行ったつもりですが、もう１段階上の抽象化を今後は検討します。
0
設計を考えずに判断すると、メソッドに切り出し出来るのならした方が良いと思います。
同じコードはないほうがいいからです。
設計的な視点から見ると、そのコードを共通化するメリット・デメリットを挙げてみたらどうでしょうか？
・これから共通コードが再利用される可能性はどの程度あるのか
・以降、共通コードが変更、または増えそうな見通しはあるか
・『単純に同じ処理してるから』で括ってないか？ 『たまたま同じ処理』ということはないか？
・今は共通だがもしかしたら共通でなくなる可能性はないか
・13行を共通化するとトータルでコードがどれだけ減るのか(逆に引数11個セットするのにコストかかってないか)
引数が多い問題はクラスで渡すほうがいいと思います。
パラメータークラスを作る手間を惜しんでいる方をたまに見かけるのですが、そのクラスを作るメリット・デメリットを挙げてから判断してみては如何でしょうか？
要はそのクラスを作る、保守するコストに見合ったものが得られるかどうかです。
設計的に見たら、引数が多い問題は見なおしてみたほうが良いと思います。
例えば共通メソッドにせず、クラス化して切り出したらどうでしょうか。
この場合、メソッドの引数には本当にその時必要な引数のみを渡すことが出来ることが多いです。
例えば引数にTextBoxを渡してメソッド内でTextプロパティを変更しているような場合、TextBoxそのものは予めクラスに渡しておけますよね。
投稿2015/11/30 04:20
退会済みユーザー
総合スコア0
補足です。
差し支えなければ13行程度のメソッドをお教え頂けませんか？
ある程度推測してアドバイス出来るかもしれません。
メリット、デメリットを列挙するのは、冷静な判断に役立ちますね。
・これから共通コードが再利用される可能性はどの程度あるのか
少なくとも今回のシステム全体で3回程度。ひょっとすると5回程度利用する。
・以降、共通コードが変更、または増えそうな見通しはあるか
変更はありうる。それによりコードの可読性が落ちる可能性あり。
・『単純に同じ処理してるから』で括ってないか？ 『たまたま同じ処理』ということはないか？
機能的に同じ処理なので共通化しやすい、と考えた。
・今は共通だがもしかしたら共通でなくなる可能性はないか
微妙にある。多少は吸収できるが、それによりコードの可読性が落ちる可能性あり。
・13行を共通化するとトータルでコードがどれだけ減るのか(逆に引数11個セットするのにコストかかってないか)
引数をセットするため、コードはほとんど減らない。
formをごそっとわたせば、13*2 or 13*４行減る。
classはformごとわたす方向で。
多少整理しました。
クイズのプログラムです。
共通のform(coreform)にtextboxを４つ,checkboxを４つ配置し、３つのsubformはそれを継承しています。
subformは問題ごとに作っています。
public partial class subform1: coreform {
static string 正解順 = "1234";
static string 回答順 = "";
void 回答入力_TextChanged(object sender, EventArgs e) {
TextBox textbox = sender as TextBox;
if (textbox.Name == "text0") 回答順 += "1";
else if (textbox.Name == "text1") 回答順 += "2";
else if (textbox.Name == "text2") 回答順 += "3";
else if (textbox.Name == "text3") 回答順 += "4";
//正解が合っているかどうか判定する。
if (回答順.Length == 正解順.Length && 回答順 == 正解順) {
//正解。
cb0.Checked = cb1.Checked = cb2.Checked = cb3.Checked = true;
回答順 = "";
}
else if (回答順.Length < 正解順.Length) ;
else 回答順= "";
}
TextBoxのイベントハンドラを共通化しているという認識でいいんでしょうか？
まず、イベントハンドラを共通化するというのは基本的にNGだと思います。
何故ならイベントハンドラとは機能(出力)がまとまっているわけでなく、イベント(入力)がたまたま同じ処理をしているということだからです。
もちろんイベントハンドラを共通化する場合もあります。
例えば同じボタンがいっぱいあって、それら全部同じ事をします、というようなケースです。
今回のケースはForm毎にTextChangedの実装が変わる可能性があります。その時共通化したコードから今度は個別に戻す手間がかかります。
なのでまずTextChangedは個別のFormで書いて、そのイベントハンドラ内に共通のメソッドを呼び出すように変更したらどうでしょうか？
はい。これは現在のコードで、このtextchangedのなかで、
正解判定(textbox1, textbox2, textbox3, ..., checkbox1, checkbox2..., 正解文字列, 回答文字列);
と共通のメソッドを作ろうかと考えたのです。
ただこれだと引数が多すぎて悩んでいるわけです。
ちょっと全体が見えないので合っているか不明ですが、こんなクラスではどうでしょうか？
public class AnswerChecker
{
public Dictionary<int, TextBox> AnswerControls { get; } = new Dictionary<int, TextBox>();
public Dictionary<int, string> AnswerStrings { get; } = new Dictionary<int, string>();
public bool Check()
{
var cs = from item in AnswerControls
order by item.Key
select item;
foreach(KeyValuePair kv in cs)
{
string inputValue = kv.Vaue;
string answer = AnswerStrings[kv.Key];
if (inputValue != answer)
return false;
}
return true;
}
}
これをForm_Loadで初期設定しておきます。
private AnswerChecker checker = new AnswerChecker();
private void Form_Load(object sender, EventArgs e)
{
checker.AnswerControls.Add(1, textBox1);
checker.AnswerControls.Add(2, textBox2);
checker.AnswerControls.Add(3, textBox3);
checker.AnswerStrings.Add(1, "正解1");
checker.AnswerStrings.Add(2, "正解2");
checker.AnswerStrings.Add(3, "正解3");
}
そしてTextChengedで呼び出します。
private void textBox1_TextChanged(object sender, EventArgs e)
{
if (checker.Check() == true)
{
//正解
}
}
こうすると以下のメリットがあります。
・クラス側は一度作成すれば回答順や正解のチェック方法が変わらないかぎり変更しなくてよい。
・回答欄、問題の答えは各Formで定義するため、問題数や答えが変動しても各Formで対処できる。
・イベントハンドラ(UI操作)と問題のチェックという機能の分離がされているため、影響範囲が明確になる
理想を言えばまだ以下の点で改善がいると思いますけど。
・TextBoxに依存している(まだUI依存してる)
→CheckBoxやComboBox対応など必要？
・回答がstring一択しかできない
この辺りは今回ちょっと判らないので割り切りました。
コードサンプルありがとうございます。
クラスはいいですよねー。
最近、クラスを使えるようになってきて、いいなーと思っています。
ただ、初期設定が難点ですね。
今回の場合、クイズの問題ごとに微妙に違っている（しかも仕様が未完成）ということもあり、classとLoadと正解判定とあちこちで設定するのは避けたい、という感じです。
正解判定はクラス内の一箇所でしかしていないですよ。Formでは一切行っていません。
Loadは初期設定(テキストボックスとそれに対応した答えの設定)のみです。
この時に答えを設定しているのですがForm_Load時以外でも何度も問題が差し替わるということでしょうか？
私はForm一つにつき問題が確定しているという認識で作っていました。
(なのでLoad時に一回問題と回答をセットすれば良いのかなと)
もしTextBox数がダイナミックに可変するようなら、新たな問題を設定する時に再度初期化すればいいだけだと思います。
あ。そうではなくて、いま複数のFormがあって、変更のたびに初期設定を行うのは面倒だ、ということです。
もし集まっtailとかで雑談できれば、もうすこしお話しできますね。
・Form毎に設問設定するのが面倒
ということですね。
この辺りはDBなどに設定しておいて勝手に突っ込むとかの実装をすれば初期化コードも共通化出来ると思います。
残念ですが私は集まっtailは不参加なのです。
0
提示されれた情報だけではわかりませんが、ひとつのまとまった機能は1ヶ所で定義してそれを共有するのが王道だと思います。システムの調査を依頼されて、大企業でも消費税率をひとつのfunctionを作って共有しようとしないところはびっくりしました。わたしが調査できたのはごく一部でしたが、たぶん作ったサブシステムか個人毎に作っていたのかも？
投稿2015/11/30 03:32
総合スコア16415
なるほど。
わたしもまとめるのが王道だと思うのです。
とはいえ、という感じで。。。。
あなたの回答
tips
プレビュー
