Go to Qiita Advent Calendar 2024 Top
LoginSignup
5
0

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

C++Advent Calendar 2024

Day 17
@Hourier(掘江 弘己)

リテラルの型エイリアスは止めろ、マジで止めろ

Last updated at Posted at 2024-12-16

はしがき

この記事はC++ Advent Calendar 2024の17日目の記事です。
前日は@exli3141 さんでした (本記事執筆時点でタイトル未定)

改めましてこんにちは、掘江です！
変愚蛮怒の開発メンバーをやっています。
今日は26万行ある変愚蛮怒のソースコードの不快深い海を泳ぎきったお話をします。
面白かったらぜひDiscordにも遊びに来て下さいね。
それでは早速どうぞ！

型エイリアスとは

こういうのです。
変愚蛮怒にはこんな型エイリアスが数十存在します。

typedef int32_t ITEM_NUMBER; /*!< ゲーム中のアイテム数型を定義 */

void floor_item_increase(PlayerType *player_ptr, INVENTORY_IDX i_idx, ITEM_NUMBER num)

何が悪いかは敢えて述べるのもアホらしくなるほど一目瞭然だと思います。

  • numが「数」であることだけは辛うじて分かるが、「何の数なのか」は型を見ないと分からない
    • しかも時々間違っている
    • 数値幅と符号の有無さえ合っていればC++は警告を出さない
    • 即ち間違っていようがいまいがコンパイルエラーにならない
  • 頻繁に共用されている
    • 昭和の悪習1 に 「1つの変数に複数の意味を持たせる」 というのがあった
    • そしてまだ山ほど残っている
  • 慣れていないと、ITEM_NUMBER が自作クラスなのかリテラルの型エイリアスなのか分からない2
  • 慣れていても、数値幅 (8ビット or 16ビット or 32ビット3)は調べないと分からない
  • それでも使いたいならせめてtypedef からusing に書き換えろ

C言語の時代でも構造体は当然存在していたので、C++20なんてモダンな環境で今更こんなものを使う必要はありません……そう本来ならね。

何が困るかって、変愚蛮怒にある型エイリアスは50個以上あることです。
何事も慣れてくると感覚が麻痺しがちですが、今まで何度も「モンスター実体インデックスとモンスター定義ID」の取り違えでバグり散らかしてきました
本記事執筆時点でも「DUNGEON_IDX」と「普通のshort4」が混じっていてカオスです。
流石に「読めない」ことにキレ散らかしたので、「ダンジョンIDを指し示す変数」を全部enum class に変えました。
そのたった1回のコミットでLOCは数百行、前後の作業も含めると2000行近い地獄が展開されました。

結果

これ5が……

#pragma once

#include <vector>

extern std::vector<DEPTH> max_dlv;

こうじゃ！！6

#pragma once

#include <map>
#include <memory>
#include <optional>
#include <utility>
#include <vector>

enum class DungeonMessageFormat {
    DUMP,
    KNOWLEDGE,
    RECALL,
};

class DungeonRecord {
public:
    DungeonRecord() = default;
    bool has_entered() const;
    int get_max_level() const;
    int get_max_max_level() const;
    void set_max_level(int level);
    void reset();

private:
    std::optional<int> max_max_level;
    std::optional<int> max_level;
};

enum class DungeonId;
class DungeonDefinition;
class DungeonRecords {
public:
    DungeonRecords(DungeonRecords &&) = delete;
    DungeonRecords(const DungeonRecords &) = delete;
    DungeonRecords &operator=(const DungeonRecords &) = delete;
    DungeonRecords &operator=(DungeonRecords &&) = delete;
    ~DungeonRecords() = default;

