龍神録プログラミングの館8章について

[koi]

勉強さしてもらってます。
//パッドとキーボードの両方の入力をチェックする関数
で配列とポインタを使ってかいてみたのですが短くなっている気がしません。
プログラムは初心者です。どこを直せばもっと短く、きれいになるか教えてください。

void GetHitPadStateAll(){
int i,PadInput,mul=1,*p;

PadInput = GetJoypadInputState( DX_INPUT_PAD1 );//パッドの入力状態を取得
for(i=0;i<16;i++){
if(PadInput & mul) pad.key++;
else pad.key=0;
mul*=2;
}
int q[/url]={configpad.left,configpad.up,configpad.right,configpad.down,
configpad.shot,configpad.bom,configpad.slow,configpad.start,
configpad.change};
p=&q[0];

int s[/url]={KEY_INPUT_LEFT,KEY_INPUT_UP,KEY_INPUT_RIGHT,KEY_INPUT_DOWN,KEY_INPUT_Z,
KEY_INPUT_X,KEY_INPUT_LSHIFT,KEY_INPUT_ESCAPE,KEY_INPUT_LCONTROL};
for(int i=0;i<9;i++)
input_pad_or_key(&pad.key[*(p+i)] ,CheckStateKey(s));
}

[Justy]

　利用規約を読んで、preタグを忘れずに。



＞どこを直せばもっと短く、きれいになるか教えてください

　プログラムは初心者です、という割には十分綺麗に短くまとまっていると思います。
　それでも更に短く綺麗にということであれば。


・　int iの宣言位置
　関数冒頭で iを宣言しているのに、２つ目の for文の中でも iを宣言しています。
　間違いではないですが、紛らわしいので、for文の中だけで宣言するか、外で１つ
宣言して使い回すかした方がいいでしょう。


・　変数 mul
　 この変数は PadInputとのビット演算を行う為の変数なのでmul *= 2は
mul <<= 1とした方が適切ではないでしょうか。

　でも、そうすると今度は if(PadInput & mul)の部分が if(PadInput & (1 << i))とも
書けることになるので、mul変数そのものが不要になりますね。


・　変数 p。
　使い方は合っていますが、あまり存在の意味がないです。
　pは使わず普通に qでいいかと。


・　配列 q
　配列 qは今のような形（関数が呼ばれるたびに配列を作り直す）であれば
configpad.XXXの変数を入れる配列ではなく、&pad.key[configpad.XXX]を
入れる変数の方がいいのかも。


・　配列 s。
　配列 sの使用用途は CheckStateKey()の引数の為ですよね？
　CheckStateKeyの引数は unsigned charです。
　なので、unsigned charの配列にした方がいいでしょう。


　とりあえずといったあたりを改善してみてはどうでしょうか。

[koi]

回答ありがとうございます。

すいません。投稿した直後にpreタグつけ忘れていることに気付きました。

書き直しました。

//============key.cpp================
void GetHitPadStateAll(){
        int i,PadInput,mul=1;
        int *q[/url]={  &pad.key[configpad.left],
                    &pad.key[configpad.up],
                    &pad.key[configpad.right],
                    &pad.key[configpad.down],
                    &pad.key[configpad.shot],
                    &pad.key[configpad.bom],
                    &pad.key[configpad.slow],
                    &pad.key[configpad.start],
                    &pad.key[configpad.up]
                };

        unsigned char s[/url]={KEY_INPUT_LEFT,KEY_INPUT_UP,KEY_INPUT_RIGHT,KEY_INPUT_DOWN,KEY_INPUT_Z,
                 KEY_INPUT_X,KEY_INPUT_LSHIFT,KEY_INPUT_ESCAPE,KEY_INPUT_LCONTROL};
      

        PadInput = GetJoypadInputState( DX_INPUT_PAD1 );//パッドの入力状態を取得
        for(i=0;i<16;i++){
                if(PadInput & (1<<i))  pad.key++;
                else                   pad.key=0;
                
        }

        for(i=0;i<9;i++)
        input_pad_or_key(q,CheckStateKey(s));
}

なんだか初期化の部分が長くなってしまいましたが、構造体部分を書きなおせばもっと短くなるんでしょうか？

//=============struct.cpp=============
//コンフィグに関する構造体 
typedef struct{
        int left,up,right,down,shot,bom,slow,start,change;

}configpad_t;

//==============GV.h==================
extern configpad_t configpad;

>配列 qは今のような形（関数が呼ばれるたびに配列を作り直す）であれば

他にも書き方があるという意味ですよね。どのような書き方なのか知りたいです。
よろしくおねがいします。

[管理人]

十分綺麗だと思いますが、初期化をもっと短くかきたいのなら、
構造体の中身のアドレスは連続している事を用いればポインタでループして初期化できると思います。
例えば

typedef struct{
        int left,up,right,down,shot,bom,slow,start,change;
}configpad_t;

においてleft+1のアドレスがupになるわけですね。

[Justy]

　書き方がいろいろあって、さらに話が分岐してたりするので、
ちょっとややこしくなりつつありますね。



