プログラムの評価依頼

[dig]

いつもお世話になっております。

DirectX, C++で龍神録のサイトを参考にシューティングゲームを作成しました。
どなたかこのプログラムの評価をお願いできますでしょうか？
（動作は確認済みです。ローカル変数・構造体変数は全て小文字、メンバ変数はキャメルケース、その他はパスカルケースです。）

それとC++初級者は脱出できたといえるでしょうか？　こちらもよろしく御願いします。

[softya（ソフト屋)]

ざっと見立て気づいたことだけ。
・メソッド名と動作が一致していない。CalcBackGroundでDrawBackGroundしているなど。
・やたらと引数が多い。なぜ情報を整理できなかったのでしょうか？
・スマートポインタも検討したほうが良いです。
・元の龍神録を引きずりすぎていると思います。マジックナンバーが残ったままとか。
・デザインパターンの適用も検討し見てください。
・GameSequenceCentralとSequenceCentralが似てますね。
細かい所は見れてませんが参考までに。

何処からを初心者脱出というかですが、こういうことに気を付けられたらでしょうか。
「コードの臭い (code smell) - リファクタリングの必要性を示す兆候」
http://www.crystal-creation.com/softwar ... -smell.htm

[Dixq (管理人)]

龍神録の館のコードは良くないと断りを入れた上で書きます。
館で紹介しているコードと同じことを書いているのに指摘されるのは納得がいかないでしょうが・・。

> 構造体.flag = 2;

これはコメントなしでは意味が分かりません。
そもそも「フラグ」という言葉を使うべきではなく、フラグとしての変数にはtrueかfalseの2値以外は入れるべきではないです。
.flag = 2;
がそれ以上の操作を無効にすることを示すなら
.isAvailable = false; //有効ではない
や
.isAlive = false; //生きていない
等にしてはいかがでしょう。
コメントなしで、コードの意味がスーっと分かるような形が理想です。

> RegisterShot
> ShotCalc

文法的な細かなことは気にしなくていいと思いますが、自分の中で決まりを作っておいた方が良いと思います。
上記の場合、形が統一出来ていないですよね。
お勧めは
動詞+名詞(目的語)
でしょうか。
上の二つで言えば上の方ですね。

> 変数名の書き方
> sound_length
> soundHandle

ハイフンでつなぐか、ラクダ型にするか、どちらかに統一してみては。

> .direction = 2;

どちらの方向を代入したのか分からないので、列挙型を使ってはどうでしょう。

コード:

enum eDirection {
    eLeft,
    eUp,
    eRight,
    eDown,
};
...
.direction = eRight;

> //shot_numのショットを登録した敵と主人公との角度を返す。
> double ...::ShotAtan2(...)

角度を取得する関数名なら Get...(); にすべきでしょう。