    static DungeonRecords &get_instance();
    DungeonRecord &get_record(DungeonId dungeon_id);
    const DungeonRecord &get_record(DungeonId dungeon_id) const;
    std::map<DungeonId, std::shared_ptr<DungeonRecord>>::iterator begin();
    std::map<DungeonId, std::shared_ptr<DungeonRecord>>::const_iterator begin() const;
    std::map<DungeonId, std::shared_ptr<DungeonRecord>>::iterator end();
    std::map<DungeonId, std::shared_ptr<DungeonRecord>>::const_iterator end() const;
    std::map<DungeonId, std::shared_ptr<DungeonRecord>>::reverse_iterator rbegin();
    std::map<DungeonId, std::shared_ptr<DungeonRecord>>::const_reverse_iterator rbegin() const;
    std::map<DungeonId, std::shared_ptr<DungeonRecord>>::reverse_iterator rend();
    std::map<DungeonId, std::shared_ptr<DungeonRecord>>::const_reverse_iterator rend() const;
    size_t size() const;
    bool empty() const;
    void reset_all();

    int find_max_level() const;
    int decide_gradiator_level() const;
    std::vector<std::string> build_known_dungeons(DungeonMessageFormat dmf) const;
    std::vector<DungeonId> collect_entered_dungeons() const;
    std::pair<std::shared_ptr<DungeonRecord>, std::shared_ptr<DungeonDefinition>> get_dungeon_pair(DungeonId dungeon_id) const;

private:
    DungeonRecords();
    static DungeonRecords instance;
    std::map<DungeonId, std::shared_ptr<DungeonRecord>> records;
};

はい、全くの別物です。
ちゃんと実益もありました。

View とModel の分離

変愚蛮怒のソースコードは地獄のトランザクションスクリプトです。
こういうコードが腐って悪臭を放っています。

// シグネチャだけで読む気が失せる
void hoge(Fuga *fuga, int *y1, int *x1, int *y2, int *x2) {
  int i, x = 0; // 言語仕様的にiは未初期化. もうこの時点で逃げ出したい.
  bool ret; // 何を示す結果なのか理解不能

  // 初期値の0はともかく最大値の10ってなんやねん
  for (i = 0; i < 10; i++) {
    something_process1(fuga);
    something_view1(fuga); // 順番は変えられない
    ret = something_process2(fuga); // fugaまみれ
    if (!ret) {
      ...
    }
    
    // AもBもCもDもEもFもGもクソ長すぎて実際は画面端を遥か先までロケットでつきぬけている
    if ((A && B && C) || (D && E && F && G)) {
      something_view2(fuga); // こういうのをViewに分離できない
    }
  }

  *y2 = ...
  // え、ループ外でiを宣言したのに結局使わなかったの？ (C72の名残)
}

より具体的にはこんな感じです。

Before

void do_cmd_knowledge_dungeon(void)
{
	FILE *fff;
	
	char file_name[1024];
	int i;
	
	
	/* Open a new file */
	fff = my_fopen_temp(file_name, 1024);
	if (!fff) {
	    msg_format(_("一時ファイル %s を作成できませんでした。", "Failed to create temporary file %s."), file_name);
	    msg_print(NULL);
	    return;
	}
	
	if (fff)
	{
		for (i = 1; i < max_d_idx; i++)
		{
			bool seiha = FALSE;

			if (!d_info[i].maxdepth) continue;
			if (!max_dlv[i]) continue;
			if (d_info[i].final_guardian)
			{
				if (!r_info[d_info[i].final_guardian].max_num) seiha = TRUE;
			}
			else if (max_dlv[i] == d_info[i].maxdepth) seiha = TRUE;
			
			fprintf(fff, _("%c%-12s :  %3d 階\n", "%c%-16s :  level %3d\n"), seiha ? '!' : ' ', d_name + d_info[i].name, max_dlv[i]);
		}
	}
	
	/* Close the file */
	my_fclose(fff);
	
	/* Display the file contents */
	show_file(TRUE, file_name, _("今までに入ったダンジョン", "Dungeon"), 0, 0);
	
	/* Remove the file */
	fd_kill(file_name);
}