＞なんだか初期化の部分が長くなってしまいましたが

　あー、改行を入れると長くなりますね。
　ただ、input_pad_or_keyの行がわかりやすくなります。
　短きに重きをおくか、わかりやすさに重きをおくかってところでしょうか。



＞構造体部分を書きなおせばもっと短くなるんでしょうか

　そこに手を付けていいのであれば、たしかに短くスマートになりそうです。
　ただ、この関数の外に修正範囲が及ぶので、どうしたものか・・・と。

　さわりだけ説明しておきますと、

[color=#d0d0ff" face="monospace]    typedef enum configpad_index_t
    {
        configpad_index_left,
        configpad_index_up,
        configpad_index_right,
        configpad_index_down,
        configpad_index_shot,
        configpad_index_bom,
        configpad_index_slow,
        configpad_index_start,
        configpad_index_change,
        configpad_index_max
    } configpad_index;
    
typedef struct{
    int pad[configpad_index_max];
}configpad_t;[/color]

　こんな感じの configpad_tに変えると configpad.shotは configpad.pad[configpad_index_shot]と書くことになりますが、
配列 qが必要なくなり、

[color=#d0d0ff" face="monospace]    for(int i = 0; i < configpad_index_max; i++)
        input_pad_or_key(&pad.key[configpad.pad], CheckStateKey(s));[/color]

　と書けるようになります。
　反面、configpad_tのメンバが変わっているので、あちこちで修正が必要になりますが・・・。



＞においてleft+1のアドレスがupになるわけですね

　管理人さんのこの方法でもいいと思います。

　ただ、configpad_tの先頭のメンバが leftで、ラストが change、
その間にいくつメンバがある（ループする数）のかをある程度自動的に
判定して処理できるようにしておかないと、今後のメンテナンスに問題が残りそうです。

　そのあたりを考えてコーディングしなければいけない、かなぁと思います。
（決めうちで数を決めたりすると、後でメンバが増えた時(bom2とか）に
関数側の変更を忘れかねないですから）

[Justy]

＞他にも書き方があるという意味ですよね。どのような書き方なのか知りたいです

　これは別にコードが短くなるということではないのですが、
配列を毎回作らないという方法があります。

　今回のようなケース、特に sの方は毎回関数が呼ばれる度に全く同じ配列を作成しています。
　呼ばれる度に同じ配列を作るのはある意味無駄ではないでしょうか。

　そこで、配列の頭に static constをつけると静的な変数になるので関数が実行される度に配列を
作る、ということがなくなります（一度作ったらそれっきり、constがついているので変更もできません）。

　配列 sの方はそのまま static constをつけるだけでＯＫです。
　配列 qの方はそのままではだめで、configpad.XXXへのポインタの配列にする必要があります。
　というのも、configpad.XXXの変数の値は呼ばれる度に異なる可能性があるからです。
　そのポインタという形にしておけば作り直す必要はなくなります。

　で、ここで sと q両方が static const化できたのなら、
元々関数の中に配列がある必要がないので、さくっと関数の外に出してしまえば、関数の中はそれなりに短くなります。


　本当を言ってしまえば、こういう配列が複数あるなら sと qは構造体に
１つにまとめてしてしまい、その配列を作った方が短くなるというよりかは綺麗になります。
　なぜなら、ミスで sと qの要素数が異なる、ということもなくなりますし、
対応する内容の確認もしやすくなりますから。

　こんな感じでしょうか。

[color=#d0d0ff" face="monospace] static const struct consfig_pad_check_list
    {
        int *           pad;
        unsigned char   keyboard;
    }
    check_list[/url] =
    {
        { &configpad.left, KEY_INPUT_LEFT },
        { &configpad.right, KEY_INPUT_RIGHT },
        { &configpad.up, KEY_INPUT_UP },
        { &configpad.down, KEY_INPUT_DOWN },
        { &configpad.shot, KEY_INPUT_Z },
        { &configpad.bom, KEY_INPUT_X },
        { &configpad.slow, KEY_INPUT_LSHIFT },
        { &configpad.start, KEY_INPUT_ESCAPE },
        { &configpad.change, KEY_INPUT_LCONTROL },
    };

void GetHitPadStateAll(){
    int pad_state = GetJoypadInputState( DX_INPUT_PAD1 );
    for(int i=0;i<16;i++)
        pad.key = (pad_state & (1 << i))? pad.key+1: 0;
    for(int i = 0; i < sizeof(check_list)/sizeof(check_list[0]); i++)
        input_pad_or_key(&pad.key[*check_list.pad], CheckStateKey(check_list.keyboard));
}[/color]

　配列テーブルを外に出してしまうことで、GetHitPadStateAll()は随分すっきりします。
（input_pad_or_keyの行がごちゃっとしている感はありますが・・・）

[koi]

できました。

　　　　int i,PadInput,mul=1,*q[9];
        int* p= (int*)(&configpad);
        for(i=0;i<9;i++)
            q=&pad.key[p];

ですね
かなり短くなりました。ありがとうございました。