評価をください

[電気屋]

前回、この掲示板でプログラムの構造について質問した電気屋です。
そのときの中にプログラムを見せてくださいとあったので、バグをっとて一応動く形にしたので評価をください。
////////////////////////////
今回の目的:
プログラムを見てもらい自分のレベルを知ること


今回特に見てもらいたこと:
現段階であまり好ましくない（無駄なプログラムまたは回りくどいなどなど）所を書いてください。

プログラムを見ての率直な感想


今回は、まだ勉強中の身なので分割（プロトタイプ宣言も含む）はできず、長ったらしい文になっていますがそこは勉強中なのでお願いします。
またこのプログラムは基盤にするつもりなので、ひつ方最低限しか書いていません（自分の中の最低限）

プログラム内容：
シューティングゲーム（全方向型）
S KEY 時計回りに回る
A KEY 反時計回りに回る
Z KEY 発射

[あたっしゅ]

VC++ 2010 Express 使ってコンパイルしてみた。

#define とかあるから、.c でコンパイルしたら、DxLib の中に namespace があったので、エラー。
.cpp でコンパイルしなおした。

DrawString のところ(　2 箇所 )、文字列を _T() でくくったりしたけど、なんかリンクエラー出るので、
注釈つけてみたら、動いた。


で、本題。

TAMA と tama、RANGE と range のような、大文字と小文字が違うだけ、というのは、よくない。
TAMA のほうは TAMA_MAX かねえ。RANGE は、よくわからない。

struct shot の中x, y は、グローバルな x, y とまぎわらしいか ? _x, _y の方がマシか ? class の中なら m_x, m_y だが。
flag という名前のフラグは、駄目。flagActive かねぇ。