> switch(){ case 0:... case 1:... case2:...

マジックナンバーが多いと何をしているのか分からなくなるので、これも列挙型等で分かりやすく書くと良いと思います。

> クラス名と重複した関数名

クラス名で既に識別可能な物は改めて関数名に付けなくても良いと思います。
例えば「Enemy::EnemyDamage()」はちょっと冗長ですよね。
基本クラスは自分のことをすると思いますから、他のクラスに対して処理をするような関数名にだけ何か識別できる名前を付けると良いかと思います。

> struct.hの存在

PlayerDataの定義がPlayer.hではなくstruct.hにあるのは少々抵抗があります。
Playerデータを使いたい人がPlayer.hをincludeして使えるようにすべきではないでしょうか。

> 継承が無い

みたところ継承しているクラスがありませんでした。
Enemy,Boss,Playerは共通点が多いと思いますので、親クラスを作ってはどうでしょう。
私が実装する時は添付ファイルのような継承関係を作っています。
is-a, has-aを意識した継承関係を作れるようになり、ポリモーフィズムを学んだ上で、デザインパターンを学ぶと良いと思います。

この辺を学ぶための、C++のバイブルとも言える本があるので、紹介します。

[dig]

ざっと見立て気づいたことだけ。
1.メソッド名と動作が一致していない。CalcBackGroundでDrawBackGroundしているなど。
2.やたらと引数が多い。なぜ情報を整理できなかったのでしょうか？
3.スマートポインタも検討したほうが良いです。
4.元の龍神録を引きずりすぎていると思います。マジックナンバーが残ったままとか。
5.デザインパターンの適用も検討し見てください。
6.GameSequenceCentralとSequenceCentralが似てますね。
細かい所は見れてませんが参考までに。

すばやい対応ありがとうございます。
1.確かに甘いところが多々あります。次回組むプログラムでは気をつけたいと思います。
2.自分でも多いと思いました。Dx○○系のクラスは特にカオスです。ここも次回組むプログラムでは気をつけたいと思います。
3.自分的にはいちいちDeleteしてるシーケンス制御で使ってみようと思っていました。しかし、画面がいつまでたっても切り替わらないなど不具合がおきました。
　そこで質問ですが純粋仮想関数を含む基底クラスをスマートポインタとして扱うことはやってはいけないことだったのでしょうか？
4.はい。確かに残ったままの部分が多いですね。次はもう少し減らせればと思います。次は龍神録から離れます。3DのFPS作成にとりかかります。
5.例えばどのクラスをどのようなパターンでsoftyaさんなら適用しますか？さしつかえなければ簡単にでいいので答えていただけますでしょうか？
6.似ています。機能は同じでnamespaceが違うだけです。やはり統一すべきでしょうか？

何処からを初心者脱出というかですが、こういうことに気を付けられたらでしょうか。
「コードの臭い (code smell) - リファクタリングの必要性を示す兆候」
http://www.crystal-creation.com/softwar ... -smell.htm

まだまだ初級者脱出は遠いということですね。
自分ひとりで組んで完成させても、それがどのレベルのプログラムなのかわからないことが多いので非常に参考になります。

[dig]

龍神録の館のコードは良くないと断りを入れた上で書きます。
館で紹介しているコードと同じことを書いているのに指摘されるのは納得がいかないでしょうが・・。

> 構造体.flag = 2;

これはコメントなしでは意味が分かりません。
そもそも「フラグ」という言葉を使うべきではなく、フラグとしての変数にはtrueかfalseの2値以外は入れるべきではないです。
.flag = 2;
がそれ以上の操作を無効にすることを示すなら
.isAvailable = false; //有効ではない
や
.isAlive = false; //生きていない
等にしてはいかがでしょう。
コメントなしで、コードの意味がスーっと分かるような形が理想です。

その通りですね。フラグならわかりやすいbool型のほうを使うように心がけます。

> RegisterShot
> ShotCalc

文法的な細かなことは気にしなくていいと思いますが、自分の中で決まりを作っておいた方が良いと思います。
上記の場合、形が統一出来ていないですよね。
お勧めは
動詞+名詞(目的語)
でしょうか。
上の二つで言えば上の方ですね。

確かに逆ですね。この辺り見やすくするためにはできる限り統一させるようにします。

> 変数名の書き方
> sound_length
> soundHandle

ハイフンでつなぐか、ラクダ型にするか、どちらかに統一してみては。

> .direction = 2;

どちらの方向を代入したのか分からないので、列挙型を使ってはどうでしょう。

コード:

enum eDirection {
    eLeft,
    eUp,
    eRight,
    eDown,
};
...
.direction = eRight;

確かに列挙型のほうがわかりやすいです。そうするようにしていきます。
変数名はローカルもメンバも区別しないほうがよいのでしょうか？（一応ローカルは全てアンダースコアを含めた小文字で、メンバはラクダ型（キャメルケース）と分けてました）

> //shot_numのショットを登録した敵と主人公との角度を返す。
> double ...::ShotAtan2(...)

角度を取得する関数名なら Get...(); にすべきでしょう。

> switch(){ case 0:... case 1:... case2:...

マジックナンバーが多いと何をしているのか分からなくなるので、これも列挙型等で分かりやすく書くと良いと思います。

おっしゃるとおりです。これは気をつけます。

> クラス名と重複した関数名

クラス名で既に識別可能な物は改めて関数名に付けなくても良いと思います。
例えば「Enemy::EnemyDamage()」はちょっと冗長ですよね。
基本クラスは自分のことをすると思いますから、他のクラスに対して処理をするような関数名にだけ何か識別できる名前を付けると良いかと思います。

私はどちらかというと長い名前を付ける癖がありますね。これもできるだけ改善したいと思います。

> struct.hの存在

PlayerDataの定義がPlayer.hではなくstruct.hにあるのは少々抵抗があります。
Playerデータを使いたい人がPlayer.hをincludeして使えるようにすべきではないでしょうか。

それが自然ですね。Struct.hに全て書くべきではありませんでした。このあたりも今後の課題とします。

> 継承が無い

みたところ継承しているクラスがありませんでした。
Enemy,Boss,Playerは共通点が多いと思いますので、親クラスを作ってはどうでしょう。
私が実装する時は添付ファイルのような継承関係を作っています。
is-a, has-aを意識した継承関係を作れるようになり、ポリモーフィズムを学んだ上で、デザインパターンを学ぶと良いと思います。

この辺を学ぶための、C++のバイブルとも言える本があるので、紹介します。

Enemy,Boss辺りを組んでいて確かに同じようなこと書いてるなとは感じていました。
ここは継承すべきであったと思います。このあたりも次に生かせればと思います。

EffectiveC++は買いました。前回の評価依頼の時に何度か返信に上げられていたので。
ただ血肉とはなっていないようです。この辺りは作っては壊しを繰り返して実践的に覚えていければと思っております。
貴重なご意見ありがとうございました。

[softya（ソフト屋)]

>3.自分的にはいちいちDeleteしてるシーケンス制御で使ってみようと思っていました。しかし、画面がいつまでたっても切り替わらないなど不具合がおきました。
>　そこで質問ですが純粋仮想関数を含む基底クラスをスマートポインタとして扱うことはやってはいけないことだったのでしょうか？

auto_ptrは、純粋仮想関数を含むと問題があったはずです。
shared_ptrとかなら大丈夫だったはず。　まぁ、確認してみてください。

>5.例えばどのクラスをどのようなパターンでsoftyaさんなら適用しますか？さしつかえなければ簡単にでいいので答えていただけますでしょうか？

使うパターンですか？私もC++は独学だけなので仕事で使っている人とは視点が違うと思うのと作る人の趣味、作るゲームの種類で変わると思います。
よく出てきそうなものは、FactoryMethod、シングルトン、stateパターン、Mediatorパターンなど、まぁ使えたり使えなかったりしますが、その試行錯誤も意味がありますよ。
ただ、クラスまみれでゲームを一本丸々組んでいないので余り当てにしないでください。

こちらも参考に。
「デザインパターン習得編」
http://marupeke296.com/DP_main.html

>6.似ています。機能は同じでnamespaceが違うだけです。やはり統一すべきでしょうか？
はい。

[赤鬼]

人様のコードを評価出来るほど熟練した訳では無いですが少しだけ個人的に気になった事を。

テクスチャを動的確保しているのになぜオーディオや、その他多数動的確保しなかったのか気になりました。
また余計なところで必要の無いヘッダのincludeが多々見受けられました。
これではビルドの際時間が余計に掛かるかと思われます。
（ヘッダファイルの解析はそれほど早くありません。また、プリコンパイルも視野に入れて見てはどうでしょうか？）
後は他の方のを参考にしてください。

[dig]

>3.自分的にはいちいちDeleteしてるシーケンス制御で使ってみようと思っていました。しかし、画面がいつまでたっても切り替わらないなど不具合がおきました。
>　そこで質問ですが純粋仮想関数を含む基底クラスをスマートポインタとして扱うことはやってはいけないことだったのでしょうか？

auto_ptrは、純粋仮想関数を含むと問題があったはずです。
shared_ptrとかなら大丈夫だったはず。　まぁ、確認してみてください。

ひとつ質問があります。C++の公式といいますか、信頼できるといいますかそういった日本語関数リファレンスページはあるのでしょうか？
それともVisualStudioなのでmsdnのほうをみるべきなのでしょうか？

>5.例えばどのクラスをどのようなパターンでsoftyaさんなら適用しますか？さしつかえなければ簡単にでいいので答えていただけますでしょうか？

使うパターンですか？私もC++は独学だけなので仕事で使っている人とは視点が違うと思うのと作る人の趣味、作るゲームの種類で変わると思います。
よく出てきそうなものは、FactoryMethod、シングルトン、stateパターン、Mediatorパターンなど、まぁ使えたり使えなかったりしますが、その試行錯誤も意味がありますよ。
ただ、クラスまみれでゲームを一本丸々組んでいないので余り当てにしないでください。

こちらも参考に。
「デザインパターン習得編」
http://marupeke296.com/DP_main.html

デザインパターン本はJavaのものを持っており、サンプルはひととおり組んでみましたが、やはり血肉とはなっていないようです。
このあたりも今後の課題とします。シングルトンは今回一箇所で使いましたが、それ以外の他のパターンも積極的に使っていくようにします。

>6.似ています。機能は同じでnamespaceが違うだけです。やはり統一すべきでしょうか？
はい。

統一ですか。自分にとっては分けたほうがわかりやすかったのですが、客観的に見ると冗長にしか見えないようですね。
このあたりも考えて見ます。

[dig]

人様のコードを評価出来るほど熟練した訳では無いですが少しだけ個人的に気になった事を。

テクスチャを動的確保しているのになぜオーディオや、その他多数動的確保しなかったのか気になりました。
また余計なところで必要の無いヘッダのincludeが多々見受けられました。
これではビルドの際時間が余計に掛かるかと思われます。
（ヘッダファイルの解析はそれほど早くありません。また、プリコンパイルも視野に入れて見てはどうでしょうか？）
後は他の方のを参考にしてください。

いえいえ評価ありがとうございます。コードは人にさらさないといいんだか悪いんだかよく判断できないので...。

動的確保の点ですが、あまりよく覚えていませんが、多分オーディオ関連から先に造ったためだと思います。テクスチャ関連クラスを作るときには
「動的のほうが楽」と判断し、vectorを使ったのではないかと思います。（vectorも失敗でしたが...)ここは統一すべきでした。
DirectX関連クラスはどれも失敗作であったと自分でも思います。使っていて使いづらいと思うところが多々ありました。
ただDirectXに慣れるという面ではよかったと思っています。この辺り次でもう少し改善できればと思っております。
無駄なヘッダincludeも見苦しいので気をつけるようにしたいと思います。またプリコンパイルは使ったことがなかったのでこのあたりも
調べてみようかと思います。

