C言語ゲーム製作途中（勉強中）

質問

プレイヤーが画面を四方八方に移動まで。コードを一通り見ていただきたいです
現在、定番で見やすいプログラムを意識しています。又、今後質問が宜しく願います

以下よりダウンロードです（実行ファイル、ソースコード(234行)等）
http://gamdev.org/up/img/10376.lzh

環境
OS:VISTS、統合開発環境:VC++2005 EE、言語:C
ライブラリ:DXライブラリ、画像エディタ:ペイント、
自動作曲:Random various music v1.5、MP3変換ソフト:iTunes

聞きたい優先順位（下に行くほど無視してもよいです）
(1)
関数名（頭は大文字）、変数名(全て小文字、アンダーバー使用)、グローバル変数名(g_をつけたほうがいいのか…)、
などの区別化
関数分け（他の方法）、マクロ定義名（名称つけ方がよくない等）
(2)
関数ごとのヘッダコメント。用途と引数くらいは書いたほうがいいのか
(3)
ウインドウのアクティブ、非アクティブで一時停止と再生の処理の仕方
動作はうまくいっているようですがやり方は正しいのか

DXライブラリを使っていますが、コメントで大体理解できると思います
他に気になった点、こうしたほうがいい等、色々な意見を願います　<(_ _)>

回答No.2

precog · ベストアンサー

まず一般論。

・コードを見てわかるような事をコメントする必要は無い。
・名前もコメントの一部。
・書法に頼りすぎりるとバグを見逃す。(g_がついてないのでローカルだと思い込んでいた等)
・コメントはコードを読むときの手助けになるような、より上位の概念を中心に書く (コードを読むときの前提情報や、他のモジュールとのつながりを示唆するもの)

> #include <stdio.h> //fopen,fclose,fwrite
> #include <math.h> //sqrt

ここのコメントは有害。
まず、ソースの読解を助けていない。
必要なら呼び出し関数は機械的に検出可能である。
さらに、このコメントが正しいことを将来にわたって保証することはできない。(大いなる無駄な作業が発生する)

> //マクロ定義

このコメントも上記同様。
定数の定義はマクロという意識は通常無い。

> #define DIV_IMAGE_NUM 24 //画像分割数
> #define IMAGE_X_SIZE 32 //画像サイズ
> #define IMAGE_Y_SIZE 32
> #define IMAGE_SIDE_NUM 6 //分割画像縦横の数
> #define IMAGE_LENGTH_NUM 4

ここのコメントも意味が無い。
定数名は、名が体を現していない点で表現として弱い。名前が属性しか表してない。ここだけ読んで何を決めているのかわかるだろうか?
説明が長くなるなら、プロなら設計書を書くので、それとリンクする名前にすればよい。

> #define P_START_POSI_X 290 //プレイヤースタートポジション
> #define P_START_POSI_Y 200

ここは固定で良いのか? ↓の画面サイズとリンクすべきでは?
これは書法というよりバグに近い。

> #define DISP_MODE_SIZE_X 640 //ウインドウ画面モードサイズ
> #define DISP_MODE_SIZE_Y 480

> //構造体定義

構造体の定義をしているのは見ればわかる。なんの構造体を定義しているのか等を書かないと意味が無い。

> struct player
> {
> int px; //x位置 0-35(4byte)。小数点単位で移動

バイト数は通常処理系依存。こう書かれると4byteでないといけない理由があるのかと心配すると同時に、並び順にも依存があるのではないかと疑いたくなる。
整数なのに、「小数点単位で移動」というのも意味がわからない。(実数でも意味がわからないですけど)

> int py; //y位置 0-17(4byte)
> int pspd; //スピード 1-2(4byte)
> int direc; //向き、方向

向きは、マジックナンバーになるので定義が必要になるのではないか?

> //float ptx; //弾x位置 0-35(4byte)
> //float pty; //弾y位置 0-17(4byte)
> //char ptflg; //弾発射フラグ 0-1(1byte(アライメントのためパディング効果4byte))
> };

このパディングは恐らく意味が無いが、そもそも何故必要なのか?
バグの予感がする。

> //グローバル変数
> int m_count = 0; //ゲームが終了するまでカウント
>
> int ikey_up = 0; //上キーフラグ
> int ikey_down = 0; //下キーフラグ
> int ikey_left = 0; //左キーフラグ
> int ikey_right = 0; //右キーフラグ
>
>
> //プロトタイプ宣言

通常ソースの中にプロトタイプは書かない。子を上に書くか、外部から呼び出しがあるならヘッダーファイルに含めることで兼用可能。

