Visual studioの関数化

Question

弾と敵と自分の「行動、初期化、表示」の関数化をしたのですが、プロトタイプ宣言、関数の呼び出し、関数本体の定義等、しっかり関数化できているか見てほしいです。関数化する前と同じ動作はできています。場所などおかしいところがあれば教えてほしいです。

#include "MyGameMain.h"

//ゲーム情報
DG::Image::SP imgPlayer, imgShot, imgEnemy;
XI::GamePad::SP in1;

enum class State { Normal, Hit, Non };

struct Chara {

State    state;
int      x, y;
int moveCnt;
ML::Box2D hitBase;

};

Chara player;
Chara enemys[30];//敵最大３０
Chara shots[10]; //ショット最大10発
void Shot_Appear(int x_, int y_);
void Player_Initialize(Chara& c_, int x_, int y_);
void Player_UpDate(Chara& c_);
void Player_Draw(Chara& c_);
void Enemy_Initialize(Chara& c_, int x_, int y_);
void Enemy_Draw(Chara& c_);
void Enemy_UpDate(Chara& c_);
void Shot_Initialize(Chara& c_,int x_,int y_);
void Shot_Draw(Chara& c_);
void Shot_UpDate(Chara& c_);

//-----------------------------------------------------------------------------
//初期化処理
//機能概要：プログラム起動時に１回実行される（素材などの準備を行う）
//-----------------------------------------------------------------------------
void MyGameMain_Initialize()
{
imgPlayer = DG::Image::Create("./data/image/player.png");
imgShot = DG::Image::Create("./data/image/Shot.png");
imgEnemy = DG::Image::Create("./data/image/Enemy1.png");

//キーボードの入力を受け取るオブジェクトを作成する
//アナログスティック
XI::AnalogAxisKB ls = { DIK_LEFT, DIK_RIGHT, DIK_UP, DIK_DOWN };
XI::AnalogAxisKB rs = { DIK_NUMPAD4, DIK_NUMPAD6, DIK_NUMPAD8, DIK_NUMPAD2 };
//トリガー
XI::AnalogTriggerKB tg = { DIK_E, DIK_R };
//十字キーとキーボード
XI::KeyDatas_KB    key = {
	{ DIK_Z, XI::VGP::B1 },{ DIK_X, XI::VGP::B2 },
	{ DIK_C, XI::VGP::B3 },{ DIK_V, XI::VGP::B4 },
	{ DIK_A, XI::VGP::SE },{ DIK_S, XI::VGP::ST },
	{ DIK_Q, XI::VGP::L1 },{ DIK_W, XI::VGP::R1},
	{ DIK_D, XI::VGP::L3 },{ DIK_NUMPAD5, XI::VGP::R3 },
};
in1 = XI::GamePad::CreateKB(ls, rs, tg, key);

Player_Initialize(player, 50, 270 / 2);

//弾の初期化
for (int s = 0; s < 10; ++s) { shots[s].state = State::Non; }
//敵の初期化
for (int e = 0; e < 30; ++e) {
	int x = (e % 6) * 40;
	int y = (e / 6) * 40;
	Enemy_Initialize(enemys[e], 160 + x, 40 + y);
}

}
//-----------------------------------------------------------------------------
//解放処理
//機能概要：プログラム終了時に１回実行される（素材などの解放を行う）
//-----------------------------------------------------------------------------
void MyGameMain_Finalize()
{
imgPlayer.reset();
imgShot.reset();
in1.reset();
imgEnemy.reset();
}
//-----------------------------------------------------------------------------
//更新処理
//機能概要：ゲームの１フレームに当たる処理
//-----------------------------------------------------------------------------
void MyGameMain_UpDate()
{
Player_UpDate(player);
//プレイヤ弾の行動
for (int s = 0; s < 10; ++s) {
Shot_UpDate(shots[s]);
}
//敵の行動
for (int e = 0; e < 30; ++e) {
Enemy_UpDate(enemys[e]);
}
}
//---------------------------------------------------------------------------
//描画処理
//機能概要：ゲームの１フレームに当たる表示処理 ２Ｄ
//-----------------------------------------------------------------------------
void MyGameMain_Render2D()
{
Player_Draw(player);
//プレイヤ弾の表示

for (int s = 0; s < 10; ++s) {
	Shot_Draw(shots[s]);
}
for (int e = 0; e < 30; ++e) {
	Enemy_Draw(enemys[e]);
}

}
//--------------------------------
//弾の出現処理
void Shot_Appear(int x_, int y_)
{
for (int s = 0; s < 10; ++s) {
if (shots[s].state == State::Non) {//空き領域を見つけたら
//弾を生成する
shots[s].state = State::Normal;
shots[s].x = x_;
shots[s].y = y_;
shots[s].hitBase = ML::Box2D(-4, -4, 8, 8);
break;
}
}
}
//-------------------------------------
//プレイヤの初期化
void Player_Initialize(Chara& c_, int x_, int y_)
{
c_.state = State::Normal;
c_.x =x_;
c_.y = y_;
c_.hitBase = ML::Box2D(-32, -12, 64, 25);
}
//---------------------------------------
//プレイヤの行動
void Player_UpDate(Chara& c_)
{
auto inp = in1->GetState();
//プレイヤー行動
if (c_.state == State::Normal) {
if (inp.LStick.BL.on) { c_.x -= 3; }
if (inp.LStick.BR.on) { c_.x += 3; }
if (inp.LStick.BU.on) { c_.y -= 3; }
if (inp.LStick.BD.on) { c_.y += 3; }
if (inp.B1.down) {
Shot_Appear(c_.x + 20, c_.y - 10);
Shot_Appear(c_.x + 20, c_.y + 10);
}
}
}
//--------------------------------------------
//プレイヤの表示
void Player_Draw(Chara& c_)
{
if (c_.state == State::Normal) {
ML::Box2D draw(-32, -12, 64, 25);
draw.Offset(c_.x, c_.y);
ML::Box2D src(0, 0, 64, 25);
imgPlayer->Draw(draw, src);
}
}
//-----------------------------------------------
//敵の初期化
void Enemy_Initialize(Chara& c_, int x_, int y_)
{
c_.state = State::Normal;
c_.x = x_;
c_.y = y_;
c_.moveCnt = 0;
c_.hitBase = ML::Box2D(-20, -20, 40, 40);
}
//----------------------------------------------
//敵の表示処理
void Enemy_Draw(Chara& c_)
{
if (c_.state == State::Normal) {
ML::Box2D draw(-20, -20, 40, 40);
draw.Offset(c_.x, c_.y);
ML::Box2D src(0, 0, 40, 40);
imgEnemy->Draw(draw, src);
}
}
//--------------------------------------------
//敵の行動処理
void Enemy_UpDate(Chara& c_)
{

if (c_.state == State::Normal) {
	if (c_.moveCnt < 100) { c_.x += 1; }
	else { c_.x -= 1; }
	c_.moveCnt++;
	if (c_.moveCnt >= 200) { c_.moveCnt = 0; }
	//敵とプレイヤのあたり判定を行う（敵が主体）
	//自分(敵）の矩形を用意
	ML::Box2D me = c_.hitBase.OffsetCopy(c_.x, c_.y);
	if (player.state == State::Normal) {//相手（プレイヤ）の矩形を用意
		ML::Box2D you = player.hitBase.OffsetCopy(player.x, player.y);
		//接触判定
		if (you.Hit(me) == true) {
			c_.state = State::Non;
			player.state = State::Non;
		}
	}
}

}
//-------------------------------------
//弾の初期化
void Shot_Initialize(Chara& c_, int x_, int y_)
{
c_.state = State::Non;
}
//--------------------------------------
//弾の表示処理
void Shot_Draw(Chara& c_)
{
if (c_.state == State::Normal) {//有効になっている弾のみ
//弾の表示処理
ML::Box2D draw(-16, -16, 32, 32);
draw.Offset(c_.x, c_.y);
ML::Box2D src(0, 96, 32, 32);
imgShot->Draw(draw, src);
}
}
//--------------------------------------
//弾の行動処理
void Shot_UpDate(Chara& c_)
{
if (c_.state == State::Normal) {//有効になっている弾のみ
//弾の行動処理
c_.x += 4;
//弾が画面外に出たとき、弾を無効する
if (c_.x > 480 + 16) {
c_.state = State::Non;
}
//弾と敵の当たり判定を行う
if (c_.state == State::Normal) {
//自分（弾）の矩形を用意
ML::Box2D me = c_.hitBase.OffsetCopy(c_.x, c_.y);
//すべての敵と当たり判定を試みる
for (int e = 0; e < 30; ++e) {
if (enemys[e].state == State::Normal) {//敵が存在している場合のみ
//相手（敵キャラ）の矩形を用意
ML::Box2D you = enemys[e].hitBase.OffsetCopy(enemys[e].x, enemys[e].y);
//接触判定//自分と接触しているか相手に判断してもらう
if (you.Hit(me) == true) {
enemys[e].state = State::Non;
c_.state = State::Non;
}
}
}
}
}
}

