関数内では、グローバル変数は直接使わず、引数として渡して使ったほうが良いですか？

Question

シューティングゲームをc言語&openglで作っています。困ったことが起きたので質問させていただきます。

以下のようなコードがあったとします。

MISSILE g_missile[10]
int g_missile_num;

void setMissile(MISSILE *missile, double x, double y, double angle, double speed) {
	missile->x = x;
	missile->y = y;
	missile->angle = angle;
	missile->speed = speed;

	g_missile_num++;  //ミサイルの数をカウントする
}

int main(){
  for(i=0; i<10; i++){
    setMissile(&missile[g_missile_num], 0, 0, 0, 0);
  }
  return 1;
}

setMissile関数では、g_countというグローバル変数にmissileが作成された数を記録しています。

しかし、「グローバル変数は関数内で使わない。使うなら引数として渡して使う」という設計思想が世の中にはありますよね（あるきがする）。

では、上のコードはどのようにするのが良いのでしょうか？

Comments

[LouiS0616]

ご提示のコードの場合、ミサイル数をカウントする必然性が無いように思えます。単に簡単な例を示しただけで、本当はもう少し煩雑なのでしょうか？

[N---------]

ミサイルを画面に描画するdrawMissile(MISSILE *missile)関数というのを使うときに、g_missile_num変数をつかいます。
すべてのミサイルを画面上に描画するために、drawMissile関数をミサイルの数だけループさせたいので
（こんな感じのコード→for(i=0; i<g_missile_num; i++)
drawMissile(missile[i]); ）、その時に必要になります

Answer 1

こんにちは。

しかし、「グローバル変数は関数内で使わない。使うなら引数として渡して使う」という設計思想が世の中にはありますよね（あるきがする）。

微妙に間違ってます。「なるべくグローバル変数は使わない。」です。（C/C++では関数外でグローバル変数を使うことは初期化を除きできません。）

で、なぜ「なるべくグローバル変数は使わない。」方が良いのかですが、最大の要因はアクセスする必要ない関数がアクセスできないようにしておけば、プログラマのミスにより「間違って」アクセスすることがないのでバグが格段に減るという経験則から来ています。グローバル変数は事実上全ての関数がアクセスできるので、バグの温床になりがちなのです。

ですので、ご提示のg_missileとg_missile_numを、アクセスする必要がない関数から「間違って」アクセスできないようにすれば、別にグローバル変数のままでも大差ありません。
C言語で可能な方法としては、下記のような方法が考えられます。

①アクセスする必要がある関数の数が少なければ、引数で渡すのが理想的です。

②そこそこ多数の関数がアクセスするなら、それらの関数を全て同じソース・ファイルに収めて、g_missileとg_missile_numをstatic変数とする方法もあります。

②更に多数の関数がアクセスするなら、g_missileとg_missile_numをextern宣言したヘッダを、これらをアクセスする必要がある関数群を定義した .cファイルでのみインクルードすることも視野に入ります。

なお、ご提示のような方法でグローバル変数とした場合の問題点としてもう１つあります。
下記は１つのセットと思います。

MISSILE g_missile[10]
int g_missile_num;

もし、このようなセットを複数設けたい場合（例えば、敵も同じようにミサイルを持つ場合や、協力プレイ可能なゲームで自機が同時に２機以上存在するなどなど）には、これでは非常に面倒です。
この２つを構造体にまとめ、これらをアクセスする関数addMissile(当該構造体*)みたいなものを設けます。addMissile()へg_missile_numのインクリメントを移動し、かつ、setMissile()を呼び出すと複数のセットを設けることが簡単になり、かつ、必要性の低い依存（setMissileがg_missile_numに依存）を切り離せます。

ちなみに、この発想を推し進めていくとyumetodoさんのようなコードもゴールの１つとなります。
実際の場面で、そこまでやるべきかどうかはケースバイケースですが。
汎用性は高いですがその分複雑です。汎用性と複雑さのトレードオフをどこにおくのかも結構重要です。経験的には仕様を満たす最小限から複雑さがどんと増える手前くらいで汎用化を留めておくとトータルの工数が最低になるように感じます。

Comments

[N---------]

回答ありがとうございます！助かります！

Answer 2

ミサイルの本数が固定なら、最初から定数マクロを使っても良いのですよね。

