プログラムの評価依頼

[dig]

おはようございます。

C++の勉強にと本サイト（龍神録プログラミング館）を参考にC++でシューティングゲームを作ってみました。
しかしながら、C#ばかりでC++の経験の浅い私にはC++として組めているのか（許容範囲レベルとして）いまいちわかりません。

そこでどなたか詳しい方以下のプロジェクトを見ていただけないでしょうか？（デバックという意味ではありません。）
よろしくお願いします。

[beatle]

まだ全体を見てませんが、少し気になる点としては、ScoreクラスがscoreFormatとhighScoreFormatをメンバとして持っていることです。
これらはScore::DrawScore()内でしか使われておらず、Score::DrawScore()のローカル変数にした方が良さそうです。

それから、グローバル関数が一つもないのが気になりました。
C#はそもそもグローバル関数は作れないのでいいのですが、C++はグローバル関数が作れますから、その利点を十分に生かせばいいと思います。
グローバル関数は、クラスのメンバ関数と違って、ステートに関係ない処理を記述するには持って来いです。
Effective C++には、あるクラスのpublicメンバ関数だけで実装できる処理は、メンバにせずにグローバル関数にしなさいという記述があります。

（コードの命名規則はクラス名と関数名は「パスカルケース」。メンバ名は「キャメルケース」。ローカルやその他の名前は「全て小文字で単語の区切りにアンダースコア」で記述しています。）

WinMain関数内で定義しているローカル変数がcamelCaseになってしまっていますよ。

[h2so5]

全部は見ていませんが、

1. Enemy.cpp の37行目に「LoadEnemyData()でnewしました。」というコメントがありますが、
このような、どこでnewしたのか一見分からないような構造は止めたほうがいいと思います。

2. 龍神録では使っていないと思いますが、C++で文字列を扱うならstd::stringを使うほうが良いです。
strncpyなどを使わなくて済みます。

[たかぎ]

さわりしか見ていませんので、漠然とした回答になりますが...

1. 静的に解決できることを動的に行わないようにしましょう。
2. const修飾を徹底しましょう。
3. RAIIも徹底しましょう。
4. カプセル化や有効範囲の制限を適切に行いましょう。
5. 標準ライブラリ（Visual C++ 2008の場合はTR1を含む）を利用しましょう。
6. エラー対策（エラーコードの処理や例外安全など）を正しく行いましょう。

そして...
間違ったセマンティックスに対してコンパイルエラーが発生するように実装しましょう。

これだけ気を付ければ、かなり改善するはずです。

[dig]

まだ全体を見てませんが、少し気になる点としては、ScoreクラスがscoreFormatとhighScoreFormatをメンバとして持っていることです。
これらはScore::DrawScore()内でしか使われておらず、Score::DrawScore()のローカル変数にした方が良さそうです。

それから、グローバル関数が一つもないのが気になりました。
C#はそもそもグローバル関数は作れないのでいいのですが、C++はグローバル関数が作れますから、その利点を十分に生かせばいいと思います。
グローバル関数は、クラスのメンバ関数と違って、ステートに関係ない処理を記述するには持って来いです。
Effective C++には、あるクラスのpublicメンバ関数だけで実装できる処理は、メンバにせずにグローバル関数にしなさいという記述があります。

（コードの命名規則はクラス名と関数名は「パスカルケース」。メンバ名は「キャメルケース」。ローカルやその他の名前は「全て小文字で単語の区切りにアンダースコア」で記述しています。）

WinMain関数内で定義しているローカル変数がcamelCaseになってしまっていますよ。

beatleさん　ご指摘ありがとうございます。WinMain内の記述はご指摘のとおり間違っていました。失礼しました。
Scoreの件も考えてみたいと思います。
グローバル関数については使いどころがよく理解できていません。
「共有すべき関数などをグローバル関数とする。」という感覚でよろしいのでしょうか？（たとえばSwap()みたいなものとか）

全部は見ていませんが、

1. Enemy.cpp の37行目に「LoadEnemyData()でnewしました。」というコメントがありますが、
このような、どこでnewしたのか一見分からないような構造は止めたほうがいいと思います。

2. 龍神録では使っていないと思いますが、C++で文字列を扱うならstd::stringを使うほうが良いです。
strncpyなどを使わなくて済みます。

h2so5さん　ご指摘ありがとうございます。
１についてですが質問があります。C++ではnewする場合どこに書くのがセオリーなのでしょうか？
２についてはご指摘のとおりstringのほうが便利ですね。そちらを使うようにします。

さわりしか見ていませんので、漠然とした回答になりますが...

