移動

[meigin]

こんにちは

座標で考えてみました。

可読性って重要ですね。
でも、自分の書いたのが他の人が見ても可読性があるのか
心配なんですよ。

改善した方が良いよって指摘して下さると嬉しいです。


#include <windows.h>
#include <stdio.h>
#define MAX 20

//空白の挿入
void kouran(int i){
for(int ix = 0; ix < i; ix++){
::printf(" ");
}
}

//改行
void jyouge(int i){
for(int iy = 0; iy < i; iy++){
::printf("\n");
}
}
//向き
enum direction{
top = 0, //上
bottom = 1, //下
right = 2, //右
left = 3, //左
};
//キャラクター
void kyara(int x, int y, direction z){
char ckya[4][3][8] = {
{"　↑　\n", "↑↑↑\n","　↑　\n"},
{"　↓　\n", "↓↓↓\n","　↓　\n"},
{"　→　\n", "→→→\n","　→　\n"},
{"　←　\n", "←←←\n","　←　\n"},
};

//表示出来る範囲内か？
const int Y_SIZE = 3;
if(y + Y_SIZE < 0 || y - Y_SIZE > MAX){
return;
}
const int X_SIZE = 6;
if(x + X_SIZE < 0 || x - X_SIZE > MAX){
return;
}

//キャラの表示範囲を決定する
int icy = 0;
int i_end_y = Y_SIZE;
if(y < 0){
icy = ::abs(y);
}else if(y > MAX){
i_end_y = Y_SIZE - (y - MAX);
}

int icx = 0;
int i_end_x = X_SIZE;
if(x < 0){
icx = ::abs(x);
if(icx % 2){
icx--;
}
}else if(x > MAX){
i_end_x = X_SIZE - (x - MAX);
if(i_end_x % 2){
i_end_x--;
}
}

::jyouge(y);


for(int iy = icy; iy < i_end_y; iy++){
::kouran(x);
for(int ix = icx; ix < i_end_x; ix+=2){
char c[3];
c[0] = ckya[z][iy][ix];
c[1] = ckya[z][iy][ix + 1];
c[2] = '\0';
::printf(c);
}
::printf("\n");
}

}
int main(void){
//座標と向き
int x = 0, y = 0;
direction z = ::bottom;

//入力されたコード
int i = 0;

//見本として表示
::kyara(x, y, z);

while(::printf("上:8 下:2 左:4 右:6 終:0\n"), ::scanf("%d", &i), i != 0){
::system("cls");
if(i == 8){
y--;
z = ::top;
}
if(i == 2){
y++;
z = ::bottom;
}
if(i == 4){
x--;
z = ::left;
}
if(i == 6){
x++;
z = ::right;
}
::kyara(x, y, z);
}
return 0;
}

[管理人]

特に読みにくいことは無かったですよ。
しかし、３次元配列を使用するとちょっとパッと見ではわかりにくくなりますね。２次元配列にすると条件文が増えるので、どっちがいいということはないと思いますけど。

http://dixq.net/g/#22

こんなアニメーションに似てますね。

[Justy]

＞可読性って重要ですね。
　重要ですね。
　でも難しいです。自分にとって読みやすいのと他人にとって読みやすいのは大きく違うことが多々ありますから。
　シンプルに書くのが読みやすいという人もいればシンプルすぎて判らないという人もいますし、関数化して処理の流れを追いやすくしたら、今度はデバッガで追いにくいという人もいたりして・・・。
　
　それに、ソースコードの品質としてその他の要素・・・保守性、柔軟性、移植性、再利用性、理解性・・・とかと時にはぶつかってしまい、可読性を捨てなければならないこともあったりして。


＞改善した方が良いよって指摘して下さると嬉しいです。
　可読性という観点から見た場合、適宜関数化もされていますし概ね処理もシンプルで流れはとてもわかりやすいです。
　定数にきちんと constつけているあたりも素晴らしいです。

　強いて難点を上げるとすれば関数名や変数名の命名でしょうか。
　このあたりは好みとかあるので一概には言えないのですが、もう少し判りやすい名前にした方がいいかと思います。
　例えば「jyouge(上下)」とかは何をする関数なのか今ひとつ見えてこない名前である気がします。
　kyara()をざっと上から見ていくと
　　表示範囲のチェックをして、範囲を決定して、上下・・・上下？　ってかんじになりません？
　コメントで明確に意図を書くという手でてもいいのですが、関数名を「改行を追加する・表示する、カーソル行を設定する」とかそういう類の意味の関数名をつけるとぐっと良くなります。