Comments

[YAmaGNZ]

https://teratail.com/help/avoid-asking
コードをください・デバッグしてください等の丸投げの質問は推奨されていません。

[fana]

別質問が立った模様：
https://teratail.com/questions/302rgva0zcj0r0

Answer 1

「関数化」に関係しそうな話をちょっとだけ．

引数を変更しない場合，const を付ける．

//この関数は c_ の内容を変更しないという想定である．
//であれば，引数の型は Chara& ではなく const Chara& とする．
void Shot_Draw( const Chara& c_ )
{
  //想定と異なる（c_ を変更してしまうような）コードをうっかり書いた場合，
  //コンパイルエラーになる．（→ので，そこでミスに気付くことができる）
}

早期return という考え方がある．

「ある条件のときにはもうこれ以上やることが無いという場合，その場でreturn する」という形で書くと，
処理を読む際に処理フローがわかりやすい（という意見がある）．
あと，とりあえずネストが減る分だけ見やすい．
（ただし，このような，関数の出口が複数になる形の実装を否定する流儀もある）

void Enemy_UpDate(Chara& c_)
{
	//c_.stateがNormalでないならばこの関数では何もしない
	if (c_.state != State::Normal)return;

	if (c_.moveCnt < 100) { c_.x += 1; }
	else { c_.x -= 1; }
	c_.moveCnt++;
	if (c_.moveCnt >= 200) { c_.moveCnt = 0; }
	
	//敵とプレイヤのあたり判定を行う（敵が主体）

	//player.state が Normal でない場合，この関数ではこれ以降やることはない
	if (player.state != State::Normal)return;

	ML::Box2D me = c_.hitBase.OffsetCopy(c_.x, c_.y);	//自分(敵）の矩形を用意
	ML::Box2D you = player.hitBase.OffsetCopy(player.x, player.y);	//相手（プレイヤ）の矩形を用意
	//接触判定
	if (you.Hit(me) == true) {
		c_.state = State::Non;
		player.state = State::Non;
	}
}

