STGクラス設計

[タンタル]

前回の続きになります。
自分なりにコードをかいたのですが、どうもうまくいかないようです。
先に困ったところ、聞きたいところをあげておきます。

１．弾生成の手順および弾生成メソッドの場所。ISLeさんに、自機クラスにはいれないと言われましたが、解決策を理解しきれておらず、どうしようか困って結局自機クラスに入れました。CMyShip::Shot(CMyBulletManage)がそれですが、このように引数に他クラスを渡すしか異クラス間のデータのやり取りの方法が分からなかったためこうなりました。

２．上記の影響で、分割コンパイルすると、クラスが未宣言扱いだったり、多重定義だったりしてしまい、デバッグ以前の問題になってしまいます。上手なコンパイル方法が分からないです。#ifndef#endifを使うと思うのですが、どこにどうするべきかが分からないです。(MyShip.hにMyBullet.hをインクルードすると多重定義、インクルードしないとクラス未定義で怒られます。)

今回もご迷惑をおかけしますが、よろしくお願いします。

main.cpp

コード:

#include "Game.h"
#include"DxLib.h"

// main関数の開始
int WINAPI WinMain( HINSTANCE hInstance, HINSTANCE hPrevInstance,LPSTR lpCmdLine, int nCmdShow )
{
	ChangeWindowMode(TRUE)	;		// debug用ウィンドウサイズ
	if( DxLib_Init() == -1 )		// ＤＸライブラリ初期化処理
	{
		return -1 ;			// エラーが起きたら直ちに終了
	}

	SetDrawScreen( DX_SCREEN_BACK ) ;	// 描画先画面を裏画面にする

	CGame Game;

	while (1) {
		ClearDrawScreen()	;	// 画面クリア

		// 本文の記述
		Game.Process();

		ScreenFlip()	;		// redraw 1
		WaitTimer(17)	;		// 約60fps

		if (ProcessMessage() == -1 ) break	;	// windowsメッセージ処理
		if ( CheckHitKey(KEY_INPUT_ESCAPE ) == 1) break	;	// escキーで終了
	
	}

	DxLib_End()	;//DXライブラリ終了処理

	return 0	;
}

Game.h

コード:

#include"MyBullet.h"
#include"MyShip.h"

class CGame{
private:
	CMyShip MyShip;
	CMyBulletManage MyBulletManage;

public:
	void Process();
};

MyShip.h

コード:

class CMyShip{
private:
	int x,y,v;
	int GraphHandle[4];
	int handlec,c;
public:
	CMyShip();
	void Move();
	void Draw();
	void Shot(CMyBulletManage MyBulletManage);
	void Process(CMyBulletManage MyBulletManage);
};

MyBullet.h

コード:

class CMyBullet{
private:
	int x,y;
	static const int v;
	bool f;
	int GraphHandle[8];
public:
	CMyBullet();
	void Set(int x,int y);
	void Move();
	void Draw();
	bool IsAllocated();
};

class CMyBulletManage{
private:
	CMyBullet MyBullet[30];
	int WaitKeyc;
public:
	CMyBulletManage();
	void Set(int x,int y);
	void Process();
};

Game.cpp

コード:

#include"Game.h"

void CGame::Process(){
	MyShip.Process(MyBulletManage);
	MyBulletManage.Process();
}

MyShip.cpp

コード:

#include"DxLib.h"
#include"MyShip.h"

CMyShip::CMyShip(){
	CMyShip::x=320;
	CMyShip::y=400;
	CMyShip::v=3;
	CMyShip::handlec=0;
	CMyShip::c=0;
	LoadDivGraph("jiki.png",4,4,1,24,32,CMyShip::GraphHandle);
}

void CMyShip::Move(){
	if(CheckHitKey(KEY_INPUT_LEFT)==1){
		CMyShip::x-=CMyShip::v;
		if(CMyShip::x<0) CMyShip::x=0;
	}
	if(CheckHitKey(KEY_INPUT_UP)==1){
		CMyShip::y-=CMyShip::v;
		if(CMyShip::y<0) CMyShip::y=0;
	}
	if(CheckHitKey(KEY_INPUT_RIGHT)==1){
		CMyShip::x+=CMyShip::v;
		if(CMyShip::x>640-24) CMyShip::x=616;
	}
	if(CheckHitKey(KEY_INPUT_DOWN)==1){
		CMyShip::y+=CMyShip::v;
		if(CMyShip::y>454) CMyShip::y=454;
	}
}