評価ありがとうございました。

[softya（ソフト屋)]

>ひとつ質問があります。C++の公式といいますか、信頼できるといいますかそういった日本語関数リファレンスページはあるのでしょうか？
>それともVisualStudioなのでmsdnのほうをみるべきなのでしょうか？

公式なのはJISとかISOでしょうね。
JIS規格閲覧のみ無料で可。読みやすいかは疑問。
http://www.jisc.go.jp/app/pager?%23jps. ... ISNO=X3014
MSDNも英語しか信頼出来ない。

ただ、C++11はJIS規格がまだのはず。
「C++11 - Wikipedia」
http://ja.wikipedia.org/wiki/C%2B%2B11

公式ではないですが英語だとこんなのもあります。
「cplusplus.com - The C++ Resources Network」
http://www.cplusplus.com/

ライブラリ限定なら日本語のこんなのも。これは個人の人かなぁ。
「cpprefjp - C++ Library Reference」
https://sites.google.com/site/cpprefjp/

[dig]

>ひとつ質問があります。C++の公式といいますか、信頼できるといいますかそういった日本語関数リファレンスページはあるのでしょうか？
>それともVisualStudioなのでmsdnのほうをみるべきなのでしょうか？

公式なのはJISとかISOでしょうね。
JIS規格閲覧のみ無料で可。読みやすいかは疑問。
http://www.jisc.go.jp/app/pager?%23jps. ... ISNO=X3014
MSDNも英語しか信頼出来ない。

