書こうと思ったきっかけとか

昔、いわゆる長年掛けて熟成されたソースに手を入れる機会があり、ほぼswitch文だけで1000行みたいなCの関数を既存コードを前に奮闘してた記憶を掘り起こしながら、私が当時どのようにリファクタリングを行っていたのか、備忘録というか、メモ書きしておこうと思い立ったので。

冗長なswitch文

とりあえず、以下のような敵種別と敵ステータス型が定義されてるとして

typedef enum {
    ENEMY_SLIME,
    ENEMY_BIRD,
    ENEMY_MAGE,
    ENEMY_ORC,
    ENEMY_DRAGON,

    // なんだか他にもいっぱい

}EnemyType;

typedef struct {
    char name[20];
    int hp, mp, atk, def, ex, gold;
}EnemyStatus;

種別ごとに固有のステータスを取得する関数があったとします。

EnemyStatus GetEnemyStatus(EnemyType type) {
    EnemyStatus status;

    strcpy(status.name, "unknown");
    status.hp = 1;
    status.mp = 0;
    status.atk = 0;
    status.def = 0;
    status.ex = 0;
    status.gold = 0;

    switch(type){
        case ENEMY_SLIME:
            strcpy(status.name, "slime");
            status.hp = 30;
            status.atk = 10;
            status.ex = 1;
            status.gold = 1;
            break;
        case ENEMY_BIRD:
            strcpy(status.name, "bird");
            status.hp = 80;
            status.atk = 20;
            status.def = 10;
            status.ex = 5;
            status.gold = 10;
            break;
        case ENEMY_MAGE:
            strcpy(status.name, "mage");
            status.hp = 50;
            status.mp = 800;
            status.atk = 10;
            status.def = 30;
            status.ex = 20;
            status.gold = 20;
            break;
        case ENEMY_ORC:
            strcpy(status.name, "orc");
            status.hp = 300;
            status.atk = 100;
            status.def = 20;
            status.ex = 30;
            status.gold = 50;
            break;
        case ENEMY_DRAGON:
            strcpy(status.name, "dragon");
            status.hp = 800;
            status.mp = 100;
            status.atk = 150;
            status.def = 80;
            status.ex = 100;
            status.gold = 300;
            break;

        // 以下延々と敵の種類数だけ続く
    }

    return status;
}

…うわぁ。もうね。なんかね。この時点で外部から読み込みなよ…と突っ込みたくもなるのですが、なにぶん熟成されたソースなのでひとまずはご了承ください。

文字位置を揃える

とりあえず、私がまずやるのがこれです。
何百何千行ものコードを整理する際、やはり一番こわいのは必要な処理まで消すことなので、まず処理に関わる文字には一切タッチせず、

スペースと改行だけを挿入・削除だけを行う

と決心します。そして
・case～breakの処理を1行にする（これいろんな人に怒られそう）
・言語のトークンレベルで各case内処理の左右位置を可能な限り揃える
という整理をします。こんな感じになります。

    switch(type){
        case ENEMY_SLIME  :  strcpy(status.name, "slime" );  status.hp =  30;                     status.atk =  10;                    status.ex =   1;  status.gold =   1;  break;
        case ENEMY_BIRD   :  strcpy(status.name, "bird"  );  status.hp =  80;                     status.atk =  20;  status.def = 10;  status.ex =   5;  status.gold =  10;  break;
        case ENEMY_MAGE   :  strcpy(status.name, "mage"  );  status.hp =  50;  status.mp  = 800;  status.atk =  10;  status.def = 30;  status.ex =  20;  status.gold =  20;  break;
        case ENEMY_ORC    :  strcpy(status.name, "orc"   );  status.hp = 300;                     status.atk = 100;  status.def = 20;  status.ex =  30;  status.gold =  50;  break;
        case ENEMY_DRAGON :  strcpy(status.name, "dragon");  status.hp = 800;  status.mp  = 100;  status.atk = 150;  status.def = 80;  status.ex = 100;  status.gold = 300;  break;

        // 以下延々と敵の種類数だけ続く
    }