void CMyShip::Draw(){
	if(CMyShip::c%16==0) CMyShip::handlec++;
	if(CMyShip::handlec==4) CMyShip::handlec=0;

	DrawGraph(CMyShip::x,CMyShip::y,CMyShip::GraphHandle[CMyShip::handlec],TRUE);
}

void CMyShip::Shot(CMyBulletManage MyBulletManage){
	MyBulletManage.Set(CMyShip::x,CMyShip::y);
}

void CMyShip::Process(CMyBulletManage MyBulletManage){
	CMyShip::Move();
	CMyShip::Shot(MyBulletManage);
	CMyShip::Draw();
}

MyBullet.cpp

コード:

#include"DxLib.h"
#include"MyBullet.h"

const int CMyBullet::v=8;

CMyBullet::CMyBullet(){
	CMyBullet::x=0;
	CMyBullet::y=0;
	CMyBullet::f=FALSE;
	LoadDivGraph("jtm.png",8,8,1,16,18,CMyBullet::GraphHandle);
}

void CMyBullet::Set(int x,int y){
	CMyBullet::x=x;
	CMyBullet::y=y;
	CMyBullet::f=TRUE;
}

void CMyBullet::Move(){
	CMyBullet::y-=CMyBullet::v;
	if(CMyBullet::y<0) CMyBullet::f=FALSE;
}

void CMyBullet::Draw(){
	DrawGraph(CMyBullet::x,CMyBullet::y,CMyBullet::GraphHandle[0],TRUE);
}

bool CMyBullet::IsAllocated(){
	return CMyBullet::f;
}


CMyBulletManage::CMyBulletManage(){
	CMyBulletManage::WaitKeyc=0;
}

void CMyBulletManage::Set(int x,int y){
	int i;

	if(CheckHitKey(KEY_INPUT_SPACE)==1){
		CMyBulletManage::WaitKeyc++;
		if(CMyBulletManage::WaitKeyc%5==1){
			for(i=0;i<30;i++){
				if(MyBullet[i].IsAllocated()==TRUE) continue;

				MyBullet[i].Set(x,y);

				break;
			}
		}
	}
	else CMyBulletManage::WaitKeyc=0;
}

void CMyBulletManage::Process(){
	int i;
	for(i=0;i<30;i++){
		if(MyBullet[i].IsAllocated()==FALSE) continue;

		MyBullet[i].Move();
		MyBullet[i].Draw();
	}
}

以上が、これまでに皆様から頂いたアドバイスで自分なりにたどり着いた結論になります。

[nullptr]

懸念通りというか･･･まず、プログラムのコンパイルから失敗しましたか。
今から添削してみます。

[nullptr]

MyShip.h

コード:

/// CMyBulletManageの宣言がない　前方宣言で対処します MyBullet.hをここでインクルードしてもよい（MyBullet.hの多重インクルードを防止すれば） MyShip.cppに関連注釈
class CMyBulletManage;　　/// ☆

class CMyShip{
private:
    int x,y,v;
    int GraphHandle[4];
    int handlec,c;
public:
    CMyShip();
    void Move();
    void Draw();
    void Shot(CMyBulletManage MyBulletManage);
    void Process(CMyBulletManage MyBulletManage);
};

MyShip.cpp

コード:

#include"DxLib.h"
#include"MyShip.h"
#include "MyBullet.h"   /// ☆
//< ↑どこかしらでインクルードしないとダメに決まってます。分割コンパイルをしたことがないのでしょうか？
 
CMyShip::CMyShip(){
    CMyShip::x=320;
    CMyShip::y=400;
    CMyShip::v=3;
    CMyShip::handlec=0;
    CMyShip::c=0;
    LoadDivGraph("jiki.png",4,4,1,24,32,CMyShip::GraphHandle);
}
 