void dump_aux_recall(FILE *fff)
{
	int y;
	fprintf(fff, _("\n  [帰還場所]\n\n", "\n  [Recall Depth]\n\n"));

	for (y = 1; y < max_d_idx; y++)
	{
		bool seiha = FALSE;

		if (!d_info[y].maxdepth) continue;
		if (!max_dlv[y]) continue;
		if (d_info[y].final_guardian)
		{
			if (!r_info[d_info[y].final_guardian].max_num) seiha = TRUE;
		}
		else if (max_dlv[y] == d_info[y].maxdepth) seiha = TRUE;

#ifdef JP
		fprintf(fff, "   %c%-12s: %3d 階\n", seiha ? '!' : ' ', d_name+d_info[y].name, max_dlv[y]);
#else
		fprintf(fff, "   %c%-16s: level %3d\n", seiha ? '!' : ' ', d_name+d_info[y].name, max_dlv[y]);
#endif
	}
}

int choose_dungeon(cptr note, int y, int x)
{
	int select_dungeon;
	int i, num = 0;
	s16b *dun;

	/* Hack -- No need to choose dungeon in some case */
	if (lite_town || vanilla_town || ironman_downward)
	{
		if (max_dlv[DUNGEON_ANGBAND]) return DUNGEON_ANGBAND;
		else
		{
			msg_format(_("まだ%sに入ったことはない。", "You haven't entered %s yet."), d_name + d_info[DUNGEON_ANGBAND].name);
			msg_print(NULL);
			return 0;
		}
	}

	/* Allocate the "dun" array */
	C_MAKE(dun, max_d_idx, s16b);

	screen_save();
	for(i = 1; i < max_d_idx; i++)
	{
		char buf[80];
		bool seiha = FALSE;

		if (!d_info[i].maxdepth) continue;
		if (!max_dlv[i]) continue;
		if (d_info[i].final_guardian)
		{
			if (!r_info[d_info[i].final_guardian].max_num) seiha = TRUE;
		}
		else if (max_dlv[i] == d_info[i].maxdepth) seiha = TRUE;

		sprintf(buf,_("      %c) %c%-12s : 最大 %d 階", "      %c) %c%-16s : Max level %d"), 
					'a'+num, seiha ? '!' : ' ', d_name + d_info[i].name, max_dlv[i]);
		prt(buf, y + num, x);
		dun[num++] = i;
	}

	if (!num)
	{
		prt(_("      選べるダンジョンがない。", "      No dungeon is available."), y, x);
	}

	prt(format(_("どのダンジョン%sしますか:", "Which dungeon do you %s?: "), note), 0, 0);
	while(1)
	{
		i = inkey();
		if ((i == ESCAPE) || !num)
		{
			/* Free the "dun" array */
			C_KILL(dun, max_d_idx, s16b);

			screen_load();
			return 0;
		}
		if (i >= 'a' && i <('a'+num))
		{
			select_dungeon = dun[i-'a'];
			break;
		}
		else bell();
	}
	screen_load();

	/* Free the "dun" array */
	C_KILL(dun, max_d_idx, s16b);

	return select_dungeon;
}

After

void do_cmd_knowledge_dungeon(PlayerType *player_ptr)
{
    FILE *fff = nullptr;
    GAME_TEXT file_name[FILE_NAME_SIZE];
    if (!open_temporary_file(&fff, file_name)) {
        return;
    }

    const auto known_dungeons = DungeonRecords::get_instance().build_known_dungeons(DungeonMessageFormat::KNOWLEDGE);
    for (const auto &known_dungeon : known_dungeons) {
        fprintf(fff, "%s", known_dungeon.data());
    }

    angband_fclose(fff);
    FileDisplayer(player_ptr->name).display(true, file_name, 0, 0, _("今までに入ったダンジョン", "Dungeon"));
    fd_kill(file_name);
}

