ソースの評価？添削？を[雑談かな]

[白]

　タイトルの通りなのですが、前回こちらの掲示板で
cuiのポーカーのコードを載せて、いろいろと助言を
いただき、それをもとに改良してみました。

　本来であれば、学校の先生等に添削してもらえれば
いいのですが、普通の高校に通っているため、
頼めず、周りにはプログラミングできる人もいないため、
自分の書いたコードがどのくらいだめなのか、が分からないので、
ぜひともだめな点を指摘していただきたいです。

　http://www1.axfc.net/uploader/File/so/47616.zip
PASS abab
上のURLから一式ダウンロードできます。


　注:イラストが下手ってことに関しては、練習用なので
許してください。それと、ゲームを本格的に作ろうとしてるのではないので、
Dxlibは使っていません。

　コンパイラ:VC++2008
OS:WinXP SP3

　前回の質問:
http://www.play21.jp/board/formz.cgi?ac ... &rln=51269

[白い時空]

自分が気になった点を、

なぜ、カードのマークにSがふたつあるんでしょうか？
別の文字のほうがいいですね。見分けられないですし。


気になるのは、カード交換のところです。
各桁に交換する数字を指定しているようですが、ポーカーでは基本的に同じカードを２回交換することは無いと思います。

ですから、1と0だけで入力し
「10011」なら、1,2,5桁目が1なので、1,2,5枚目のカードを交換。
「11111」なら、全部の桁が1なので、全部のカードを交換。
「00000」(つまり0)なら、数字が1の桁がないので交換しない。
という感じにしたほうが楽だと思います。


他にちょっと気になったのは、

char *wlhcoubuf = new char[32];
これにわざわざnewを使う意味はあるんですか？

mycards.suu = yamafuda[sumcard].suu;
mycards.mark = yamafuda[sumcard].mark;
同じ構造体なので別に分ける必要はなく、
mycards = yamafuda[sumcard];
でOKです。


プログラムはよく出来てると思います。
ですが、役を判定する関数がプレイヤーとCPUのカードを判定するせいで、関数の長さが2倍になっています。
これを解決できれば、もっと良くなると思います。
objectを渡すのではなく、手札の配列を渡してみたらどうですか。

[白]

Sが二つあるのは単純に間違えていただけです。今、言われて気付きました…

>>ですから、1と0だけで入力し
>>「10011」なら、1,2,5桁目が1なので、1,2,5枚目のカードを交換。
>>「11111」なら、全部の桁が1なので、全部のカードを交換。
>>「00000」(つまり0)なら、数字が1の桁がないので交換しない。

　最初は1と0で考えてみたのですが、どこを交換するかを把握するアルゴリズムが作れず、1~5を使って書きました。よろしければ、その部分のアルゴリズムについて教えていただけますか？

>>char *wlhcoubuf = new char[32];
これにわざわざnewを使う意味はあるんですか？
いえ、多分、プログラムが動かなかったときにいろいろと思考錯誤していた結果newが残っていたのだと思います。

　構造体の代入については前も指摘されたような…学習できていなのか…

　なるほど、判定用の関数に手札の配列を渡せばいいのですか、それなら、CPUとプレイヤー用の二つに分ける必要がなくなりそうです。

　ところで、これの次はレベルアップのためにはどんなプログラムを作ることに挑戦してみればよいのでしょうか？(いろいろなアルゴリズムに触れたりさまざまな関数に触れることができるような)題材があれば押してください。お願いします。

[白い時空]

ちょっと返信が遅れてしまいました。

10進数でやるときは

//入力
delcard=0;
int count=1;
for(i = 0;i < 5;i++){
    if(BST_CHECKED == SendMessage(hSel,BM_GETCHECK,0,0))    //チェックされている状態なら
        delcard += 1*count;
    
    count *= 10;
}
//交換
for(i = 0;i < 5;i++)
    if(del%10==1){
        mycards = yamafuda[sumcard];
        sumcard++;
    }
    del = del / 10;
}

2進数でやるときは

//入力
delcard=0;
for(i = 0;i < 5;i++){
    if(BST_CHECKED == SendMessage(hSel,BM_GETCHECK,0,0))    //チェックされている状態なら
        delcard += 1 << i;
}
//交換
for(i = 0;i < 5;i++)
    if(delcard & 1<<i ){
        mycards = yamafuda[sumcard];
        sumcard++;
    }
}

2進数がわかるなら使ってください。

…とここまで書いて気付いたんですが、

//カード1枚を交換する関数を作成
void ChangeCard(struct card *change){
    *change=yamahuda[sumcard];
    sumcard++;
}

for(i = 0;i < 5;i++){
    if(BST_CHECKED == SendMessage(hSel,BM_GETCHECK,0,0))    //チェックされている状態なら
        ChangeCard(&mycards);    //交換する
}