1. 静的に解決できることを動的に行わないようにしましょう。
2. const修飾を徹底しましょう。
3. RAIIも徹底しましょう。
4. カプセル化や有効範囲の制限を適切に行いましょう。
5. 標準ライブラリ（Visual C++ 2008の場合はTR1を含む）を利用しましょう。
6. エラー対策（エラーコードの処理や例外安全など）を正しく行いましょう。

そして...
間違ったセマンティックスに対してコンパイルエラーが発生するように実装しましょう。

これだけ気を付ければ、かなり改善するはずです。

たかぎさん　ご指摘ありがとうございます。
1～６についてよく考えてみようと思います。
ただ「間違ったセマンティックスに対してコンパイルエラーが発生するように実装...」についてはよくわかりませんでした。
これはどういうことなのでしょうか？

[h2so5]

１についてですが質問があります。C++ではnewする場合どこに書くのがセオリーなのでしょうか？

決まりがあるわけではないですが、デストラクタでdeleteしているならコンストラクタでnewしているのが自然でしょう。
それ以上new/deleteの関係が複雑になるならスマートポインタの使用を検討すべきです。

また、本当に Enemy.cppで enData を動的確保する必要性があるかどうか考えたほうがいいです。
可変長の配列が必要ならSTLコンテナを使ってください。

[beatle]

最近のC++の流れ的には、newしたポインタをそのまま扱うのではなく、スマートポインタなどのRAII（Resource Acquisition Is Initialization：リソースの獲得は初期化。要するに、クラスのコンストラクタでリソースを獲得するようにすること）なクラスを使ってリソースを管理しましょうということになっています。
C#をお使いならよくご存知だと思いますが、ガベージコレクションがあるために、自分でdeleteを書きませんよね？C++界もそういう流れになってきています。

C++ではスマートポインタというクラスを使って、「明示的なnewに対する明示的なdeleteを書かない」ようにします。
明示的なdeleteを書く代わりに、スマートポインタのデストラクタにdelete作業を任せてしまいます。

コード:

#include <memory>

std::shared_ptr<int> MakeInt()
{
    return std::make_shared<int>(1);
}

void f()
{
    std::shared_ptr<int> ptr_int = MakeInt();
    std::cout << *ptr_int << std::endl;
    *ptr_int = 2;
    std::cout << *ptr_int << std::endl;

    // ここでptr_intが指しているメモリ領域は、ptr_intのデストラクタによりdeleteされる
}

std::shared_ptrは参照カウント式のスマートポインタでして、動作はC#の参照型に近いです。どこからも参照されなくなると、自動的にdeleteされます。

[たかぎ]

たかぎさん　ご指摘ありがとうございます。
1～６についてよく考えてみようと思います。
ただ「間違ったセマンティックスに対してコンパイルエラーが発生するように実装...」についてはよくわかりませんでした。
これはどういうことなのでしょうか？

他の項目とも関連するのですが...
例えば、オブジェクトのコピーが適切でない（今回の場合、多くがそれに該当するかと思います）場合、コピーコンストラクタやコピー代入演算子を非公開にして、コピーしようとするとコンパイルできなくなるような配慮を行うべきだということです。
（他にもいろいろなケースが考えられます）
Visual C++ 2008なので、static_asserｔが使えないのがやや痛いですね。

[たかぎ]

std::shared_ptrは参照カウント式のスマートポインタでして、動作はC#の参照型に近いです。どこからも参照されなくなると、自動的にdeleteされます。

Visual C++ 2008なので、std::tr1::shared_ptrですね。

[dig]

１についてですが質問があります。C++ではnewする場合どこに書くのがセオリーなのでしょうか？

決まりがあるわけではないですが、デストラクタでdeleteしているならコンストラクタでnewしているのが自然でしょう。
それ以上new/deleteの関係が複雑になるならスマートポインタの使用を検討すべきです。

また、本当に Enemy.cppで enData を動的確保する必要性があるかどうか考えたほうがいいです。
可変長の配列が必要ならSTLコンテナを使ってください。

h2so5さま　
回答ありがとうございます。new/deleteは自然な関係であったほうがよいということですね。肝に銘じておきます。
スマートポインタについてはまったく知りませんでした。次に作成するプログラムで使ってみようと思います。
STLコンテナについてもほとんどわからないのでこれも調べてみます。

最近のC++の流れ的には、newしたポインタをそのまま扱うのではなく、スマートポインタなどのRAII（Resource Acquisition Is Initialization：リソースの獲得は初期化。要するに、クラスのコンストラクタでリソースを獲得するようにすること）なクラスを使ってリソースを管理しましょうということになっています。
C#をお使いならよくご存知だと思いますが、ガベージコレクションがあるために、自分でdeleteを書きませんよね？C++界もそういう流れになってきています。