関数の分け方とか

↑の Enemy_UpDate 関数で言えば，
//敵とプレイヤのあたり判定を行う（敵が主体） というコメント行の前後で２つの関数に分けることも考えられる．

Comments

[fana]

あと，「関数化」とは無関係だけど，

> //敵最大３０

この 30 みたいな値をコード各所に散りばめない方がいいんじゃないかな．
これだとこの最大数を変更しようと思ったら各所を書き換えて回る作業が大変だろうし（修正漏れがあるとバグるし），
後から（忘れた頃に）コード読むときに「この唐突に現れた 30 っていう値の意味とは何なの？」とかなる可能性もあるし．

というわけで，
こういうのは「敵の最大数」を表す定数を用意して，各所はその定数を参照するようにすると良いであろう．
（この定数の値を変更するだけで，プログラム全体が変更後の敵最大数で適切に動作するように実装しておく）

Answer 2

※まず，他者に「コードを見てくれ」と言うならば，相応に見やすい形で提示することを心掛けるべきではないでしょうか．
現状のぐちゃぐちゃの表示では見る気もしません．
なので，コードをきちんと見ずに以下を答えます．

---

プロトタイプ宣言、関数の呼び出し、関数本体の定義等、しっかり関数化できているか見てほしいです。関数化する前と同じ動作はできています。