次に、
///////////////////////////////
///////////////////////////////
//メインプログラム
///////////////////////////////
///////////////////////////////
///////////////////////////////
int WINAPI WinMain( HINSTANCE hInstance, HINSTANCE hPrevInstance,LPSTR lpCmdLine, int nCmdShow ){
if( ChangeWindowMode(TRUE) != DX_CHANGESCREEN_OK || DxLib_Init() == -1 ) return -1;
SetDrawScreen( DX_SCREEN_BACK ) ;



///////////////////////////////
//初期化
///////////////////////////////
int UP =0;
int LEFT =0;
int DOWN =0;
int RIGHT=0;
int A =0;
上記のように、注釈も字下げした方がいい。
行頭から注釈始まると、そこで関数が別れていると思われる。

あと、円周率は M_PI。math.h で定義されている。くわしくは、俺の URL にジャンプ。

[釣り師]

////////////////////
や、
/****************/
で区切られているのはいいのですが、これが多用されていると分かりづらい、見づらい印象を受けます。
あと、せっかく細かい注釈を入れているのですから、それぞれ#defineで定義したときに注釈を入れておくのがいいと思います。

[MNS]

特に大きな問題はないと思います。
本来であれば、幾つかの関数に処理を分けるべきですが、
それはまだこれから、なんですかね。

強いて言うならば、変数名は意識すると良いと思います。
特に、このプログラムには、
グローバル変数としての、main関数のローカル変数としての、構造体の変数としての、
x, yという変数が登場しており、区別がつきません。

構造体の変数としては、変数名はx,yで良いと思いますが(_x,_yは良くありませんよ)
グローバル、ローカルでは区別を付けるべきでしょう。
用途はそれぞれで異なると思いますから。
（今回のプログラムでは、グローバル変数のx,yは使用していないのでしょうか？）

[白い時空]

///////////////////////////////
//変数
///////////////////////////////
int muki;
int x,y,i;
int counter;
int range;
///////////////////////////////

///////////////////////////////
//初期化
///////////////////////////////
int UP =0;
int LEFT =0;
int DOWN =0;
int RIGHT=0;
int A =0;
int S =0;
int X =0;
int Y =0;
int Z =0;
int muki =0;
int x =0;
int y =0;
int counter=0;
int range=0;
///////////////////////////////

ここで、muki,x,y,counter,rangeが複数宣言されています。
片方必要ないですよね？


KEY入力部分では
if( Key[ KEY_INPUT_UP ] == 1 )UP =1;
if( Key[ KEY_INPUT_UP ] == 0 )UP =0;
こんな感じになっていますが、
UP = Key[ KEY_INPUT_UP ];
でいいのではないでしょうか？


あとは、
UP～ZまでのKEY入力の変数はまとめてKEY入力の構造体に、
muki,x,y,rangeもまとめて自機の構造体にしたほうがいいと思います。

[dic]

簡素でとてもわかりやすいプログラムだと思います

ですが、一点だけ
グローバル変数とわかるような変数名だといいですね

[ookami]

ESCキーで終了する処理が、

while(!ProcessMessage() && !ClearDrawScreen() && !GetHitKeyStateAll( Key ) && !Key[KEY_INPUT_ESCAPE]){

と、

if( Key[ KEY_INPUT_ESCAPE ] == 1 ) break;

の2箇所に書かれていますので、
片方いらないかと。

あと、sin, cos の中で int÷int の部分で、
小数点以下が切り捨てられる誤差が生じています。
上下左右だけなのであまり気にならないですが、
将来的に、斜めとかやりたい場合は、改善しておいた方がよいと思います。

[softya]

前回の質問を貼っておきます。
http://www.play21.jp/board/formz.cgi?ac ... 1286861566

で前回の話も絡めて見させていただきました。
私のなりの感想を書かせていただきます。

・コメントも、もう少しシンプルで良いというか重要度で重み付けをしてください。
あと判定とか操作とかありますが、何の判定なのかとか何の操作とかを書かれたほうが良いと思います。
・UP/LEFTの同時押しなどで√２倍速で進むので考慮されたほうが良いと思います。
・自機のパラメータも構造体にまとめるとすっきりします。
・関数分けを行うとスッキリすると思います。
・640とかの定数値もdefineすると変更に強くなります。

[電気屋]

評価ありがとうございます。
早速、変更または改良をしました。
まず、移動の方法を下記のように変更しました。
if(UP==1){
if(LEFT ==1){
ch.y-=(C_SPEED/(double)(sqrt(2.00)));
ch.x-=(C_SPEED/(double)(sqrt(2.00)));
}
else if(RIGHT==1){
ch.y-=(C_SPEED/(double)(sqrt(2.00)));
ch.x+=(C_SPEED/(double)(sqrt(2.00)));
}
else ch.y-=C_SPEED;
}

else if(DOWN ==1){
if(LEFT ==1){
ch.y+=(C_SPEED/(double)(sqrt(2.00)));
ch.x-=(C_SPEED/(double)(sqrt(2.00)));
}
else if(RIGHT==1){
ch.y+=(C_SPEED/(double)(sqrt(2.00)));
ch.x+=(C_SPEED/(double)(sqrt(2.00)));
}
else ch.y+=C_SPEED;
}
else if(LEFT ==1)ch.x-=C_SPEED;
else if(RIGHT==1)ch.x+=C_SPEED;

いかがでしょ？評価お願いします。

[softya]

ちょっと複雑になっちゃいましたね。
ヒントとしては、移動ベクトルテーブルを作ってUP,DOWN,LEFT,RIGHTをうまく添字にして移動ベクトルテーブル参照にするとすごくすっくりします。
typedef struct tMoveVect {
double x;
double y;
} tMoveVect;
tMoveVect MoveVect[16] = {
{ 0.0, 0.0 },
後は考えてみてください。
};
　int index = (UP*8) + 考えてみてください。
ch.x += moveVect[index].x * C_SPEED;
ch.y += moveVect[index].y * C_SPEED;
こんな感じに。
これだけが正解じゃないですがひとつの方法と言うことです。