1
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?

More than 1 year has passed since last update.

FizzBuzz問題を使って、コードの読みやすさを考えてみた

Last updated at Posted at 2023-07-11

はじめに

基本的なC#の文法(関数とクラスの書き方)を把握しているが、実際のコードでどのように活用すればよいかわからない方を対象とします。
FizzBuzz問題とそれを派生した問題を様々な書き方で解くことを通じて、関数とクラスの利用方法を筆者なりの考え方を伝えます。

関数やクラスは何のために使うの?

プログラムを実行する機械は、ソースコードに従って動くだけなので、関数やクラスを使わなくても機械は困りません。

関数やクラスはプログラムを読む「人間」がソースコードの理解を容易にするために利用するものです。
ソースコードは機械が動作するためではなく、未来の自分や他の人に向けた手紙です。

「人間」が読みやすいプログラムは、ソースコードから何をしたいのか(仕様)を読み取ることができることであり、
そのためには、以下の内容が重要と筆者は考えています。

  1. 変数と関数へのスコープ(アクセス可能な範囲)が必要最小限になっていること
  2. 処理順序の工夫や関数化によって、冗長な処理がないこと
  3. 変数や関数の命名規則記載(コーディングルール)が統一されていること

今回は1と2について説明します。
3はマイクロソフトのコーディング規約を参考にするのがおすすめです。

FizzBuzz問題

以下のFizzBuzz問題に対して、いくつかの解答例を示し、
それぞれに対して筆者が考える良い点、悪い点をコメントとして追加します。

FizzBuzz問題

  • 1から200までの数値を1ずつ増加させて表示する。
  • 数値が3の倍数の時は、数値の代わりにFizzと表示する。
  • 数値が5の倍数の時は、数値の代わりにBuzzと表示する。
  • 数値が3と5の倍数の時は、数値の代わりにFizzBuzzと表示する。

出力例

1 2 Fizz 4 Buzz 6 ... 14 FizzBuzz 16 ... 199 Buzz

実装例1 そのまま実装する

問題文に書かれている数値と文字列表示の条件を素直にif-else文を使って書いてみます。

public static void Main()
{
    for (int i = 1; i <= 200; i++)
    {

        if (i % 3 == 0 && i % 5 == 0)
        {
            // <コードの悪い点>
            // 15の倍数の時の処理が冗長。
            // 3の倍数の処理と5の倍数の処理を連続して実行すればよい。
            Console.WriteLine("FizzBuzz");
        }
        else if (i % 3 == 0)
        {
            // <コードの悪い点>
            // 定数3と5が複数回登場しているため、
            // これらが同じ意味を持つのか判断が難しい。
            // readonly int fizzSpeakTiming = 3;
            // と変更不可の変数を宣言し、
            // これ以降は変数を経由してアクセスすることで、
            // 同じ意味で利用していることを明示できる。
            Console.WriteLine("Fizz");
        }
        else if (i % 5 == 0)
        {
            Console.WriteLine("Buzz");
        }
        else
        {
            Console.WriteLine(i.ToString());
        }
    }
}

実装例2 処理順序を工夫する

15の倍数の時の処理は、3の倍数の処理と5の倍数の処理を連続して実行すればよいことに着目して書いてみます。

public static void Main()
{
    for (int i = 1; i <= 200; i++)
    {
        // <コードの良い点>
        // isSpeakedはforループ毎にリセットされる値なので、
        // forループの外からアクセス可能にする必要はない。
        // そのため、変数の読み書き可能な範囲(スコープ)は適切。
        bool isSpeaked = false;

        if (i % 3 == 0)
        {
            isSpeaked = true;
            Console.Write("Fizz");
        }

        // <コードの悪い点>
        // 直前のif(i %3)とほぼ同じ処理を繰り返しているため、冗長。
        // 違いがどこにあるか注意して読む必要がある。
        // 値だけが異なり、手続き(if文やfor文の構造)が同じ場合は、
        // 関数にまとめることができる。
        if (i % 5 == 0)
        {
            isSpeaked = true;
            Console.Write("Buzz");
        }

        if (!isSpeaked)
        {
            Console.Write(i.ToString());
        }

        Console.WriteLine();
    }
}

実装例3 関数化する(あまりよくない例)

実装例2をベースに関数を使って書いてみます。

static int g_Count = 0;
static bool g_IsSpeaked = false;

public static void Speak(int inSpeakTiming, string inMessage)
{
    if (g_Count % inSpeakTiming != 0)
    {
        // <コードの悪い点>
        // Speak関数ではg_Countの値を取得するだけで、変更していない。
        // しかし、g_Countはグローバル変数でどこからでも変更可能なため、
        // 読み手はこの関数内でg_Countの値が変更されていないか
        // 注意して読む必要がある。
        // g_Countはグローバル変数ではなく、関数の引数とすることで、
        // この値が変更されないことを読み手に示すほうが良い。
        return;
    }

    ShowMessage(inMessage);

    // <コードの悪い点>
    // 関数の動作結果としてg_Speakedの値を変更しているが、
    // g_Countと同様の理由で、
    // g_Speakedもグローバル変数にしないほうが良い。
    // 単一の値を結果として返すのであれば、
    // 関数の戻り値を活用するほうが良い。
    g_IsSpeaked = true;
}

public static void ShowMessage(string inMessage)
{
    // <コードの悪い点>
    // 関数内の処理内容が単純でわかりやすいため、
    // 関数にする必要がない。逆に冗長になり、読みにくくなる。
    // パッと見てもわかりにくい処理や数十行で1連の処理を
    // 関数にするべき。
    Console.Write(inMessage);
}