C++ではスマートポインタというクラスを使って、「明示的なnewに対する明示的なdeleteを書かない」ようにします。
明示的なdeleteを書く代わりに、スマートポインタのデストラクタにdelete作業を任せてしまいます。
...
std::shared_ptrは参照カウント式のスマートポインタでして、動作はC#の参照型に近いです。どこからも参照されなくなると、自動的にdeleteされます。

beatleさま
スマートポインタの詳しい説明ありがとうございます。今のところこれらのことはよく理解できていません。
次に作成するプログラムから使ってみようと思います。

たかぎさん　ご指摘ありがとうございます。
1～６についてよく考えてみようと思います。
ただ「間違ったセマンティックスに対してコンパイルエラーが発生するように実装...」についてはよくわかりませんでした。
これはどういうことなのでしょうか？

他の項目とも関連するのですが...
例えば、オブジェクトのコピーが適切でない（今回の場合、多くがそれに該当するかと思います）場合、コピーコンストラクタやコピー代入演算子を非公開にして、コピーしようとするとコンパイルできなくなるような配慮を行うべきだということです。
（他にもいろいろなケースが考えられます）
Visual C++ 2008なので、static_asserｔが使えないのがやや痛いですね。

たかぎさま
回答ありがとうございます。しかしながらイマイチ理解できませんでした。「...今回の場合、多くがそれに該当するかと思います...」と書かれていますが、
エラーが発生するように実装しなければならない部分は、例えば私のコードでいうとどのファイルの何行目の部分でしょうか？何度も申し訳ありませんが
よろしくお願いします。

[たかぎ]

たかぎさま
回答ありがとうございます。しかしながらイマイチ理解できませんでした。「...今回の場合、多くがそれに該当するかと思います...」と書かれていますが、
エラーが発生するように実装しなければならない部分は、例えば私のコードでいうとどのファイルの何行目の部分でしょうか？何度も申し訳ありませんが
よろしくお願いします。

コピーの禁止に限っていえば、一番わかりやすいのはSoundクラスでしょうか？
コンストラクタで

コード:

	soundEffect = new int[SOUND_KIND_MAX];
	soundEffectPlayFlag = new int[SOUND_KIND_MAX];

のようにオブジェクトを生成し、デストラクタで

コード:

	delete [] soundEffect;
	delete [] soundEffectPlayFlag;

のように解放しています。
ところが、Soundクラスのオブジェクトをコピーされた場合を考えてみてください。
具体的には、

コード:

Sound s1;
Sound s2(s1);

この場合、s2のデストラクタでdeleteが実行されたあと、同じポインタに対してs1のデストラクタでもdeleteしてしまいます。
つまり、二重解放が発生します。
shared_ptrを使ってこの問題を回避することも可能ですが、用途によってはそもそもコピーできないようにしてしまう方が得策です。
つまり、

コード:

class Sound
{
    ...
private:
    Sound(const Sound&);
    Sound& operator=(const Sound&);
};

のようにしておくのです。
これで、間違ってコピーするようなコードを書いてもコンパイルエラーになります。

他のクラスでも、同じファイルに対して何度も何度もLoadGraphを呼んでしまうとか、そんなことをやっても適切に動作するように実装するか、そもそもそんなことができないようにコンパイルエラーにしてしまうかする必要があるかと思います。

[dig]

たかぎさま
回答ありがとうございます。しかしながらイマイチ理解できませんでした。「...今回の場合、多くがそれに該当するかと思います...」と書かれていますが、
エラーが発生するように実装しなければならない部分は、例えば私のコードでいうとどのファイルの何行目の部分でしょうか？何度も申し訳ありませんが
よろしくお願いします。

コピーの禁止に限っていえば、一番わかりやすいのはSoundクラスでしょうか？
...
他のクラスでも、同じファイルに対して何度も何度もLoadGraphを読んでしまうとか、そんなことをやっても適切に動作するように実装するか、そもそもそんなことができないようにコンパイルエラーにしてしまうかする必要があるかと思います。

たかぎさま
詳しい説明ありがとうございました。なるほどそういうことですね。理解できました。



C++は大変な言語ですね。様々な配慮をしなければなりません。
しかしながらこれも慣れなのでしょう。もう少し小規模なプログラムで練習し直してきます。

今回プログラム評価していただいた皆様に御礼申し上げます。
本当にありがとうございました。

[たかぎ]

C++は大変な言語ですね。様々な配慮をしなければなりません。
しかしながらこれも慣れなのでしょう。もう少し小規模なプログラムで練習し直してきます。

おっしゃるように、ある程度は慣れの問題で、決してC++は大変な言語ではありません。
逆に、私からすればC#の方がずっと大変な言語に感じています。
（かゆいところにまるで手が届きません）