「動作できている」ということですから，少なくとも必要な宣言等は存在しているし，
「関数化する前と同じ動作」が達成できているならば，とにかく処理実装は間違っていないということでしょう．

ぱっと見で，

void Shot_Initialize(Chara& c_, int x_, int y_)
{
  c_.state = State::Non;
}

みたいな使われてない引数が存在していたりするのが見えたりしますが……

そもそもそれらの関数が「一体何をするものなのか，引数はそれぞれ何なのか」といった情報が全くありませんので，他者には意味不明です．
（なので，今現在の実装の妥当性の判断みたいなのは他者にはできなんじゃないかな）

このコードがまるごと単一の翻訳単位(.cppファイルとか）なのであれば，
主要な変数群がことごとく外部変数になっている時点で「関数化してみたけどどうか？」とかいわれても，その「関数化」なる作業の目的と言うか，何をどこまでやるのが今現在のあなたのゴールなのか？　みたいな点がこちらにはわかりません．
しかしあなたは「今行った作業（関数化）によって何がどう改善されるという想定なのか？」というのを知っているわけですから，それと照らし合わせてみれば「うまくいってるのかどうか」をご自身で判断できるのではないでしょうか．

関数の実装を別々の翻訳単位に移動させても大丈夫な（分割コンパイルできるような）形というのを目指してみるのが良いのかもしれません．

Comments

[fana]

なんというか，
実装の仕方を何やかんやと変えても「同じ動作」なのであれば，
処理の実装（というか実現というか）は１ミリも前に進まないわけで，
そしたら　そのコードをいじくりまわす作業によって一体何が進んでいるのか？　その作業のゴールとはどこなのか？　っていう話がある気がするんですよね．

「関数化」という作業の名称(?)だけ言われても，どこが目指すべきゴールなのかがわからない．
「今はとりあえずここはこのままにしておこうと思ってるんで」みたいなのとかがあるでしょうし？

[fana]

> 関数化する前

の状態には，何らかの　解決すべき課題/問題点　が存在するのだと認識していて，それを解消するために「関数化」という作業を行っているのだ

…といったような話があるハズなので，その　解決すべき課題/問題点　というのを他者と共有しなければ，話ができないんじゃないのかな？　とか．なんかそんな．

[lx.qz9]

無知でごめんなさい、。
丁寧にありがとうございます。
すいませんでした

[fana]

単に「何の話をしたいのか？　というところの説明が不足しているのでは」と言っているだけですぞ．

最初にこういうコード書いてたんだけどー
これだとこういうところが良くないと思うんすよねー
で，関数化したらそこがいい感じになるかなって思って書き変えてみたんすけどー
これで「良くない」点を解消できる感じですかね？

…くらいの話が成されていれば，とりあえず何の観点での話をすればよいのかが読み手に分かるので（誰かが）回答することも可能になるんじゃないですか？　っていう．

それに対して突然

> 無知でごめんなさい

とか言われても，どんな反応なのかわかりませんぞ……
（知識の有無を問題視するような話はしてませんし？）

[fana]

たとえば…

void Enemy_UpDate(Chara& c_)

という関数は，更新対象を引数で指定している形だけども，
それとは別に player という外部変数の値を参照したり書き換えたりもしてるよね．

関数内で更新する対象というのが２つあるのだとして
｛一方は引数で指定し，もう一方は指定しない｝という形にしてあることに関して
・何故なのか？
・その形で良いのか？
みたいな背景事情的な話というのは，あなたしか知らないことなのだし，あなたが全く自由に決めてよいことでもある．

「player を外部変数として，プログラム全体で読み書きするのだ」と決めているのであれば，このコードはその想定通りの実装であるという意味においては「全く問題が無い」と言える．
逆に「外部変数をいろんな箇所で読み書きするというスタイルはどうなのか？」みたいく考えているのであれば，「改善すべきと考えているけども改善できてない点が残っている」と言える．

前者の形を想定しているのであれば
void Player_UpDate(Chara& c_)
とかは逆に「ここ引数で指定する必要なくない？」みたいな話になるのかもしれないし，
後者の形を想定しているのであれば
「void Enemy_UpDate(Chara& c_) には player も引数として指定すべきではあるまいか？」という話になるのかもしれない．