　変数名の方は iの使われ方と direction型を zとしているのが気になりました。
　iは一定回数繰り返すループ内のどーでもいいような局所変数として利用するような場合はともかく、関数の引数だったりとか、mian()で疲れているような「入力された数値」というような意味を持っているものならば、その意味に合った名前を付けた方がいいと思います。

　マクロ MAXも同様で、今はいいのですがこのプログラムを今後拡張するにあたって、何かの最大値というのが複数登場した場合に困る名前です。
　SCREEN_WIDTH_MAXとか、DISP_X_MAXとか何か別の単語と組み合わせておくといいかと思います。


　さて、以下蛇足です。
　今は ENTERキーもって入力が終わった、として処理をしていますが、これをキーが入力された即座に反応するようにしてみてはどうでしょうか。
[color=#e0f0ff" face="sans-serif]
#include <conio.h>
[/color]
　とした上で

[color=#e0e0ff" face="sans-serif]
do
{
printf("上:8 下:2 左:4 右:6 終:0\n");
while (!kbhit())
Sleep(10);

i = getch();

if(i == '8'){
....
if(i == '2'){
....
}while(i != '0');
[/color]

　とするとキーボードの入力があった時だけ反応するようになるので、レスポンスが良くなりますし、scanfを使った場合の独特のエラー処理の面倒さ（例えば "2d"と入力された場合とか）に悩まされずに済むかと思います。

[GPGA]

指摘しても良いといわれておりますので、指摘させさせていただきます。
実は私、プログラムを添削するのがと～ても好きなんです。

ただし、私の添削には独断と偏見がかなり入るらしいので、私の考えに疑問を
持つことがあったらどんどんいってください。


まず、可読性ですが
while(::printf("上:8 下:2 左:4 右:6 終:0\n"), ::scanf("%d", &i), i != 0){
　　・・・
}

さすがにこれは読みにくいですね。
私なら、以下のように記述します。
while (true) {
　　::printf("上:8 下:2 左:4 右:6 終:0\n");
　　i = getch() - '0';
　　if (i == 0) {
　　　　break;
　　}

　　・・・
}

getchはconio.hをインクルードすることにより、使用することができます。
今回のプログラムで使用すると、ちょっと幸せになれるかもしれません。


#define MAX 20
についてですが

他の部分で、定数を
const int Y_SIZE = 3;
のように const を使用しているのに、MAXだけdefineなのは一貫性がありませんね。

static const int MAX = 20;

と置き換えたほうが良いと思います。



//向き
enum direction{
　　top = 0,　　　//上
　　bottom = 1,　//下
　　right = 2,　　//右
　　left = 3,　　//左
};
明示的な初期化というのは、わからなくもないのですが
enumのすべてメンバに対して、値を入れるというのは、やりすぎではないかと思います。
ただ、今回のプログラムの使用方法を見ると賛否両論に分かれると思いますので、強くは言いません。


最後にC++で記述していることなので、言うことなのですが
関数の引数がない場合、voidをつけないほうがC++的な記述らしいです。

余談ですが、C++の場合、関数の引数に何も書かないとvoidとしてコンパイルされますが
Cの場合、関数の引数に何も書かないと int としてコンパイルされます。
以下のプログラムをコンパイルした場合、C++だとエラーがでますが、Cだとエラーになりません。

void sub();

int main()
{
　　sub(1);
　　return 0;
}

void sub(int a)
{
}

[GPGA]

ぐは、getch先に言われてしまった。
ついでに、ポイントを落としてしまった。

[管理人]

＞ぐは、getch先に言われてしまった。

ほぼ同時でしたね＾＾；

＞ついでに、ポイントを落としてしまった。

３倍ゲットする時もあるので…

[Justy]

＞ぐは、getch先に言われてしまった
　すごい僅差な時間差ですね。
　しかも考えることも同じ（ｗ

＞ついでに、ポイントを落としてしまった。
　これだけの分量書いて落ちるとは・・・・。

[meigin]

Justyさん GPGAさん　管理人さん　

ありがとう

getch();
知りませんでした。
返す値が数値だと思っていたら、文字だったんですね。
動かないと思っていたら''が必要だとわかり
すぐに動くようになりました。
Enterを押す面倒が無くなって楽になりました。


変数名とか関数名とかちゃんと考えた方が良いですよね。
今度からもうちょっと考えることにします。


while (true)はfor(;;)の方が好きなんで使ったことがないんです
今回はdo{}while (式);の方が良さそうなので、
こちらを使うことにしました。
無限ループはifを１つ無駄にしているような気がするんですよ

定数とマクロの混在はダメですか。
これに代入とかしないので、統一する必要がないと思うのです。

マクロはグローバルな時に使って、
定数はローカルな時に使うようにしているのです。

マクロを安易に定数へ書き替えると
#if defined (MAX)
#endif
が存在すると、この中身が無視されてしまいます。

今回はたまたま、一部の関数でしか使いませんでしたが、
mainでも使うかも知れないと思ってマクロにしたのです。

voidは書かないのが一派的ですか。
いちいち書く必要があるのかと思っていました。
参考になりました。

[GPGA]

> [color=00B0FF">while[/color] ([color=00B0FF">true[/color])は[color=00B0FF]for[/color](;;)の方が好きなんで使ったことがないんです
> 今回は [color=00B0FF">do[/color]{}[color=00B0FF]while[/color] (式);の方が良さそうなので、
> こちらを使うことにしました。
私も、最初[color=00B0FF">do[/color] [color=00B0FF]while[/color]にしたのですが、そうしてしまうと
0で終了した際の挙動が、変わってしまうんですよね。
どちらにしても、[color=00B0FF]if[/color]文を使用しないと同じ挙動にならなかったので
あえて、[color=00B0FF">while[/color] ([color=00B0FF]true[/color]) を使用しました。


> 定数とマクロの混在はダメですか。
> これに代入とかしないので、統一する必要がないと思うのです。

> マクロはグローバルな時に使って、
> 定数はローカルな時に使うようにしているのです。
そもそも定数とマクロを同じものとして考えてはいけません。
C++でこそ、[color=00B0FF">const[/color] [color=00B0FF">int[/color] は [color=FF0000">定 数[/color] なりますが、Cの時代 [color=00B0FF">const[/color] [color=00B0FF]int[/color] は
値を変更できない [color=FF0000]変 数[/color] でした。
以下のプログラムを見てください。

[color=00B0FF">const[/color] [color=00B0FF]int[/color] MAX = 10;

[color=00B0FF]int[/color] main()
{
    [color=00B0FF]int[/color] ary[MAX];

    [color=00B0FF]return[/color] 0;
};

上記のプログラムはC++ではコンパイルが通りますが、Cではエラーが出ます。
Cでは、上記のことがあったため [color=FF0000">し か た な く[/color] 定数に[color=00B0FF]#define[/color]を用いていたのです。

[color=00B0FF]#define[/color]はそもそも使用するべきではありません。その効力が強力すぎるからです。
[color=00B0FF]#define[/color]は名前空間の壁を越えて、全てを置き換えてしまいます。

以下はその一例となります。あ、この例では名前空間は関係ないです。

[color=00B0FF]#define[/color] ChangeMode		20

[color=00B0FF">void[/color] ChangeMode([color=00B0FF]int[/color] mode)
{
}

これをビルドした場合、プリプロセッサが関数名が置き換え

[color=00B0FF">void[/color] 20([color=00B0FF]int[/color] mode)
{
}

この、記述になった後にコンパイルが行われエラーが起こります。
なお、VC++6.0では「C2059: 構文エラー : 'constant'」とでました。 エラーが出ている行に飛んでみると

[color=00B0FF">void[/color] ChangeMode([color=00B0FF]int[/color] mode)

と記述されています。何のエラーだかさっぱりわかりません。
次に、ChangeModeを[color=00B0FF">#define[/color]ではなく[color=00B0FF">const[/color] [color=00B0FF]int[/color]に置き換えてコンパイルしたところ
「error C2373: 'ChangeMode' : 再定義されています。」とでました。
どこかでChangeModeを定義されていることが一発でわかります。

もちろん、名前を大文字で統一するなど、自分なりの何か規約を決めれば回避できないとはいえません。
しかし、それはあくまで自分ひとりでプログラムを組む場合で、さらに規模が小さいプロジェクトでしょう。
ある程度の規模になってしまうと、このようなことが起こりえないとはいえません。
結論としては、このような問題が発生する可能性を残すこと自体が、間違っているということです。

ただし、わたしとしてもまったく[color=00B0FF]#define[/color]を使用しないわけではありません。
Debug時とRelease時での_DEBUGマクロでプログラムを区切ったりすることは多々あります。
引数つきマクロも、本当は良くありませんが、時々使用します。(個人プロジェクトもしくは、ライブラリ内で使用するもののみ)
_DEBUGマクロくらいでしたら、誰でも知っている範囲ですので使用しても問題ないと思っています。

[meigin]

>私も、最初do whileにしたのですが、そうしてしまうと
>0で終了した際の挙動が、変わってしまうんですよね。

違いが解らないのですが、宜しければ教えて下さい。


定数では同じ名前を使えないと思うのです。
広範囲に渡って変更してしまうのが
嫌なら制限すれば良いと思います

#define MAX 100
int Max_a(){
return MAX;
}
int Max_b(){
return MAX + MAX;
}
#undef MAX

#define MAX 200
int Max_c(){
return MAX;
}
#undef MAX

間違える可能性があるのなら、
コメントで補足しておけばいいと思います。

全く確認もせずに使うというとは無いと思います。

全て大文字で関数を書くのは少ないと思います。
なので、そんなエラーを見かけるのは珍しいと思います。

[GPGA]

> 違いが解らないのですが、宜しければ教えて下さい。
meiginさんや私が提示した [color=00B0FF]while[/color] を使用したプログラムでは0を押した瞬間、ループから抜けるため
main関数内の ::system("cls"); ～ ::kyara(x, y, z);が実行されませんが
[color=00B0FF">do[/color] [color=00B0FF]while[/color]を使用したプログラムでは、0を押した後に、上記の範囲のプログラムが実行されます。


> 定数では同じ名前を使えないと思うのです。
グローバル領域の定数に同じ名前を使ってはいけないと思いますが。
meiginさんのプログラムは、最初に[color=00B0FF]#define[/color]でMAXを100と定義した後
一度#undefをして再び、MAXに200を入れていますが、つまりこれを行った時点で
MAXの値が変更されているわけですから、値が変更されて瞬間、MAXは定数ではなく変数ということになりますよね？

異なるオブジェクト間で、同じ名前の定数を使用する場合は、クラス別に分ければ問題ありません。

[color=00B0FF]class[/color] A
{
[color=00B0FF]public[/color] :
	[color=00B0FF">static[/color] [color=00B0FF">const[/color] [color=00B0FF]int[/color] MAX = 100;					// クラスAの、ある値の最大値
};

[color=00B0FF]class[/color] B
{
[color=00B0FF]public[/color] :
	[color=00B0FF">static[/color] [color=00B0FF">const[/color] [color=00B0FF]int[/color] MAX = 200;					// クラスBの、ある値の最大値
};

[color=00B0FF]int[/color] main()
{
	[color=00B0FF]int[/color] a = A::MAX;
	[color=00B0FF]int[/color] b = B::MAX;

	[color=00B0FF]return[/color] 0;
}

上の書き込みで言いました、名前空間の壁を越えずに、定数を使用できるというのは、上記のようなことを示します。
しかし、グローバル領域にある定数が完全に悪いとはいいません。
私のゲームプログラムの話になりますが、画面の幅と高さなどそのプログラム全体の
基底にあるものは、グローバル領域に定数を定義しています。

[meigin]

>do whileを使用したプログラムでは、0を押した後に、上記の範囲のプログラムが実行されます。

そう言うことですか、書き方を変えたら中身も変えるのが普通だと思います。
なので、無限ループ&&ifを使わなくても同じように作動しましたよ。


普段はマクロとか定数は殆ど使ってなかったです。

ライブラリとか、付属してあるｈファイルとかには、
マクロが使われていて、その方が良いのかなと思いこんでいました

私は動けばどちらでも構わないのですが……。

クラスにするぐらいなら、名前空間の方が分かり易く良さそう。
namespace A{
const int MAX = 100;
}
これでもA::MAX;で使えますよね。

クラスは好きではないのです。
メンバ変数を持たないクラスは
ただの関数でいいやと思っているほどです


信用してマクロは使わない。
定数は名前空間で。

参考になりました。
ありがとう御座います。