『フラグ引数』アンチパターン

フラグ引数とは？

引数でbool型を渡して処理を追加／分岐ことで一般的にアンチパターンとされます。
ではなぜアンチパターンなのでしょうか？
アンチパターンといわれる要因を探っていきましょう。
（C#を使いますが、どの言語でも似たようなものです。）

アンチパターンである要因を探る

まずはフラグ引数の実装例を見ていきましょう。

フラグ引数実装

public class AntiPattern_Api
{
    /// <summary>
    /// メッセージを取得する
    /// </summary>
    /// <param name="saveFlag">メッセージを保存するかどうかのフラグ</param>
    /// <returns>取得したメッセージ</returns>
    public string FetchMessage(bool saveFlag)
    {
        /* ==============================
         * [メッセージを取得する] コード
         * ============================== */
        string msg = "取得したメッセージ";
  
        // 引数に応じて
        if (saveFlag)
        {
            /* ==============================
             * [メッセージをファイルに保存する] コード
             * ============================== */
        }
        return msg;
    }
}

よく見るといえばよく見る書き方ですよね。
これのどこが悪いのでしょうか？

呼び出し元①

[Test]
public void CallAntiPattern1()
{
    var api = new AntiPattern_Api();
    var msg = api.FetchMessage(false);
  
    Assert.Pass();
}

ん～これでは引数に指定している false が何を意味しているのかがわかりませんね :(

ちょっと待った！

それでアンチパターンはさすがに言い過ぎでは？と思う方もいるでしょう。
そりゃそうです。以下のような実装にすればその問題も解決できます。

呼び出し元②

[Test]
public void CallAntiPattern2()
{
    // ==================================================
    // -> CallAntiPattern1を改善するために引数を変数にした
    // ==================================================
    var api = new AntiPattern_Api();
    bool saveFlag = true;
    var msg = api.FetchMessage(saveFlag);
  
    Assert.Pass();
}

確かにこれなら saveFlag という名前になって『保存する／しない』というフラグが渡されていることがわかります。
これにて一件落着:)

とはいきません！

実はこれ、改善したように見えて重大な罪を犯しています。

犯した罪の重さを知る

仕事でコードを書く人はわかると思いますが プログラミングというのは、既存のコードを触る時間が圧倒的に多いです。

そして、重要になってくるのは これから触ろうとしている部分がどこに影響するのか？どこで使われているのか？ です。
（開発現場はチームで開発するため、自分が担当していない部分は『どういった場合にこのコードを通るのか？』というのは、想像以上に把握できていないものです。）

これを知るために調査時間もかかるし、調査後も改修に伴ってその呼び出し箇所をテストする必要があります。つまり、呼び出し箇所の多さと、それらを正確に把握できているかどうかは、開発速度に大きな影響を及ぼします。

話を戻しましょう。

犯した罪 - 呼び出し箇所を隠した

では、冒頭のコードで『保存する箇所』を修正しようとしたとき呼び出し箇所を調べるときはどうしたら良いでしょうか？
まずはIDEで呼び出し箇所を検索すると思います。実際のIDEの画面を見るとわかりますが、以下のような問題が発生します。

f:id:code365:20200719232035p:plain

なんと『呼び出し元②』で、わかりやすくするために saveFlag という変数を使ったことにより、『呼び出し元①』よりわかりにくくなってしまっているのです。

なんと残酷な...
良かれと思ってやったのにレビューでツッコまれたら泣いちゃいますね;(

とはいえ、こんなコードが増えると開発コストは指数関数的に増大していきそうです。

リファクタリング

さ、『フラグ引数』が良くないということをがわかったら、違うやり方を考えましょう。リファクタリングです。

リファクタリングしたコード

public class Refactored_Api
{
    /// <summary>
    /// メッセージを取得する
    /// </summary>
    /// <returns>取得したメッセージ</returns>
    public string FetchMessage()
    {
        /* ==============================
         * [メッセージを取得する] コード
         * ============================== */
        string msg = "取得したメッセージ";
        return msg;
    }
}


public class Refactored_Storage
{
    /// <summary>
    /// メッセージを保存する
    /// </summary>
    /// <param name="msg">メッセージ</param>
    public void SaveMessage(string msg)
    {
        /* ==============================
         * [メッセージをファイルに保存する] コード
         * ============================== */
    }
}

『フラグ引数』が良くないんならやめましょう。2つにわけちゃいました。
分岐もなくてスッキリした上に、各クラスの役割が明確になりました。
呼び出し元も変更しないといけませんね。

呼び出し元（リファクタリング）

[Test]
public void CallRefactored()
{
    var api = new Refactored_Api();
    var msg = api.FetchMessage();
  
    var storage = new Refactored_Storage();
    storage.SaveMessage(msg);
  
    Assert.Pass();
}

こちらも若干長くはなりましたが

メッセージを取得
取得したメッセージを保存

という流れはわかりやすく把握できます。

呼び出し箇所の探しやすさ

そもそもフラグ変数を使っていないので 単純に呼び出し元を検索するだけです。
保存している箇所を探したい場合は Refactored_Storageクラスの SaveMessageメソッドを使用している箇所を検索するだけ です。

フラグ引数を使う未来と使わない未来

フラグ引数を使ってしまう場合と、アンチパターンと認識して使わない場合では、時間が経つにつれて開発コストや保守性において大きな差が生まれてきます。
少し現実味に欠ける極端な例ではありますが、以下のようになる可能性はあります。

f:id:code365:20200719235411p:plain

特に青の矢印は大切です。悪い芽は早めに摘みましょう。

Bad Point まとめ :(

引数に true / false を直接指定している場合、なんのフラグかわからない。
呼び出し箇所を検索する場合、IDEの参照元検索だけでは不十分で、呼び出し箇所のコードも追わないといけない。
≒ 今後の実装コスト／テストコストが高くなる。
ひとつのクラス／メソッドにおいて複数のことをしている可能性が高い。
『Single Responsibility Principle：単一責任の原則』に違反している。
機能追加などにより複雑さが増しやすい。

軽い気持ちでやっちゃいがちですので、アンチパターンとして認識しましょう。

GitHub

サンプルコードは以下のリポジトリにあります。
随時アンチパターンを追加していく予定です。

github.com

今日も平常運転

楽しい人生を諦めないためのブログ