…わぁ、なんか、すごく怒られそう。そして横に長い。
これはあれですね。
・1行に(セミコロンで区切られる)複数の文を書いてはならない。
・1行は半角80文字以内としなければならない。
みたいなコーディング規約があったら一発アウトですね。

デフォルト値を補完するなど

ENEMY_SLIMEのmpなどは、switchの外で初期化された0に設定されていますね。これをswitch内に補完してみましょう。


    switch(type){
        case ENEMY_SLIME  :  strcpy(status.name, "slime" );  status.hp =  30;  status.mp  =   0;  status.atk =  10;  status.def =  0;  status.ex =   1;  status.gold =   1;  break;
        case ENEMY_BIRD   :  strcpy(status.name, "bird"  );  status.hp =  80;  status.mp  =   0;  status.atk =  20;  status.def = 10;  status.ex =   5;  status.gold =  10;  break;
        case ENEMY_MAGE   :  strcpy(status.name, "mage"  );  status.hp =  50;  status.mp  = 800;  status.atk =  10;  status.def = 30;  status.ex =  20;  status.gold =  20;  break;
        case ENEMY_ORC    :  strcpy(status.name, "orc"   );  status.hp = 300;  status.mp  =   0;  status.atk = 100;  status.def = 20;  status.ex =  30;  status.gold =  50;  break;
        case ENEMY_DRAGON :  strcpy(status.name, "dragon");  status.hp = 800;  status.mp  = 100;  status.atk = 150;  status.def = 80;  status.ex = 100;  status.gold = 300;  break;

        // 以下延々と敵の種類数だけ続く
    }

こうなりました。

データを抽出して処理と分離

ここまで揃えると、もはやそのままデータ配列として使えそうなテキストデータになりつつあるので、そのまま利用してデータ配列に切り出しちゃいます。

	static const EnemyStatus EnemyStatusMaster[] = {
		{  "slime"  ,   30,    0,   10,   0,    1,    1  },
		{  "bird"   ,   80,    0,   20,  10,    5,   10  },
		{  "mage"   ,   50,  800,   10,  30,   20,   20  },
		{  "orc"    ,  300,    0,  100,  20,   30,   50  },
		{  "dragon" ,  800,  100,  150,  80,  100,  300  },

		// 以下延々と敵の種類数だけ続く

		{  "unknown",    1,    0,    0,   0,    0,    0  },
	};

こんな風に。私のPCではサクラエディタの矩形選択機能(Shift+F6)がこの加工作業に大活躍してくれます。

すると処理は、もはやいろいろ要らなくなってしまいます。


EnemyStatus GetEnemyStatus(EnemyType type) {
    static const EnemyStatus EnemyStatusMaster[] = {
        {  "slime"  ,   30,    0,   10,   0,    1,    1  },
        {  "bird"   ,   80,    0,   20,  10,    5,   10  },
        {  "mage"   ,   50,  800,   10,  30,   20,   20  },
        {  "orc"    ,  300,    0,  100,  20,   30,   50  },
        {  "dragon" ,  800,  100,  150,  80,  100,  300  },

        // 以下延々と敵の種類数だけ続く

        {  "unknown",    1,    0,    0,   0,    0,    0  },
    };
    return EnemyStatusMaster[type];
}

シンプル。
もはや構造体配列を参照させれば良い感じになってきました。

以下補足的な

** 基本は等価交換 **
私がこういった何百何千行ものコードを整理する場合
・ヒューマンエラーがこわいので、可能なだけ機械的に変換する
・数学で解を導くのと同様に、イコールで結べる等価なソースコードを経て段階的に直す
・何が固有なデータで、何が共通な処理かを見極める
といった点を留意してます。

** データをどこに置くか問題 **
切り出したデータを実際どう取り扱うかは、組込とかPCとか、環境に依存することになるかと思います。
・ファイル読込の処理時間が許されるなら、csvファイルに分割できるかもしれません
・ハードコーディングでいくなら、静的領域に置くようにするかもしれません
switch文でハードコーディングされている状態ではデータの外部化が難しいので、まずはプログラムからデータを抽出して切り離そう、という話でした。