ただ、C++11はJIS規格がまだのはず。
「C++11 - Wikipedia」
http://ja.wikipedia.org/wiki/C%2B%2B11

公式ではないですが英語だとこんなのもあります。
「cplusplus.com - The C++ Resources Network」
http://www.cplusplus.com/

ライブラリ限定なら日本語のこんなのも。これは個人の人かなぁ。
「cpprefjp - C++ Library Reference」
https://sites.google.com/site/cpprefjp/

ありがとうございます。早速調べてみようと思います。
様々なサイト紹介ありがとうございました。

[dig]

皆様の貴重なご意見のおかげで様々な改善点が見つかりました。
評価された皆様に御礼申し上げます。
ありがとうございました。

[Dixq (管理人)]

> 変数名はローカルもメンバも区別しないほうがよいのでしょうか？（一応ローカルは全てアンダースコアを含めた小文字で、メンバはラクダ型（キャメルケース）と分けてました）

そういう区別でしたか。
メンバ変数との区別は行って良いと思います。しかしパッと見た限り、私は両者の区別が理解できませんでした。
私が知らないだけかもしれませんが・・。
一応よく見かける方法を２つ紹介します。

①　変数名の先頭にmを付ける

int mHandle;
のように、メンバ変数にはmを付けることでローカル変数や引数と区別します。