このほうが明らかに簡単だと思いますが…どうでしょうか。

[たいちう]

ちゃんとGUIに移行して動きますね。お疲れ様です。
しかしGUIにすることで、CUIの欠点が拡大されてしまった面もあるようです。


１．判定部分のobjectによる場合分けが冗長です。
objectの替わりに判定すべき配列(の先頭アドレス)を渡すことで解決できます。
int Cstraight(const card *cards);

２．シャッフルの部分のアルゴリズム
無駄が多く厳密性もありません。
cf. http://ray.sakura.ne.jp/tips/shaffle.html
(rand()自体が厳密でないし、今回の用途では十分でしょうが一応指摘)

３．ソースコードが読みにくい
インデントが揃ってなかったり、消すのがめんどうだったり、、、
他人に見てもらう前に、自分でできることはやりましょう。


指摘できるところはまだまだありそうですが、
主に３．の理由でこれ以上は見ていません。
１．と３．を直した状態で再度あげてもらえれば、また見ます。

[白]

>白い時空さん
　0と1でやるようにするということはCPU用の処理の部分をかなりいじらなければいけなくなり、
すぐにできず、今回のupには間に合いませんでした。

2進数と10進数の2通りも出していただけて、大変参考になります。
すぐに印刷して明日の学校の授業中に精読してみます。

　一つ気になったのですが、2進数で行うメリットとはなにがあるのでしょうか？
いろいろな参考書にも10進数以外の処理は書いてあり、でも、必要性も感じず
全く勉強してこなかったので、なぜ10進数が使える環境で2進数を使うのでしょう？

>たいちうさん
前回も回答していただきありがとうございました。大変参考になりました。
>>1
今直してみました。ただ、const修飾子をつけると、カード交換のときに
カードを変更できなくなるのではないでしょうか？
(今、ググったばかりなのであいまいなのですが…)

>>2
>>無駄が多く厳密性もありません。
厳密性とはどの部分がでしょうか？
一応参考サイトを読ませていただき、変更してみましたが、
考え方として変更した部分は大丈夫でしょうか？
motoとsakiが同じ値にならないようにしたので前回よりはましになっていると思うのですが…

>>3
コメントは残しっぱなしですみません、upするときにしっかりと
消すべきだったのに忘れていました。
　インデントはそろえてやっていたつもりだったのですが、
メモ帳で開いてみるとなぜかずれてました。VC++やTeraPad等で
編集していたので、その時は問題なく表示されていて
今、自分でupしたのを見て異変に気付きました。大変申し訳ありません。
いろいろと修正を試みたのですが、だめだったので、
upロダのほうにcppファイルの状態でupさせていただいたので、
そちらを見てもらえると助かります。

DLURL
http://www1.axfc.net/uploader/File/so/47766

　また、今見直してみて気になたのは
RECT型に値を代入するところです。
これは値の代入だけで4行も使ってしまうのがなんというか、こう、
きれいじゃないと思うのです。RGBマクロ的な、
一行で入力できるようなのはないのでしょうか?必要なら、自分で書くしかないのですか?

　後、この程度の少量のソースだとどのくらいの時間で読み終わりますか？
自分は、他人のソースを読むとき、このくらいの量になると、1日2日じゃ終わらないのですが。。。

[白い時空]

タブ文字と半角文字のインデントが混ざっています。タブ文字の長さはソフトによって変わるのでそれが原因でしょう。

2進数を使う目的ですが、容量が少なくなるからです。
例えば交換する枚数を増やす場合、int型の容量の関係で、10進数では10枚まで交換できますが、2進数では31枚まで交換可能です。
コンピュータは容量が制限されるので、容量は少ないほうがいいのです。

私はwindows APIの部分はほとんど見ていません。
複雑で指摘しづらいですし。
私も昔、windows APIを使っていましたがゲームを作るのには向いておらず、DXライブラリに変えました。
でもwindows APIの経験は必要ですね。

[ISLe]

インデントが揃っていない他人のソースを読むときは、コマンドラインツールで加工してしまいます。
いまはCygwin版のastyleというツールを使っています。便利ですよ。


> 　また、今見直してみて気になたのは
> RECT型に値を代入するところです。

構造体なので
// left, top, right, bottom
RECT rcc = { 20, 210, 440, 240 }; //CPU用
という記述もできます。
いまのままだとエラーになるのでcaseごとにブロック付ける必要あります。
順番間違えるとアレなのでメンバごとに代入するのが良いと思いますけど。
#C99だとメンバ指定できる

WM_PAINTの最後にbreak付いてないのに気付きました。


> 　後、この程度の少量のソースだとどのくらいの時間で読み終わりますか？
> 自分は、他人のソースを読むとき、このくらいの量になると、1日2日じゃ終わらないのですが。。。

移植の仕事がメインなので慣れているのもありますけどこのソースなら詳細まで見て30分以内ですね。
1日あれば5万行くらいのソースでだいたい内容を把握できます。