void dump_aux_recall(FILE *fff)
{
    fprintf(fff, _("\n  [帰還場所]\n\n", "\n  [Recall Depth]\n\n"));
    const auto &dungeon_records = DungeonRecords::get_instance();
    for (const auto &known_dungeon : dungeon_records.build_known_dungeons(DungeonMessageFormat::DUMP)) {
        fprintf(fff, "%s", known_dungeon.data());
    }
}

std::optional<DungeonId> choose_dungeon(std::string_view note, int row, int col)
{
    const auto &dungeon_records = DungeonRecords::get_instance();
    if (lite_town || vanilla_town || ironman_downward) {
        if (dungeon_records.get_record(DungeonId::ANGBAND).has_entered()) {
            return DungeonId::ANGBAND;
        }

        const auto &dungeons = DungeonList::get_instance();
        constexpr auto fmt = _("まだ%sに入ったことはない。", "You haven't entered %s yet.");
        msg_format(fmt, dungeons.get_dungeon(DungeonId::ANGBAND).name.data());
        msg_print(nullptr);
        return std::nullopt;
    }

    screen_save();
    const auto dungeon_messages = dungeon_records.build_known_dungeons(DungeonMessageFormat::RECALL);
    const int num_messages = dungeon_messages.size();
    for (auto i = 0; i < num_messages; i++) {
        prt(dungeon_messages.at(i), row + i, col);
    }

    if (dungeon_messages.empty()) {
        prt(_("      選べるダンジョンがない。", "      No dungeon is available."), row, col);
    }

    prt(format(_("どのダンジョン%sしますか:", "Which dungeon do you %s?: "), note.data()), 0, 0);
    const auto ids = dungeon_records.collect_entered_dungeons();
    DungeonId select_dungeon;
    while (true) {
        const auto key = inkey();
        if ((key == ESCAPE) || dungeon_messages.empty()) {
            screen_load();
            return std::nullopt;
        }

        if (key >= 'a' && key < static_cast<char>('a' + ids.size())) {
            select_dungeon = ids.at(key - 'a');
            screen_load();
            return select_dungeon;
        }

        bell();
    }
}

std::vector<std::string> DungeonRecords::build_known_dungeons(DungeonMessageFormat dmf) const
{
    const auto fmt = get_dungeon_format(dmf);
    const auto &dungeons = DungeonList::get_instance();
    std::vector<std::string> recall_dungeons;
    auto num_entered = 0;
    for (const auto &[dungeon_id, record] : this->records) {
        if (!record->has_entered()) {
            continue;
        }

        const auto max_level = record->get_max_level();
        const auto &dungeon = dungeons.get_dungeon(dungeon_id);
        const auto is_dungeon_conquered = dungeon.is_conquered();
        const auto is_conquered = is_dungeon_conquered || (max_level == dungeon.maxdepth);
        std::string message;
        switch (dmf) {
        case DungeonMessageFormat::DUMP:
        case DungeonMessageFormat::KNOWLEDGE:
            message = format(fmt.data(), is_conquered ? '!' : ' ', dungeon.name.data(), max_level);
            break;
        case DungeonMessageFormat::RECALL:
            message = format(fmt.data(), static_cast<char>('a' + num_entered), is_conquered ? '!' : ' ', dungeon.name.data(), max_level);
            num_entered++;
            break;
        default:
            THROW_EXCEPTION(std::logic_error, format("Invalid dungeon message format is specified! %d", enum2i(dmf)));
        }

        recall_dungeons.push_back(message);
    }

    return recall_dungeons;
}

なんて人類に優しい[要出典]コードなんだ！
View とModel がごっちゃになっている箇所から、Model 関連の処理をいい感じに共通化して切り出せているのが分かる[要出典]かと思います。
変数も使い回しせずなるべくconst にしたり、ポインタではなく参照にしたりと安全性も向上しています。
その上でView/Controller/Service がごっちゃに混じっているのはもう手に負えません。その内どこかの誰かが後で何とかします。
ともあれ、「画面表示に使う文字列を組み立てるメソッド」と「実際に画面へ表示する処理7」を分離できました。
ついでにほぼ同一の処理が3箇所にコピペされていることが判明8 したので全部まとめました。
26万行あるソースコードにおいて、何がどれくらいコピペされているか把握してまとめられたのは大きな収穫です。