②　this->を付ける

メンバ変数は this-> を使って参照します。

①は規格上保証されないから嫌だと言う人もいれば、②は名前が長くなるから嫌だと言う人もいるので、まぁ好みだと思います。

[dig]

そういう区別でしたか。
メンバ変数との区別は行って良いと思います。しかしパッと見た限り、私は両者の区別が理解できませんでした。
私が知らないだけかもしれませんが・・。
一応よく見かける方法を２つ紹介します。

①　変数名の先頭にmを付ける

int mHandle;
のように、メンバ変数にはmを付けることでローカル変数や引数と区別します。

②　this->を付ける

メンバ変数は this-> を使って参照します。

①は規格上保証されないから嫌だと言う人もいれば、②は名前が長くなるから嫌だと言う人もいるので、まぁ好みだと思います。

mやthis->ですか...であれば記述的にはmのほうが楽でハッキリしてて良さそうです。
ただ好みの問題になってくるとなかなか迷ってしまいます。
StyleCopのようなある程度コードスタイルを強制してくれるツールがあればいいのですが...。

どういった記述にするべきか少し考えてみようと思います。
解答ありがとうございました。

[dig]

シーケンス制御でのshared_ptrの不具合の原因がわかりましたので報告しておきます。

>3.自分的にはいちいちDeleteしてるシーケンス制御で使ってみようと思っていました。しかし、画面がいつまでたっても切り替わらないなど不具合がおきました。
>　そこで質問ですが純粋仮想関数を含む基底クラスをスマートポインタとして扱うことはやってはいけないことだったのでしょうか？

auto_ptrは、純粋仮想関数を含むと問題があったはずです。
shared_ptrとかなら大丈夫だったはず。　まぁ、確認してみてください。

softyaさんの言うとおりshared_ptrならば純粋仮想関数を含んでいても大丈夫とのことでした。
不具合の原因はプログラムの流れにありました。

コード:

SmartPointer.reset( new Title() );
...
SmartPointer.reset( new GameOver() );
...

上記のように参照先を切り替えていました。この記述自体には問題ありませんでしたが、
Titleクラスなどのデストラクタに記述したテクスチャ、サウンド開放関数が問題の元凶でした。

コード:

//デストラクタ
Title::~Title( void )
{
	DX::dxCentral.DeleteAllTexture();
	DX::dxCentral.DeleteAllSound();
}

上記のようにデストラクタを書いていましたが、reset( new GameOver() )の際、このデストラクタが呼ばれるよりも先に新しい実体のコンストラクタが呼ばれて
しまい。

新しい実体がコンストラクタで画像等読み込み→参照先のなくなった実体のデストラクタが呼ばれる→新しい実体の画像等までも消される→何も表示されない。

という状態になってしまったというのが不具合の原因でした。

以上です。ありがとうございました。

[ISLe]

auto_ptrは純粋仮想関数を含んでいても問題無いですよ。

ただauto_ptr自身をコピーして使うとき気を付けなければならないだけです。
コピーしてもコピー元が消えるとき参照先のオブジェクトも解体されるのでその後コピー先から参照すると解体済みのオブジェクトにアクセスしてしまいます。
shared_ptrは内部で作成されたコンテナを共有するのでコピーされたshared_ptrがすべて破棄されるまで参照先のオブジェクトは解体されません。