> // プログラムは WinMain から始まります
> int WINAPI WinMain( HINSTANCE hInstance, HINSTANCE hPrevInstance,
> LPSTR lpCmdLine, int nCmdShow )
> {
> struct player ply; //XXXbyte、プレイヤー

このコメントも有害。

> InitFunc( &ply ); //初動時の初期化

関数の説明を呼び出し側で書くのはおかしい。このコメントも有害。
開発環境を使って読めば、すぐになんの関数なのかはわかる。

> if( DxLib_Init() == -1 ) return -1; // ＤＸライブラリ初期化処理、エラーが起きたら直ちに終了

読めば分る。有害なコメント。

> int strclr = 0;

処理の途中で宣言するのはあまり良い習慣ではない。通常、Cのプログラマーはローカル変数は関数の先頭に書かれる物と思っている。バグ見逃しの可能性が高い。

> //今回のように、ループ処理には必要。ProcessMessage 関数はＤＸライブラリが
> //Windows というマルチタスクＯＳ上で動作するために必要不可欠な関数です。-1ならループを抜ける
> //システムから送られてくる指令や情報を常に監視し、それを処理しなくてはなりません
> //ProcessMessage 関数をただ実行するだけです。OSから送られてくる情報を処理して帰ってきます。
>
> //ウインドウがアクティブではない状態でも処理を続行するか、フラグをセットする

メインの処理は説明が多くなりがちなのは仕方なく、書くべきかどうかは微妙だが、ここがイベントループであるという程度のコメントで十分伝わるのでは?

メインの中に長く書きすぎ。初期化とメインと終了処理の三つに分解して関数にしたほうが見通しがよくなる。

> //キー入力
> void KeyBoard( void )

キーをグローバル変数で共有すると、デバイスに依存したコードがあちこちに散らばることになる。一旦、内部メッセージに置き換えたほうが変更/拡張しやすいブログラムになる。

> //プレイヤー
> void PlayerSyori( player *ply, int *pkenGraph, int *strclr )
> {
> //１ボタン有効30回ループ分
> //6*4：0～5（正面）、6～11（右）、12～17（左）、18～23（上）
> //0～1（徒歩）、2～3（剣を出す途中）、4～5（剣を出した）
>
> // 左に移動
> if( ikey_left == 1 )
> {
> ply->px -= 2 ;
> if( ply->px < 0 ) ply->px = 0 ; // プレイヤーが画面左端に来たときの処理
>
> if( (m_count % 10) < 5 ) ply->direc = 12;
> else ply->direc = 13;
> }
> // 右に移動
> if( ikey_right == 1 )
> {
> ply->px += 2 ;
> if( ply->px > 640 - 32 ) ply->px = 640 - 32 ; // プレイヤーが画面右端に来たときの処理

マジックナンバーが入っている。なんのために定数定義したのか?

☆全体的に
マジックナンバーが多いです。全部定数定義にしましょう。メンテする人 (他人) は通常、関数の中を書き換えることなく変更したいのです。
エラー後処理が入ってない。練習としてはこれでよいが、安定したPgを作れるようになりたいなら、そこをきっちりコーディングできる必要がある。それと、CではなくC++でコーディングしたほうが、可読性 / メンテナンス性の高いコードが書けますけど、、、
これは先の話で良いですが、読み込むデータ数をハードコーディングするのはおかしいです。本来、提示されたデータから読み取るべき。恐らく、データ数があわないデータを与えられると暴走するのでは?

お礼

指摘を受けての今後の改善点。多くの意見を感謝です！

・自分のコメントのつけかた。不必要が多すぎ、コードないでの
　コメントの必要性を認識する。プログラムを書く上での思考レベルの修正。やはり必要最低限か…
・グローバル変数名のつけ方g_以外で検討してみる
・メンテナンスで将来保障できる柔軟なプログラムになるか
・定数名で名が体を現していない。わかりにくい。プログラム設計書とリンクする形でもOK
・画面サイズとプレイヤーの位置のリンク

・構造体の意識は大きく改善すること。コメントについては昔のが残っていました。すみません
　マジックナンバーというのは数字（定数）と理解しました
・プロトタイプは一つのファイルという前提があったのであえて入れていました。
・関数説明は本体で
・ローカル変数は関数先頭で
・ここがイベントループであるという程度のコメントで十分伝わるという方向で対応
・メインを関数３分割で

・＞キーをグローバル変数で共有すると、デバイスに依存したコードがあちこちに散らばることになる。
　＞一旦、内部メッセージに置き換えたほうが変更/拡張しやすいブログラムになる。
　どういうことか…もう一度コードを見直します。改善の方向で確認

・プレイヤー処理で＞マジックナンバーが入っている。なんのために定数定義したのか?

・＞☆全体的に
メンテを考えマジックナンバー（数字の定数）は全て定数定義にしたほうがいいのですね。了解です

　エラー処理を対応の方向で

・C言語がある程度できたらC++に移行したいです

・＞これは先の話で良いですが、読み込むデータ数をハードコーディングするのはおかしいです。
　＞本来、提示されたデータから読み取るべき。恐らく、データ数があわないデータを
　＞与えられると暴走するのでは?
・これはどういうことだろう。もう少し見直します。

ここまでコードの悪いところを全て教えてくれたと思います。…自分でもある程度意識はしていたが…
指摘内容がすばらしく個人的には、ありがとうポイント2000以上はあげてもいいくらいです。
もう、なんというか、ありがとうございました！

回答No.3

aris-wiz

プロでだと確かに突っ込み所は多いですが、
勉強中でこのレベルまで書ければ、
後はルールと慣れという感じがします。
がんがんコードを書いていくと良いと思います。

他の方が指摘している部分はある程度のけて、
こんな風にやるとすっきりするかも、というレベルで書きます。
列挙型を使うかと結構まとまりやすいかも。

例えば「独自型」とか
typedef enum {
　PLAYER_DIR_DOWN = 0,
　//間に追加すれば段階も分けられる
　PLAYER_DIR_RIGHT = 6,
　PLAYER_DIR_LEFT = 12,
　PLAYER_DIR_UP = 18,
}PLAYER_DIR;
int direc;//向き、方向
↓
PLAYER_DIR　direc;//向き
列挙型にすることでtypedefで型を作れる為、
direcにPLAYER_DIR型以外が代入されたら、
コンパイルの時点で警告を出したりすることができます。
注意が要るのは、ありふれた名前で定義すると、
他とかぶる可能性があること。

キーフラグがありますが、
ボタンが増えるといろいろ大変。
typedef enum {
　KEY_UP = 0,
　KEY_DOWN,
　KEY_LEFT,
　KEY_RIGHT,
　//ここに追加するとフラグの配列も勝手に増える。
　KET_MAX,
}KEY_IDX;
int KeyStat[KET_MAX];
KeyStat[KEY_UP]; //上キーフラグの参照
こんな感じでしょうか。C言語での連想配列風な感じです。

あと終了処理の関数はDxLib_End関数を呼び出しているだけですが、
これをやるのであれば、終了する前にやる処理をEndFunc関数で
書いて、DxLib_End関数はWinMainで呼んでも問題ないかなと思います。

移動量、せっかくスピードメンバがあるのにもったいない :-(

お礼

勉強中というか長い間やってはいます
コードをすっきりする列挙型（向き、キー）を使い慣れていないため
少し使い方を調べて使いたいと思います。

＞終了する前にやる処理をEndFunc関数で
＞書いて、DxLib_End関数はWinMainで呼んでも問題ないかなと思います。
その処理があれば対応します

＞移動量、せっかくスピードメンバがあるのにもったいない
どいうことか…見直して対応します
ありがとうございました。

回答No.1

morigann

はっきり言って、めちゃくちゃ良く出来てますね。
（質問者様は学生さんでしょうか？将来有望です）

で、解答ですが
１：関数名や変数名に「絶対」という名前の付け方はありません。
　　質問者様がルールを決めて付けられたら良いと思います。
　　今後、会社等でお仕事されるでしょうがその時はその会社のルールがありますし。
２：そうですね、概要説明、引数、戻り値、禁止事項、引数不正時の処理などぐらいは書いておいた方が便利かと思います。
３：これは自分には分かりません

以下、あげ足取りみたいな指摘事項ですが気付いた点を上げておきます。
４：PlayerSyori内で画面端判断にdefineを使用していないのは意図的？
　　（移動量の２もdefineにした方が今後便利と思う）
５：KEYを２つ同時押ししたら、押した人は違和感感じないか？（特に「上・下同時」「右・左同時」）
　　将来斜め移動のグラフィック作成時を考慮して今の作りになっている？
６：DrawFormatStringはPlayerSyoriの外の方が良くないかな？

お礼

返答ありがとうございます。学生ではないです。
それなりに、C言語は長く学んでいます。
そして、一つ形にするのも、ものすごく時間が掛かるのです。要領が悪いと思われ…
まだまだ未熟ですがゲームを通じて覚えようと思いました。

１、なるほど、そのシステムのルールで作ればいいのですね。
今回は自分でマイルールで！まぁ一般的にどうなのかなと思ったのです^^
２、概要説明、引数、戻り値、禁止事項、引数不正時の処理
あーこの辺が重要ですね。了解です

４、気づいていたのですが…保留にしていました
５、画像が上下を向いているのに斜めに動くというのはゲームでたまにあったりするのでいいかなと…
個人的には違和感ないですが指摘で他の人にはあるのか…と気づきました
６、そうですね。ゲームシステム残りの時間という感じで外の方がよいと判断しました。