void CMyShip::Move(){
    if(CheckHitKey(KEY_INPUT_LEFT)==1){
        CMyShip::x-=CMyShip::v;
        if(CMyShip::x<0) CMyShip::x=0;
    }
    if(CheckHitKey(KEY_INPUT_UP)==1){
        CMyShip::y-=CMyShip::v;
        if(CMyShip::y<0) CMyShip::y=0;
    }
    if(CheckHitKey(KEY_INPUT_RIGHT)==1){
        CMyShip::x+=CMyShip::v;
        if(CMyShip::x>640-24) CMyShip::x=616;
    }
    if(CheckHitKey(KEY_INPUT_DOWN)==1){
        CMyShip::y+=CMyShip::v;
        if(CMyShip::y>454) CMyShip::y=454;
    }
}
 
void CMyShip::Draw(){
    if(CMyShip::c%16==0) CMyShip::handlec++;
    if(CMyShip::handlec==4) CMyShip::handlec=0;
 
    DrawGraph(CMyShip::x,CMyShip::y,CMyShip::GraphHandle[CMyShip::handlec],TRUE);
}
 
void CMyShip::Shot(CMyBulletManage MyBulletManage){
    MyBulletManage.Set(CMyShip::x,CMyShip::y);
}
 
void CMyShip::Process(CMyBulletManage MyBulletManage){
    CMyShip::Move();
    CMyShip::Shot(MyBulletManage);
    CMyShip::Draw();
}

/// ☆のところを書き換えました
これでコンパイルは通ります。これから中を見ていきます

[nullptr]