public static void Main()
{
    for (g_Count = 1; g_Count <= 200; g_Count++)
    {
        // <コードの悪い点>
        // g_IsSpeakedがfalseに初期化された後、
        // Main関数内で変更されていないため、
        // どこで変更されているのかを把握するためには、
        // g_IsSpeakedが利用されていることを記憶しつつ、
        // すべてのコードを読む必要がある。
        //
        // 今回の例ではグローバル変数は2つと少ないが、
        // これが多いコードになるに従って、
        // どのグローバル変数の変更箇所の把握が困難になってくる。
        // グローバル変数を使う場合は、
        // 関数の引数にグローバル変数を入れるなどして、
        // グローバル変数へのアクセス個所を少なくするとよい。
        g_IsSpeaked = false;

        // <コードの良い点>
        // 似たような手続きの繰り返しを回避できている。
        // 加えて、変更されない値(3,Fizz)、(5,Buzz)が
        // それぞれの関数の引数として与えられているため、
        // 3とFizz、5とBuzzに関連性があることがすぐに理解できる。
        Speak(3, "Fizz");
        Speak(5, "Buzz");

        if (!g_IsSpeaked)
        {
            Console.Write(g_Count.ToString());
        }

        Console.WriteLine();
    }
}

実装例4 より適切に関数化する

実装例3をベースに改良して書いてみます。

public static bool Speak(
    int inCount, int inSpeakTiming, string inMessage)
{
    // <コードの良い点>
    // グローバル変数を利用しておらず、
    // 引数の内容によってのみ関数の動作が決まるため、
    // この関数を他のコードにそのままコピーしても利用できる。
    // (関数の再利用性が高い)
    if (inCount % inSpeakTiming != 0)
    {
        return false;
    }

    Console.Write(inMessage);
    return true;
}

public static void Main()
{
    for (int count = 1; count <= 200; count++)
    {
        // <コードの良い点>
        // 変数isSpeakedのスコープ(読み書き可能な範囲)が
        // forループ内部に限定されているため、
        // 変数の変更範囲が把握しやすい
        bool isSpeaked = false;

        isSpeaked |= Speak(count, 3, "Fizz");

        // <コードの悪い点>
        // (3,Fizz)、(5,Buzz)は、変更されない値で、
        // Speak関数でしか利用されないため、
        // ループごとにこれらの値を指定する必要はない。
        // 関数とそれに関連する変数をセットで扱うためには、
        // クラスを利用する。
        isSpeaked |= Speak(count, 5, "Buzz");

        if (!isSpeaked)
        {
            Console.Write(count.ToString());
        }

        Console.WriteLine();
    }
}

実装例5 クラスを利用する

class FizzBuzzVersion4
{
    class Speaker
    {
        // <コードの良い点>
        // c_SpeakTimingとc_Messageのアクセス権は、
        // 以下の理由からprivate readonlyとする。
        // ・デフォルト値(0や空文字列)を入れると意図しない動作になる
        // ・1度だけ初期化が必要で、それ以降変更されない
        // ・クラス外に公開する必要がない
        private readonly int c_SpeakTiming;
        private readonly string c_Message;

        // <コードの良い点>
        // コンストラクタ実行時に、
        // SpeakTimingとMessageを初期化する。
        // デフォルトコンストラクタを実装しないことで、
        // 初期化忘れが発生しない
        public Speaker(int inSpeakTiming, string inMessage)
        {
            c_SpeakTiming = inSpeakTiming;
            c_Message = inMessage;
        }

        // <コードの良い点>
        // SpeakTimingとMessageはコンストラクタで初期化済みのため、
        // Speak関数に渡す引数はinCountのみでよい。
        public bool Speak(int inCount)
        {
            if (inCount % c_SpeakTiming != 0)
            {
                return false;
            }

            Console.Write(c_Message);
            return true;
        }
    }

    public static void Exec()
    {
        // <コードの良い点>
        // Speakerクラスを配列にまとめることで、
        // Speakerの増減や値の変更に対処しやすくなっている。
        IReadOnlyCollection<Speaker> speakerList = new List<Speaker>()
        {
            new Speaker(3, "Fizz"),
            new Speaker(5, "Buzz"),
        };

        for (int count = 1; count <= 200; count++)
        {
            bool isSpeaked = false;
            foreach (var speaker in speakerList)
            {
                isSpeaked |= speaker.Speak(count);
            }

            if (!isSpeaked)
            {
                Console.Write(count.ToString());
            }

            Console.WriteLine();
        }
    }
}

まとめ

  • 変数定義のポイント

    • 変数にアクセスできるスコープは必要最小限にして、コード内の近い行にまとめる。
    • 1度しか設定されず、それ以降変更されない変数は積極的にreadonlyやconstを付ける。
  • 関数、クラスを利用するポイント

    • 値だけ異なり、手続き(if文やfor文の構造)が同じ処理を繰り返している場合は関数にまとめる。
    • パッとみて処理内容がわからない処理や数十行で1つの意味を持つ処理は関数にまとめる。(1関数当たり100行以内が筆者の目安。それ以上になる場合は複数の関数に分ける)
    • 関数と変数に関連性を持たせたいときにクラスを利用する。(1クラス当たり300行が筆者の目安。それ以上になる場合は複数のクラスに分ける。)

今回はコードが短いため、どの解答例でも大きな差はないように感じるかもしれませんが、コードの量が増えるにつれて効果が出てきます。

1
0
2

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
1
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?