privateなメンバなどで所有権を移す可能性がない場面では余分なコンテナを作ったりしない分シンプルで便利だと思うのですがC++11では非推奨になってしまいました。


ライフサイクルのはっきりしているシーン管理より、リソース管理にこそスマートポインタを活用したほうが良いと思います。

[dig]

auto_ptrは純粋仮想関数を含んでいても問題無いですよ。

ただauto_ptr自身をコピーして使うとき気を付けなければならないだけです。
コピーしてもコピー元が消えるとき参照先のオブジェクトも解体されるのでその後コピー先から参照すると解体済みのオブジェクトにアクセスしてしまいます。
shared_ptrは内部で作成されたコンテナを共有するのでコピーされたshared_ptrがすべて破棄されるまで参照先のオブジェクトは解体されません。

privateなメンバなどで所有権を移す可能性がない場面では余分なコンテナを作ったりしない分シンプルで便利だと思うのですがC++11では非推奨になってしまいました。

なるほどauto_ptrでも大丈夫なのですね。
C++11では非推奨ということですか...極力非推奨のものは使いたくないのでshared_ptrのほうを使いたいと思います。

ライフサイクルのはっきりしているシーン管理より、リソース管理にこそスマートポインタを活用したほうが良いと思います。

なるほどリソース管理ですか。次回に生かしたいと思います。

ちなみにshared_ptrの弱点は速度以外に何かあるのでしょうか？
シーケンス制御でshared_ptrを使用したとき、たまに動作を停止したりします。SafeDeleteの時は一度もありませんでした。（私のプログラムまたは環境のせいかもしれませんが）
何か制約があるでしょうか？

[ISLe]

ちなみにshared_ptrの弱点は速度以外に何かあるのでしょうか？
シーケンス制御でshared_ptrを使用したとき、たまに動作を停止したりします。SafeDeleteの時は一度もありませんでした。（私のプログラムまたは環境のせいかもしれませんが）
何か制約があるでしょうか？

内部でコンテナを確保するのでメモリの分断がより多く発生します。
その点で速度低下に繋がると思いますが、make_sharedというコンテナと参照先オブジェクトのメモリを一括で確保する手段も用意されています。

shared_ptrは自身が確保したコンテナを参照しますから、参照先のオブジェクトから新たに作成したshared_ptrとは関連を持ちません。
最初に作成したshared_ptr自身をコピーして使わないと解体済みのオブジェクトにアクセスする不具合を引き起こすことになります。

例えば参照先のオブジェクトが内部で自分自身(this)をshared_ptrで参照したとしても、外部のshred_ptrとは連携しないので自滅を制御することはできません。
なのでわたしはゲームのオブジェクト管理には汎用スマートポインタを使わずに、参照カウンタを自分自身で持つクラスを作って使っています。
そうするとゲームのオブジェクト毎の処理をクラス内で完結させることができます。

[dig]

内部でコンテナを確保するのでメモリの分断がより多く発生します。
その点で速度低下に繋がると思いますが、make_sharedというコンテナと参照先オブジェクトのメモリを一括で確保する手段も用意されています。

shared_ptrは自身が確保したコンテナを参照しますから、参照先のオブジェクトから新たに作成したshared_ptrとは関連を持ちません。
最初に作成したshared_ptr自身をコピーして使わないと解体済みのオブジェクトにアクセスする不具合を引き起こすことになります。

例えば参照先のオブジェクトが内部で自分自身(this)をshared_ptrで参照したとしても、外部のshred_ptrとは連携しないので自滅を制御することはできません。
なのでわたしはゲームのオブジェクト管理には汎用スマートポインタを使わずに、参照カウンタを自分自身で持つクラスを作って使っています。
そうするとゲームのオブジェクト毎の処理をクラス内で完結させることができます。

あれから私もshared_ptrの問題点について調べてみました。「○○の時はweak_ptrにして」など結構制約があるようですね。
何の考えもなしに「とりあえずshared_ptr」のような感じで使えるものではないですね。これは。
とりあえずはISLeさんの言うとおり用途を限定し、シンプルにリソース管理に使ってみようと思います。

詳しい説明ありがとうございました。