コンパイルし、画像を用意しましたが、自機だけ表示されて弾が表示されませんね。
･･･指摘する点が、多すぎですねこれは(・ω・`;;;）

まず演算と描画は分離してください。そうすることで、見た目を変えたい時は描画関数だけ書き換えたり出来たほうが柔軟です。コードも見やすくなります。
弾が描画されないのは

コード:

void CMyShip::Process(CMyBulletManage MyBulletManage){
    CMyShip::Move();
    CMyShip::Shot(MyBulletManage);
    CMyShip::Draw();
}

でCMyBulletManage MyBulletManageと値渡しされているにもかかわらず戻り値でそれを返していないのですから、まったく意味がありません。
前トピに私が追加で返信したポインタに関するお話以前の話ですが、戻り値で返してコピーするかポインタ使うかにしましょう。

[nullptr]

コード:

void CMyShip::Shot(CMyBulletManage& MyBulletManage){
    MyBulletManage.Set(CMyShip::x,CMyShip::y);
}
 
void CMyShip::Process(CMyBulletManage& MyBulletManage){
    CMyShip::Move();
    CMyShip::Shot(MyBulletManage);
    CMyShip::Draw();
}

というように書き換えれば弾が表示されます（宣言の方も直してください。）
さて･･･とりあえずやっと思うような動きになったと思われるので、設計について見ていきます。

[nullptr]

main.cpp

コード:

#include "Game.h"
#include"DxLib.h"

// main関数の開始
int WINAPI WinMain( HINSTANCE hInstance, HINSTANCE hPrevInstance,LPSTR lpCmdLine, int nCmdShow )
{
	ChangeWindowMode(TRUE)	;		// debug用ウィンドウサイズ
	if( DxLib_Init() == -1 )		// ＤＸライブラリ初期化処理
	{
		return -1 ;			// エラーが起きたら直ちに終了
	}

	SetDrawScreen( DX_SCREEN_BACK ) ;	// 描画先画面を裏画面にする

	CGame Game;

	while (1) {
		ClearDrawScreen()	;	// 画面クリア

		// 本文の記述
		Game.Process();

		ScreenFlip()	;		// redraw 1
		WaitTimer(17)	;		// 約60fps

		if (ProcessMessage() == -1 ) break	;	// windowsメッセージ処理
		if ( CheckHitKey(KEY_INPUT_ESCAPE ) == 1) break	;	// escキーで終了
	
	}

	DxLib_End()	;//DXライブラリ終了処理

	return 0	;
}

Game.h

コード:

#include"MyBullet.h"
#include"MyShip.h"

class CGame{
private:
	CMyShip MyShip;
	CMyBulletManage MyBulletManage;

public:
	void Process();
};

おそらくコレはゲーム管理クラスでしょうか。
この場合、主人公と弾の管理を同階層に置いたということですね。


MyShip.h

コード:

class CMyShip{
private:
	int x,y,v;
	int GraphHandle[4];
	int handlec,c;
public:
	CMyShip();
	void Move();
	void Draw();
	void Shot(CMyBulletManage MyBulletManage);
	void Process(CMyBulletManage MyBulletManage);
};

まずshot関数について疑問がわきます。
これは、本当にパブリックでいいんですか？

外部から弾を撃てと呼ばれることがあるのでしょうか。そういう仕様でないなら、外部に公開する必要はないはずです。
関数や変数1つずつ、アクセス制御に気を配るべきですよね。これでは外部が勝手に弾を自機に撃たせてしまいます。

moveとdrawをせっかく分けているのでprocessでまとめてやるのは個人的に好きではないですが、一旦置いときましょう。

設計以前の話ですが、このファイルでまっさきに気になったのがCMyBulletManageの宣言がないこと。コンパイルは翻訳単位毎に行われますので、翻訳単位内に宣言が必要です。
前方宣言か、インクルードしてください。（個人的にはこの場合インクルードはのほうが良いと思いますが多重インクルード対策をしていないのでぜんぽうせんげんです）


MyBullet.h

コード:

class CMyBullet{
private:
	int x,y;
	static const int v;
	bool f;
	int GraphHandle[8];
public:
	CMyBullet();
	void Set(int x,int y);
	void Move();
	void Draw();
	bool IsAllocated();
};

class CMyBulletManage{
private:
	CMyBullet MyBullet[30];
	int WaitKeyc;
public:
	CMyBulletManage();
	void Set(int x,int y);
	void Process();
};

別に同じファイルに複数クラスを宣言しても良いのですが、わけないと後々どれがどれだかややこしくなるのでおすすめしません＾＾

･･･まぁ、それぞれ単体で見ればアクセスについては大丈夫そうですね。（少し気になるけど）
本当はインターフェイスを使って更に関数を絞るべきですが、まだポインタに関して理解が浅いようなのでやめときましょう。


Game.cpp

コード:

#include"Game.h"

void CGame::Process(){
	MyShip.Process(MyBulletManage);
	MyBulletManage.Process();
}

MyShip.cpp

コード:

#include"DxLib.h"
#include"MyShip.h"

CMyShip::CMyShip(){
	CMyShip::x=320;
	CMyShip::y=400;
	CMyShip::v=3;
	CMyShip::handlec=0;
	CMyShip::c=0;
	LoadDivGraph("jiki.png",4,4,1,24,32,CMyShip::GraphHandle);
}

void CMyShip::Move(){
	if(CheckHitKey(KEY_INPUT_LEFT)==1){
		CMyShip::x-=CMyShip::v;
		if(CMyShip::x<0) CMyShip::x=0;
	}
	if(CheckHitKey(KEY_INPUT_UP)==1){
		CMyShip::y-=CMyShip::v;
		if(CMyShip::y<0) CMyShip::y=0;
	}
	if(CheckHitKey(KEY_INPUT_RIGHT)==1){
		CMyShip::x+=CMyShip::v;
		if(CMyShip::x>640-24) CMyShip::x=616;
	}
	if(CheckHitKey(KEY_INPUT_DOWN)==1){
		CMyShip::y+=CMyShip::v;
		if(CMyShip::y>454) CMyShip::y=454;
	}
}

void CMyShip::Draw(){
	if(CMyShip::c%16==0) CMyShip::handlec++;
	if(CMyShip::handlec==4) CMyShip::handlec=0;

	DrawGraph(CMyShip::x,CMyShip::y,CMyShip::GraphHandle[CMyShip::handlec],TRUE);
}

void CMyShip::Shot(CMyBulletManage MyBulletManage){
	MyBulletManage.Set(CMyShip::x,CMyShip::y);
}

void CMyShip::Process(CMyBulletManage MyBulletManage){
	CMyShip::Move();
	CMyShip::Shot(MyBulletManage);
	CMyShip::Draw();
}

「const」というものがあります。これは定数などに使われることがありますが、それ以上に関数につけるのが重要です

このクラスに限らないのですが、例えばdrawメソッドは中でなにか書き換えるべきでしょうか？このクラスでは書き換えてしまっていますが、「描画関数」なのに変数の書き換えが起きてしまうのはおかしいです。
そういう際、まず計算は他の関数へ吐き出した上で、drawメソッドをconst設定します。

void CMyShip::Draw() const {
DrawGraph(CMyShip::x,CMyShip::y,CMyShip::GraphHandle[CMyShip::handlec],TRUE);
}

const設定された関数は処理で代入が行えません(コンパイルエラーになります)。「絶対に書き換えが起きない」保証になるわけです。
書き換えすべきでない関数を見定め、const指定にし、書き換えは別にまとめるべきです。
これも、アクセス制御と同じで保守性を高める工夫です。


MyBullet.cpp

コード:

#include"DxLib.h"
#include"MyBullet.h"

const int CMyBullet::v=8;

CMyBullet::CMyBullet(){
	CMyBullet::x=0;
	CMyBullet::y=0;
	CMyBullet::f=FALSE;
	LoadDivGraph("jtm.png",8,8,1,16,18,CMyBullet::GraphHandle);
}

void CMyBullet::Set(int x,int y){
	CMyBullet::x=x;
	CMyBullet::y=y;
	CMyBullet::f=TRUE;
}

void CMyBullet::Move(){
	CMyBullet::y-=CMyBullet::v;
	if(CMyBullet::y<0) CMyBullet::f=FALSE;
}

void CMyBullet::Draw(){
	DrawGraph(CMyBullet::x,CMyBullet::y,CMyBullet::GraphHandle[0],TRUE);
}

bool CMyBullet::IsAllocated(){
	return CMyBullet::f;
}


CMyBulletManage::CMyBulletManage(){
	CMyBulletManage::WaitKeyc=0;
}

void CMyBulletManage::Set(int x,int y){
	int i;

	if(CheckHitKey(KEY_INPUT_SPACE)==1){
		CMyBulletManage::WaitKeyc++;
		if(CMyBulletManage::WaitKeyc%5==1){
			for(i=0;i<30;i++){
				if(MyBullet[i].IsAllocated()==TRUE) continue;

				MyBullet[i].Set(x,y);

				break;
			}
		}
	}
	else CMyBulletManage::WaitKeyc=0;
}

void CMyBulletManage::Process(){
	int i;
	for(i=0;i<30;i++){
		if(MyBullet[i].IsAllocated()==FALSE) continue;

		MyBullet[i].Move();
		MyBullet[i].Draw();
	}
}

メンバ変数の初期化はできるだけ初期化リストを使いましょう。

ちなみに自身のクラスの変数はアクセス指定子にかかわらずアクセスでき、「this->メンバ名」とするほうがわかりやすいですよ（人により意見はありますが）

それと、まさかグローバル定数が出てくるとは･･･アクセス制御です、鬱陶しいですがアクセス制御してください！
この場合、これを無名名前空間に入れれば大丈夫です。

自機に弾管理クラスを渡す、という設計はまぁ一つの方法です。インターフェイスを渡すようにすべきですし、他の設計方法もありますが、その前に他の点を直してみてください。




　＊　＊　＊


まず、全体的に雑さが目立ちます、もちろん技術レベルは理解できましたし完璧なコードが来るとは思っていませんでしたが、せめて前回話したアクセス制御にもう少し気をつけて欲しいですね。本当にクドいんですけど、まずはそこからです＾＾；
ただ、コードを一発で綺麗に書ける人はプロだけです。私も一発では全然書けません。

もう一回リファクタリング･･･いえ、もう一回書きなおしてみてください。きっとコレよりよいコードが書けると思うので、もう少しアクセス制御やモジュール化を意識して書きなおしてみてください。私は弾管理の設計についてそのあと意見を述べます。

[タンタル]

お礼が遅くなりました。すみません。
ありがとうございます。とりあえず、目標は達成できましたが...

とりあえず、もう一度作り直してみます。
時間もかかると思いますし、その時は再び別のトピックとして質問したいと思います。

新月の獅子さん、本当におせわになりました。
ありがとうございました。

[nullptr]

ええ、時間をかけて直してみてください。

いろんなモノを作ってみるのも大切ですが、一度書いたコードをより良い物に直そうとするのはプログラムの勉強で最も大事だと私は勝手に思っています。頑張ってください(*´∀｀*)

※あーあと私結構この掲示板見ないことも多いので、ここのコミュに登録するとかしてトピ立てたら私にメッセージ送ってくれれば確実に返信できます
まぁ他の方も返信してくれるでしょうし大丈夫でしょうが

[nil]

ふと思ったのですが、
ScreenFlipは通常であれば垂直同期が働いて自動的に60fps程度(きっちりではない)になるはず。
さらにWaitTimerを使えば60fpsにはどうしてもならないと思うのですが。