[ISLe]

描画部分を細かく見てみました。

ビットマップやペンやブラシをSelectObjectしたときに以前にセットされているビットマップやペンやブラシが返るので、覚えておいて、DeleteObjectする前にSelectObjectで元に戻してください。

TextOutで文字数をsizeofで求めてるところがあります。
strlenを使うようにしましょう。
#ちゃんとstrlen使っているところもある。

かわりにDrawText使ってる(とコメントが書かれている)ところはsizeof()-1だとポインタサイズ(この場合は4)-1で3文字しか表示されなかったのでしょう。

case WM_PAINT:の最後にbreak;が無いのでそのままWM_COMMANDの処理に突入してます。
たまたまうまく動いているようです。


あとはTEXTマクロが中途半端に使われているのが気になりますかね。
UNICODEについては意識が回っていないように見えます。

[たいちう]

> 今直してみました。ただ、const 修飾子をつけると、カード交換のときに
> カードを変更できなくなるのではないでしょうか？

変更が必要かどうかで使い分けて下さい。
hantei()も同じ方法が適用できます。
ついでにCpareはスペルミスかな？


> 一応参考サイトを読ませていただき、変更してみましたが、
> 考え方として変更した部分は大丈夫でしょうか？

だめです。もう一度よく読んでください。最後まで。


> upロダのほうにcppファイルの状態でupさせていただいたので、
> そちらを見てもらえると助かります。

comp()は変わってないようですが。


> 一行で入力できるようなのはないのでしょうか?
> 必要なら、自分で書くしかないのですか?

SetRect
http://msdn.microsoft.com/ja-jp/library/cc428714.aspx


> 　後、この程度の少量のソースだとどのくらいの時間で読み終わりますか？
> 自分は、他人のソースを読むとき、このくらいの量になると、1日2日じゃ終わらないのですが。。。

私も丁寧に読んで30～60分位でしょうか。
その時副産物として、私好みに書きなおされたプログラムができていることも多いです。
今回のような質問の場合、問題点を全て探すような事はせず、
特に目立った欠点を指摘して、改善されたらもう少し見てみよう、
というスタンスですので、5分位です。

その他、WndProc()が長過ぎです。メッセージ毎に分割した方が
見通しが良くなるのではないでしょうか。

[白]

>白い時空さん
　2進数にこんな利点があったとは、なるほど、条件によっては、約3倍の量を記憶しておけるということですか。これからは少し2進数にも手を出してみたいと思います。

>ISLeさん
　breakが抜けているとは、何たる不覚、すぐに修正しました。

　一日で5万行、なんとい量…やはり、そのくらいはできるようにならないと社会では通用しませんか。。。少しでも近づけるよう精進します!

sizeofは指定した変数等のメモリサイズを返すもので、strlenは指定した文字列の長さ(\0を除いて)を返すので、sizeof("CPU")というようにしても文字列の長さを求めているわけではないので、TextOutに渡しても正しく表示されないという風に理解して大丈夫ですか？

>>あとはTEXTマクロが中途半端に使われているのが気になりますかね。
>>UNICODEについては意識が回っていないように見えます。
　ここらへんは、win32APIの勉強を始めたばかりでとりあえず、変数の型については少しずつ勉強しているのですが、文字コードはまだまだわからないので、とりあえず、コンパイルが通ればいいと思い理解しないうちに使ってしまいました。すみません。

>たいちうさん
　Cpareは完全にスペルミスです。Cpairでした(笑)

>>hantei()も同じ方法が適用できます。
というのは、hanteiも替えてしまおうと思ったのですが、どうせ、hanteiの引数に
#define CPU 1
#define PLAYER 0
というようにして渡したほうがどちらについて求めているかわかりやすいかな、と思ったので直していませんでした。
　コードの長さは「objectの替わりに判定すべき配列(の先頭アドレス)を渡す」方が、5,6行は短くなったのですが、どうも、見づらくて結局もとに戻してしまいました。。。

>>comp()は変わってないようですが。
comp()ですか？特に指摘されていないと思うのですが。。。それに、qsortを使うためにここら辺は、自分で並べ替えのアルゴリズムを考えられなくて例文をそのまま持ってきてちょっと改良しただけなので、ここらへんを変えるなら、自分でソート用の関数を作ってみようかなと思います。

>>SetRect
なるほど!こんな風なのを探していました。まさに、理想通りのでした。

　わずか5分で指摘できるのですか。そのレベルに到達できるように日々精進していきたいと思います。

　返信が遅れてすみません、コードの修正もしたのですが、とりあえず、まだまだ問題点があることがわかったので、一応解決にしてゆっくりと考えてみたいと思います。
　こちらの掲示板だと「早い、詳しい、親切」と三拍子そろっていて本当に助かりました。本当にありがとうございました。