【AI文芸シリーズ玲奈様のコードレビュー】「フラグの使い方が無駄に複雑 → 余計な変数が多すぎる」

2025年3月2日 13:33

元ネタは情報処理安全確保支援士試験令和2年午後Ⅱ問題１のJavaコードです。
前段、後段もありますが、これだけで十分長いので、この部分だけで載せます。

コードの中で childChecked というフラグが使われているけど、このフラグ、本当に必要？ という疑問がある。
今のコードの流れを整理しながら、なぜ不要なのかを詳しく解説する。

問題のコード

public int checkChild() throws NamingException {
    // サイトの識別文字列のチェック
    childChecked = childSite.equals("siteQ") || childSite.equals("siteR");
    if (!childChecked) { 
        return ERROR_SITE_UNAVAILABLE;
    }

    switch (childSite) {
        case "siteQ":
            try {
                // 認証情報チェック
                if (siteQAuth(childID, childPW) == NO_ERROR) {
                    childChecked = true; //認証成功
                } else {
                    childChecked = false; //認証失敗
                }
            } catch (NamingException e) {
                throw e;
            }

            if (!childChecked) {
                return ERROR_ID_OR_PW;
            } else {
                return NO_ERROR;
            }
        case "siteR":
            (省略)
        default:
            (省略)
    }
}

このフラグの問題点

問題点①：初期化時にフラグを設定しているけど、結局上書きしてる

childChecked = childSite.equals("siteQ") || childSite.equals("siteR");

ここで childChecked に true を代入してるけど、この値は サイトの識別文字列が正しいかどうかをチェックするだけの値 。
その後、認証の成否でまた childChecked を上書きしてるので、この初期化の意味が薄い。

問題点②：認証結果を childChecked に詰め込む意味がない

if (siteQAuth(childID, childPW) == NO_ERROR) {
    childChecked = true; //認証成功
} else {
    childChecked = false; //認証失敗
}

「認証に成功したかどうか」なら siteQAuth() の戻り値をそのまま使えばいい
わざわざ childChecked に値を詰め直す必要がない
→ ここの if-else 分岐は 無駄な代入 を生んでいるだけ。

書き換え例（簡潔化）

int authResult = siteQAuth(childID, childPW);
if (authResult != NO_ERROR) {
    return ERROR_ID_OR_PW;
}
return NO_ERROR;

認証成功かどうかは siteQAuth() の戻り値で直接判断できる
childChecked を使う必要がない

問題点③：フラグを別の用途で使い回しすぎ

if (!childChecked) {
    return ERROR_ID_OR_PW;
} else {
    return NO_ERROR;
}

ここでも childChecked に頼りすぎ。
「認証に成功したか？」は siteQAuth() の戻り値で判断するのが正しい方法。
そもそも、childChecked という変数名も意味があいまいすぎる。
- 「チェック済み」なのか、「認証成功」なのか、どっちの意味？
- 名前の意味が曖昧だと、コードを読む側が混乱する。

フラグをなくした修正後のコード

public int checkChild() throws NamingException {
    // サイトの識別文字列のチェック
    if (!isValidSite(childSite)) {
        return ERROR_SITE_UNAVAILABLE;
    }

    switch (childSite) {
        case "siteQ":
            try {
                return (siteQAuth(childID, childPW) == NO_ERROR) ? NO_ERROR : ERROR_ID_OR_PW;
            } catch (NamingException e) {
                throw e;
            }
        case "siteR":
            (省略)
        default:
            return ERROR_SITE_UNAVAILABLE;
    }
}

private boolean isValidSite(String site) {
    return "siteQ".equals(site) || "siteR".equals(site);
}

改善点

✅ childChecked を完全に削除
✅ フラグを使わず、直接 siteQAuth() の戻り値で判断する
✅ サイトの識別文字列のチェックは isValidSite() に切り出し、コードの意図を明確化

「フラグのせいで逆にバグが生まれる」ケース

フラグを多用すると、バグを誘発しやすくなる。例えば、こんなバグが潜む可能性がある。

「フラグを使ったせいで誤判定が起きるケース」

childChecked = false; // 初期化
if (childSite.equals("siteQ") || childSite.equals("siteR")) {
    childChecked = true; // ここで true
}
if (!childChecked) {
    return ERROR_SITE_UNAVAILABLE;
}

// 認証失敗した場合
childChecked = false; // ここで false に戻る
if (!childChecked) {
    return ERROR_ID_OR_PW;
}

childChecked の用途が二重になっているせいで、間違った判定が発生する可能性がある。
認証の成否と、サイトの識別の妥当性を同じ変数で管理してしまうと、意図しないバグが起きる。

フラグを無駄に増やすことのデメリット

何のための変数なのかが分かりにくくなる
- childChecked は 「認証成功か？」と「サイトの識別文字列が正しいか？」を兼ねている
- 一つのフラグで複数の状態を表すのは危険
誤ったフラグの上書きが発生しやすい
- childChecked = true だったのに、途中で false に上書きされるとバグにつながる。
不必要な if-else 文が増えてコードが読みにくくなる
- childChecked を条件にする if-else 文が増えて、無駄な分岐が増える。

結論

☠️ フラグを乱用すると、バグの温床になる

「状態管理」は、可能な限りメソッドの戻り値で表現するのが正しいアプローチ
「認証成功したか？」は boolean ではなく、戻り値（エラーコード）で管理するのがスッキリする
フラグを一つに詰め込むと、バグを生むリスクが増大する

玲奈様の最終コメント

「フラグをこうやって使い回すの、クソコードの典型よ。
そもそも、状態管理を boolean フラグでやろうとするのが間違い。
コードの意図を明確にするために、フラグなしで管理する設計を考えなさい。」