プログラマ専用SNS ミクプラ

お邪魔します。

題名の通り派生クラスがうまくいきません。
うまくいっていないのはbutton→button1の派生です。
エラーを見るクラスbutton自体宣言されていないっと出てきます。
自分では完全に手詰まりになってしまいました。。
どこがいけないのかご教授お願いします。

プロジェクトを丸ごとUPしました。↓
http://www1.axfc.net/uploader/H/
ファイル名：H_72322.zip
Key:kazuoni


あと、初めてC++で組んでいるので、
非効率的なところがほとんどだと思います。
もしよろしければ、派生クラスの回答に加えて、
「ここの記述はこうしたほうがいい（効率的だ）」
「そもそもクラスの使い方が・・・」
などの指摘をお願いできますでしょうか？
C++だとメンバ関数は多用しても、ただの関数ってほとんど使わないのでしょうか？
(すべてクラス化し、メンバ関数とする）

よろしくお願いします。

追加
派生クラス含め、コードに解説が全くされてませんでした。。
とりあえず派生クラスだけを。（解説を入れて再度upします。）

buttonが基になるクラスです。
今はbutton1だけですが、今後2,3を増やしていくつもりです。
押されたかどうかの判定をPushButtonで共通化しています。
派生されたクラスでは描写のDrawButton()を各々のメンバ関数として持っています。
ボタンによって描写の仕方をそれぞれ持つっといったところです。

ざっと見ただけですが...

button1.cppでbutton1.hしかインクルードしていないからではないでしょうか？
button1.hは、button.hが先にインクルードされていることを前提に設計されているようです。

普通はヘッダファイルをインクルードする順序に依存するような設計はしません。
button1.hをインクルードするときにはbutton.hも必要になるのであれば、button1.hからbutton.hをインクルードするようにしましょう。

たかぎさん、ご回答ありがとうございます。

やはり今回のはインクルードの順番が原因でした。。
今回はmain.hですべてのクラスのインターフェース部をインクルードしています。
なので、そのmain.hをすべてインクルードしているから、
全部大丈夫だろうっと思っていました。。
各ファイルで必要なファイルをインクルードしたら解決できました。。

今回の自分のmain.hのように、先にすべてをインクルードしたヘッダーを
インクルードする事を実現することは不可能なんでしょうか？
これが実現できたら、わざわざ先頭でいろいろな記述しなくても
main.hっとかけば済みそうなのですが・・・？

> 今回の自分のmain.hのように、先にすべてをインクルードしたヘッダーを
> インクルードする事を実現することは不可能なんでしょうか？
> これが実現できたら、わざわざ先頭でいろいろな記述しなくても
> main.hっとかけば済みそうなのですが・・・？

できますが、必要ないものまで全部ヘッダファイルに入れてしまうと、コンパイル時間が長くなります。
それと、分割された個々のヘッダファイルを間違って直接インクルードしてしまうおそれもあるので、あまり良い設計ではありません。

＞などの指摘をお願いできますでしょうか？

　同じくざっと見た感じですが、基本的なところから言いますと

■　Buttonクラスのコンストラクタで渡された char *をそのままメンバにポインタを
コピーして持っている。

　危険な兆候です。
　これはいつ外部で破棄されるかわからない（たとえばローカル変数のポインタを渡された場合とか）ので
注意が必要です。

　生のポインタをコピーするのではなく、（std::stringなどで）中身をコピーして
持ったほうがいいかと思います。


■　Button1クラスの基底 Buttonクラスが仮想デストラクタを持っていない
　まぁ、仮想関数がないんで、Button1クラスがアップキャストされることはそうないのかな、
とは思いますが……。

6章：継承
ttp://www5c.biglobe.ne.jp/~ecb/cpp/06_09.html


■　Button::PushButton
　引数のポインタ keyinputが（配列などの）一定領域を持ったポインタであることを
想定していますが、この引数では
[color=sans-serif]
int a;
button->PushButton(&a);
[/color]
ができてしまいますし、そもそも引数が NULLの可能性も考慮されていないので危険です。

　main.cppでは keyinput.StateKeyboard() を渡しているようで、
であれば keyinputそのものを参照で渡して、そこからアクセスした方が安全です。

# あれ？ private継承なのに PushButton()を呼んでる？？


■　Buttonコンストラクタでの処理
button_num=s_num は [color=sans-serif]select_num=s_num[/font] ですか？


■　__Class_Button / __Class_Button1
　予約済み識別子なので、使用は避けた方がいいでしょう。

予約済み識別子 | 移植性のあるCプログラミング
ttp://portable-c.jugem.jp/?eid=10

　あ、たかぎさんのページですね (^^


■　button.h / button1.hなどの最終行
　ファイルの最後が改行ではないので、未定義です。

翻訳過程 - 標準C++辞典 - livedoor Wiki（ウィキ）
ttp://wiki.livedoor.jp/takagi_nobuhisa/d/%CB%DD%CC%F5%B2%E1%C4%F8


■　main.h / OpenigMenu.hのインクルードガード
　ほかのヘッダにはあるのですが、これらのヘッダにはないようです。
　必要ないですか？




＞今回の自分のmain.hのように、先にすべてをインクルードしたヘッダーを
＞インクルードする事を実現することは不可能なんでしょうか？

　できるとは思いますが、お勧めはしません。
　規模が大きくなると、main.h内でのインクルード順に悩まされることになるかと。



＞わざわざ先頭でいろいろな記述しなくてもmain.hっとかけば済みそうなのですが・・・？

　１ヘッダにすべてのヘッダをインクルードするやり方は規模がとても小さなときは
それほど悪くはないのですが、規模が大きくなるに連れていろいろ問題も出てくるので、
特別な事情がない限り避け、そのヘッダのコンパイルに必要なヘッダだけを
インクルードするようにした方がいいかと思いますよ。

　昔そういう方針のプロジェクトで仕事しましたが、悲惨でした。
　中心となる１ヘッダがアプリの基本システムのヘッダをすべてインクルード、
個々のソースはそのヘッダをインクルードしていた為、個々のヘッダ単体をインクルードしただけでは
コンパイルできず、また基本システムのヘッダをちょっとでも変更した瞬間すべてのファイルが
ビルド対象（ほぼフルビルド状態）になり、待つこと３０分、ようやくビルドが終わるという……。



＞C++だとメンバ関数は多用しても、ただの関数ってほとんど使わないのでしょうか？

　そんなことはないです。
　既存のライブラリは別にしても、ただ値を渡して結果をもらうだけであれば、
クラスにする必要がないので（大抵、非グローバル名前空間上の）関数にします。

　それに、クラスの静的メンバ関数もそれに含めるのであれば、さらに機会は
多くなります。

> C++だとメンバ関数は多用しても、ただの関数ってほとんど使わないのでしょうか？

現代的なC++プログラムの設計では、特別な事情がない限り、可能なものはすべて非メンバ、非フレンドにすべきであるとされます。

たかぎさん、Justyさんご回答ありがとうございます。

インクルードの件は今後も必要な物を随時インクルードっという方針で
やっていきます。

とりあえずJustyさんに指定されたところを修正し、
ボタン表示を作ってみました。
ちょっと表示する部分が汚いですが＾＾；
もう少しきれいにできますかね。。
少し心配なのが、stringをchar *へキャストするところですが・・・
コンパイルは問題なかったのですが、ちょっと怪しいです。
コード抜粋

Button::Button(char *str,int x1,int y1,int b_num,int s_num)
{
	std::string ptr_str=str;
	b_string=std::strcpy(new char[ptr_str.size()+1],ptr_str.c_str());
	...

あ、UPしたファイルにはデストラクタでdelete[/url]してません。。
確保したらやはりdelete[/url]しないとだめですよね＾＾；

>可能なものはすべて非メンバ、非フレンドにすべきである
これだと自分のコードはとてつもなく反しているような・・・

再度、修正したものをUPしておきます。
http://www1.axfc.net/uploader/H/#recent
ファイル名：H_72351.zip
Key:kazuoni

Button関係だけしか見ていませんが...

まず、ButtonとButton1が継承関係になっている必然性がありません。
Button2以降をどうするつもりなのかにもよりますが、普通は描画関数を仮想関数にすると思います。

また、const修飾子をうまく使うようにしてください。
例えば、

Button::Button(char *str,int x1,int y1,int b_num,int s_num)

の部分は、

Button::Button(const char *str,int x1,int y1,int b_num,int s_num)

とした方がよいでしょう。
あるいは、

Button::Button(const std::string& str,int x1,int y1,int b_num,int s_num)

とするかです。
そして、

Button::b_string

は、char*ではなく、std::stringにすべきです。

また、Buttonクラスのデータメンバがprotectedになっているのも気になります。
privateにするか、それができないようであれば、根本的に設計を見直すべきです。

　あれ？　string周りがなんか間違った方向に・・・。

　とりあえず、b_stringはたかぎさんの書かれたように std::string型にして
元のデータとは別にクラス内に独立して文字列を持たせた方がいいですね。


■　main.hの color[BLACK] / color[WHITE]
　BLACKとか WHITEの数値は現状 Colorクラスに紐付いたものですよね？
　であれば、main.hに defineで記述するのではなく、Colorクラスのある
MainClass.hに Colorクラスの定数として持たせた方がいいかと思います。

　あと、MOUSE_LEFTなどもそうですが、こういう用途で defineを使うことは
推奨されません。


■　Colorクラス
　よく見ると、コンストラクタで４つ GetColor()を呼んでいますね。
　一方使われ方を見ると、Button1::DrawButton()ではローカル変数として使っています。

　これがグローバル変数で１つしかインスタンスがないのだれば、さほど問題はないのですが、
ローカル変数として何度もコンストラクタが呼ばれるのだとすると、
このクラスの内部の値は環境が変化しない限り必ず同じ値になるはずなので、
毎回 GetColor()で初期化するのはちょっと無駄な気がします。

（GetColor()は内部で使用されているスレッドとの同期をとった上で、
画面深度から求めるので、状況によってはちょっと重い処理となります）

　気にならないというのであればいいのですが。


　ついでに、operator[/url]が intではなく int&を返していますが、これは
color[WHITE] = (int)GetColor(0, 0, 0); //黒にする
のようなことを想定しているからですか？


■　たとえば button.h
　main.hをインクルードしていますが、必要ですか？
　また、KeyInputクラスが引数として使われる為、InputClass.hがインクルードされていますが、
ヘッダの段階では前方参照で済むはずです。

前方宣言
ttp://wisdom.sakura.ne.jp/programming/cpp/cpp23.html

　InputClass.hは button.cpp側でインクルードするといいかと思います。

たかぎさん、ご回答ありがとうございます。

指摘された点につきましては修正しました。
修正後のButton関連のファイルをまとめてテキストにしてUPします。

>ButtonとButton1が継承関係になっている必然性がありません。
自分もそう思っていました＾＾；
初めて継承を使ったもので、何が正しいのかも分かりません。。
ただデータメンバは限定公開ではアクセス可能でダメでしたね。。
値を見る為に関数がドバっと増えましたが、
安全性の面ではこちらのが正しいと勝手に思いますｗ

>Button::b_stringは、char*ではなく、std::stringにすべきです。
これはなぜでしょうか？
文字列操作において、std::stringのがchar *より便利という点からでしょうか？

添付しわすれました・・・。

Justyさん、ご回答ありがとうございます。

>MOUSE_LEFTなどもそうですが、こういう用途で defineを使うことは推奨されません。
っということは何かデメリットがあるのでしょうか？
それともdefineの主な用途に今回のような使い方が含まれていないっという意味でしょうか？


>Colorクラス
確かにおっしゃる通りです。。
グローバル変数にしても、コンストラクタをしっかり呼び出し、
宣言以降、アクセスできないようにすればいいんですね。


>operator[/url]が intではなく int&を返しています
某参考書からそのまま映していたら、&まで映していました。。
必要なかったです。

>インクルード
まだまだ勉強不足です。。
前方宣言はとても使えそうです。
もう少し簡潔にしてきます。

> ただデータメンバは限定公開ではアクセス可能でダメでしたね。。
> 値を見る為に関数がドバっと増えましたが、
> 安全性の面ではこちらのが正しいと勝手に思いますｗ

根本的に何かが違うのですが...

アクセッサを設けるのではなく、DrawButtonの引数として与えるのでは駄目ですか？
DrawButtonは、privateまたはprotectedな仮想関数と、引数を受け取らないpublicなメンバ関数に分離するとよいでしょう。
NVIイディオム（またはNVIパターン）を調べると幸せになれます。

> >Button::b_stringは、char*ではなく、std::stringにすべきです。
> これはなぜでしょうか？
> 文字列操作において、std::stringのがchar *より便利という点からでしょうか？

操作の利便性もありますが、寿命管理だけを考えても、std::stringにするメリットはあります。

Button関係の話ばかりですが...

OpeningMenu::Menuの中で毎回Button1のオブジェクトを生成しなおすことに意味はありますか？
OpeningMenuのメンバとしてButton1のオブジェクトを持つ方が直感的かと思います。

button_numとselect_numという変なメンバも不要かと思います。
単に選択されているかどうかを表すbool型のフラグがあれば十分ですよね。

あと、細かい話ですが、PushButtonとかDrawButtonのように、関数名にButtonをつける必要はありません。

＞＞MOUSE_LEFTなどもそうですが、こういう用途で defineを使うことは推奨されません。
＞っということは何かデメリットがあるのでしょうか？

　一番の問題は defineマクロがプリプロセッサの段階における
ただの文字列置換である、というところです。

翻訳過程 - 標準C++辞典 - livedoor Wiki（ウィキ）
ttp://wiki.livedoor.jp/takagi_nobuhisa/d/%CB%DD%CC%F5%B2%E1%C4%F8
（defineはこのページの 4で処理され、実際のコードの解析は 7で行われる）


　どういうことかというと defineを定数値として使っていたとしても、
コンパイラはそれを文字列レベルで置換するので、対象が変数だろうが関数名だろうが、
名前空間を一切考慮することなく置換していきます。

　たとえば、
[color=#d0d0ff" face="monospace]
namespace X
{
void Max();
}
struct A
{
static int Max();
};
void test()
{
int Max = 4;
}
[/color]
　のようなコードがあったとします。

　これは正当なコードですが、ここで、
[color=#d0d0ff" face="monospace]
#define Max 5
[/color]
　の定義が事前にあったりすると、
[color=#d0d0ff" face="monospace]
namespace X
{
void 5();
}
struct A
{
static int 5();
};
void test()
{
int 5 = 4;
}
[/color]
　のように変換されてから、ソースコードの評価が行われる為、
コンパイルエラーとなります(※)。

　ここで、代替手段である const定数を使い
[color=#d0d0ff" face="monospace]
const int Max = 5;
[/color]
　としていれば、コンパイルエラーはおきません。

　その他、#undefによって deinfeの内容がコロコロ変わる可能性があることや
マクロの種類によっては副作用を伴ったりしますし、
環境依存な話ではありますが（入力補完などの）IDE・デバッガなどのサポートが
受けにくくなる、というのデメリットもあります。


　もちろん、マクロでなくてはできない時……インクルードガードや条件付きコンパイルなどでは
マクロを使うべきですが、代替手段（定数なら constや enum、関数マクロなら inline、
型に依存しないの処理なら template）を使って解決できるなら、何か特別な事情がない限り
マクロを使うよりそちらを使った方が安全です。



※　実際、最近の VisualStudioでは 一部のシステムヘッダにある min()とか max()のマクロが
標準ライブラリの関数と衝突していたりします。

ttp://support.microsoft.com/kb/143208/ja
ttp://support.microsoft.com/?scid=kb%3Ben-us%3B143208&x=12&y=13

返事が遅れて大変申し訳ありませんでした。。

>NVIイディオム
いろいろページをみていたら、最終的にたかぎさんのブログへとたどり着きましたｗ
とりあえずは実装できたつもりですが、
果たしてうまくできているのでしょうか・・・。
あとはb_string(名前はnameに変更しました)や不要メンバ、関数名は
アドバイスしてくしてくださった様に変えました。
（他の関数もこれから変えていくつもりです。）
まだ何かありましたらどんどん指摘してください＾＾；

>マクロ
とても参考になります。
関数形式マクロでの副作用に加え、確かにJustyさんのおっしゃったような
副作用もありますね。。
詳しい解説ありがとうございます。


全然別の話になってしまうのですが、
いろいろと変更していたらGetColor()が戻り値intではなくDWORDになってしまいました。
このせいか、カラーを初期化する際に、すべて0が格納されてしまいます。
何が原因なのか分かりません。
また新しくプロジェクトをUPしておきます。

ファイル名：H_72560.zip
DLキー：kazuoni
http://www1.axfc.net/uploader/H/so/H_72560

よろしくお願いします。

DXライブラリの初期化が完了する以前にGetColor()を呼んでいるからですね。

あ、ちょっとInputのboolのあたりがおかしいですね＾＾；
一度TRUEになったら戻らないようになってました。。
そこは変更しておきました。（プロジェクトは変更前）

SCIさん、ご回答ありがとうございます！

おっしゃるとおりでした。。
大変失礼しました。。

念のため、boolのあたりの変更点をUPしておきます。

　うーん、突っ込みどころが増えたというか、なんというか。


■　インクルード
　いらないヘッダのインクルード、ヘッダ単独でコンパイルの通らないヘッダなどがあるようです。


■　Button::Buttonの nameへの代入処理
　ローカル変数 ptr_strが代入後未使用です。
　nameへの代入方法もおかしくはないですが、こういう場合の初期化としては
普通は +=を使いません。

　nameのコンストラクタに strポインタを引き渡した方がいいでしょう。


■　Button::do_Drawの引数 str
　クラスオブジェクトを実体を引数にすると、コピーが発生しますし、その関数実行後、
そのコピーのデストラクタが実行されます。

　そのクラスオブジェクトのコピー等が仮にコスト的にたいしたことがない場合でも、
特にコピーでなければならない理由がないのであれば参照を、可能なら const参照を
渡した方がいいでしょう。


■　Colorクラス
　白色を取得するのに color[color.white()] はちょっと冗長な感じもします。
　それに、operator[/url]による整数ならすべて受け付けるので、安全とは言い難いです。


■　main.hのグローバル変数定義
　extern版と非extern版の２つに分けて定義するのは、片方の修正は、もう一方の修正を必要とするので、
将来の修正においてバグの要因ともなりえます。

　１つにまとめるか、可能ならグローバル変数のない設計にした方がいいでしょう。


■　OpeningMenuメモリリーク
　コンストラクタでの４つの newに対し、デストラクタでは１回のみしかありません。
　又、たとえば２つ目の newに失敗した場合１つめの newは誰が deleteするのでしょうか？

　他にも、このクラスがコピーされた場合、最終的にコピー元・先のそれぞれでデストラクタが
実行されますが、２つのインスタンスは同じポインタを指しているため、二重解放になる可能性があります。

Justyさんご回答ありがとうございます！

>インクルード
再三の注意にもかかわらず、変更できていませんでした。。
大変申し訳ありませんでした。。

再度指摘されたところをすべて変えてみました。
nameの代入処理がポインタを使っていないところが不安ですが・・・。
カラークラスにおいてはグローバル変数としてはやめました。
（初期化のタイミングがネックっと感じたので＾＾；）
const参照も多用しましたが・・・これで正しいのかどうか疑問です。

そして、ボタンを動的確保していた点ですが、
よくよく考えたら、今は場合によって増えるなんてものではないので、
newそのものをやめました。
ただ、前回のような時の
>クラスがコピーされた場合の二重解放
はもし対策を実装するとすればどのような方法がよいのでしょうか？

再々度UPします。

ファイル名：H_72616.zip
DLキー：kazuoni
http://www1.axfc.net/uploader/H/so/72616

よろしくお願いします。

OpeningMenuのデータメンバが不自然な感じになってますねぇ。
Button1 button0～button3があるのなら、Button1 *button[4]は不要ですよね。
button0～3も配列にした方がいいかも。


Color::set()で行ってる処理は、コンストラクタがすべきではないですか？
それからColor::COlorはstaticやconstで修飾してもよさそうですが、その辺りはどうでしょう？


Buttonのコンストラクタですが、ptr_strを介さずに直接nameにstrを代入できます。
ちなみにstrでstd::stringをコンストラクトしてからnameに代入したい場合は name=std::string(str); や name=static_cast<std::string>(str); のように一時変数を使うと行数を抑えられます。


>>クラスがコピーされた場合の二重解放
>はもし対策を実装するとすればどのような方法がよいのでしょうか？

std::auto_ptr（危険な場合があるので注意）やboost::shared_ptr（boostというライブラリが必要）のようなスマートポインタを使うとか。
あるいはディープコピー(新たにメモリを確保して、そこにコピー)するとか。

ibisさん、ご回答ありがとうございます！

自分もそう考えてOpeningMenuのデータメンバにbutton[4]を持たせ、
コンストラクタで

OpeningMenu::OpeningMenu():
	button[0]("button0",100,100),
	button[1]("button1",100,200),
	button[2]("button2",100,300),
	button[3]("button3",100,400)

とすると
1>error C2059: 構文エラー : '['
1>error C2512: 'Button1' : クラス、構造体、共用体に既定のコンストラクタがありません。
っとなってしまったので、こんな風にしたのですが・・・
どうしたら実現できうでしょうか？

その他のご指摘のあった点は修正しました＾＾；
カラーのあたりは外部変数としていた時のエラー回避のため
変な構造にしていました。。

>スマートポインタ・ディープコピー
大変参考になります。。一度調べてから、試してみたいと思います。

目についたところを列挙していったら結構な数になってしまったので、
こちらで手を入れたコードをそのままアップしてみました。

大筋にはあまり手を入れないようにしましたので、これがベストというわけではありませんが、
ひとまず主立ったおかしな点は解消されているかと思うので、こういう書き方もあるという
例の１つとして参考にしてみてください。

>OpeningMenuのコンストラクタ

多分これでコンパイルは通るかと思います。

OpeningMenu(){
    button0 = Button1("button0",100,100);
    //以下略
}

Justyさん、ibisさんご回答ありがとうございます！
（また返信が遅れて申し訳ありません。。）

>Justyさん
わざわざpdf形式の解説まで作っていただき、本当にありがとうございました。。
pdfってこんな使い方があったんですね＾＾；
まだすべてを完璧に理解していませんが、
大体の流れを勉強させていただきました。
本当に参考になります。。
もう少しじっくり眺めて、自分のプログラムに生かしていきたいと思います！
今回の件はこれで解決とさせていただきます。
また具体的な質問が出たら新しいトピを立てさせていただきます。
（できるだけ自分で解決するように心がけますが）

回答してくださった皆様、本当にありがとうございました。
またよろしくお願いします！

解決＾＾；