集合論的表現力の向上

詳細は私の記事「ドメイン仕様に基づいてコンテナクラスを集合論的自作クラスでラップすると幸せになれた話」 をご覧頂きたいですが、「集合の中で最大の要素を取る9」とか「集合を別な集合に射影する」とか、今回もシングルトン化によって大活躍です。
特に数値から文字列への射影なんてこれ↓ですからね？ (上記より抜粋)
クラスの外からゴチャゴチャ弄るからコードがどんどんクソになるんです。
カプセル化は超大事！！ クラスの内側で何とかしましょう。

format(fmt.data(), static_cast<char>('a' + num_entered), is_conquered ? '!' : ' ', dungeon.name.data(), max_level);

おわりに

変愚蛮怒はC言語で書かれてから約35年後、C++へ生まれ変わりました
しかしそこから今までの3年半でできた成果は限られています。
この調子でまだまだ大量のリファクタリングが控えていますが、生まれ変わっていく様子をどうぞお楽しみに！
拙作記事「オープンソースゲームにコントリビュートしてみろ、飛ぶぞ」も併せて読んで頂ければ幸いです、開発メンバーは常時募集中です！

それでは今日はこの辺で、また次の記事でお会いしましょう！
明日の発表は@kc-hedgehog さんです (詳細未定)

  1. 90年代前半までは「メモリの1バイトは血の1滴」と呼ばれるほど貴重だったので、「変数は共用しまくればしまくるほど節約できて正義」「関数は長ければ長いほど関数スタックを消費しなくて済むので正義」「何ならCPU命令とカラーパレットで同じ値を共用するのが正義 (ファミコン時代のテクニック。この亜種がゴーストバスターズの「りり」バグ)」など、現代人からすれば「一刻も早く担当者をクビにしなければならないレベルのクソ設計」こそが当時では「絶対唯一の正義」でした ↩

  2. 一応型エイリアス＆enum classの要素は大文字スネークケース、構造体は小文字スネークケース、クラス＆enum class はパスカルケースと分けているので、命名規則を覚えれば間違えることは少ないです。但しこれらすらも一部慣例と歴史的経緯の塊です ↩

  3. 変愚蛮怒に64ビット幅の整数はありません。作りたいのは山々なのですが自作の64ビット幅整数演算関数が邪悪すぎる上に他の高優先度タスクが山積みです ↩

  4. そもそもインデックスではなくIDであるとか、コード上存在しないダンジョンなのに実行時には巡り巡ってバグなく出入りできていたとか、intをshortにケチる理由なんて20世紀と共に消滅したとか、ユーザの目に見えないところで開発者を苦しめまくってます ↩

  5. 「dlv」とは「dungeon level」の略です。ざっくり言って「全100Fのダンジョンの内、何階まで行けたのか」を示します。DEPTHとはint32_t で、mapになっているのはダンジョンが複数あるからです。なお昔は生配列「DEPTH[]」だったのでもっと酷かったです ↩

  6. 記事執筆時点でまだマージされていません。変更の可能性があります ↩

  7. 歴史的事情で、一時ファイルに書き出してからそれを改めて読み込んで画面に表示するとかいうイミフなことをしています。人類が理由を理解できる日は永遠に[要出典]来ないと思います ↩

  8. 変愚蛮怒のソースコードはコピペ至上主義だった時代が長かったので、全貌は誰も掴めないです ↩

  9. ただのvector<int> ならまだ良いんですが、自作クラスだと「フィールド変数hitpointsが最大」「フィールド変数priceが最大」とかが必要になってキリがないので、こういうのをカプセル化すると恩恵が大きいです ↩

5
0
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
Sign upLogin
5
0

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?