#define NUM_OF_MISSILES 10

int main(void){
    MISSILE missile[NUM_OF_MISSILES];
    
    for(size_t i = 0; i < NUM_OF_MISSILES; i++) {
        setMissile(&missile[i], 0, 0, 0, 0);
    }
    
    for(size_t i = 0; i < NUM_OF_MISSILES; i++) {
        drawMissile(&missile[i]);
    }
    
    return 0;
}

あるいは要素数を前以て保持しておくとか。

int main(void){
    MISSILE missile[10];
    const size_t num_of_missiles = sizeof(missile) / sizeof(missile[0]);
    
    for(size_t i = 0; i < num_of_missiles; i++) {
        setMissile(&missile[i], 0, 0, 0, 0);
    }
    
    for(size_t i = 0; i < num_of_missiles; i++) {
        drawMissile(&missile[i]);
    }
    
    return 0;
}

ただしミサイルの本数を動的に変化させたい場合は、
yumetodoさんの回答のような方法を選ぶのが見通しが良いと思います。

Answer 3

#include <stdlib.h>
#include <stdbool.h>
typedef struct MISSILE {
    double x;
    double y;
    double angle;
    double speed;
}
typedef struct tag_missile_vec {
    MISSILE* data;
    size_t size;
    size_t capacity;
} missile_vec;

missile_vec* missile_vec_new(size_t size, size_t capacity, double x, double y, double angle, double speed) {
    if(capacity < size) return NULL;
    missile_vec* missile;
    if(NULL = (missile->data = malloc(capacity * sizeof(MISSILE))) return NULL;
    for(size_t i = 0; i < size; ++i){
        MISSILE* m = &missile->data[i];
        m->x = x;
        m->y = y;
        m->angle = angle;
        m->speed = speed;
    }
    missile->size = size;
    missile->capacity = capacity;
    return missile;
}
bool missile_vec_push(missile_vec* missile, const MISSILE* data)
{
    if(NULL == missile || NULL == data) return false;
    if(capacity < size + 1){
        const size_t new_capacity = ((SIZE_MAX / 2) < missile->capacity) ?  SIZE_MAX : missile->capacity * 2;
        if(NULL == (missile->data = realloc(missile->data, capacity * sizeof(MISSILE)))) return false;
        missile->capacity = new_capacity;
    }
    missile->data[missile->size] = *data;
    ++missile->size;
    return true;
}
void missile_vec_delete(missile_vec* missile)
{
    if(NULL != missile) free(missile->data);
    free(missile);
}
void missile_vec_draw(missile_vec* missile)
{
    if(NULL == missile) return;
    //do something
}
int main(){
    missile_vec* missile = missile_vec_new(10, 20, 0, 0, 0, 0);
    if(NULL == missile) return 1;
    missile_vec_draw(missile);
    missile_vec_delete(missile);
    return 0;
}

Comments

[yumetodo]

まあここまでやるならC++のstd::vectorを使うべきである

[N---------]

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

Malloc関数を使ったことがないので、yumetodoさんの回答を気に使い方を調べました。

missile_vec_new関数の引数にsize_t sizeとsize_t capacityがありますが、これは何のための引数なのですか？
あとに、malloc(capacity * sizeof(MISSILE))というプログラムがでてきますが、なぜsizeof(MISSILE)にcapacityをかけるのですか？

[N---------]

上の質問ですが、考えたら分かりました。失礼しました。

[N---------]

missile_vec_draw関数やmissile_vec_delete関数でif(NULL == missile)とありますが、これは何のためにあるのですか？missile_vec_new関数で返り値がNULLだった時を考慮しているからですか？

[N---------]

もう一つ質問です。

missile_vec構造体で、dataをポインタとして宣言するのは何故ですか？

[yumetodo]

data可変長を見越して設計したからですね。あとものすごく要素数が大きくてstackに乗せたくないような場合を考えて。普通はstackを使ってあげたほうが高速です。Chironianさんも言っているように、どこまで想定するか、抽象化するか次第です。

NULLチェックは、だいたい「これくらい自明でしょ」ってところで省いたせいで後でバグを探すのが面倒になるやつなのでかならずもれなくチェックします。もっというとNULLチェックしたくないのでC++のlvalue reference使いたいです。

[N---------]

返信ありがとうございます！理解しました。