リファクタリング

リファクタリングとは

Wikipediaの記事によると、リファクタリングとは次のような操作を指します。

リファクタリング (refactoring) とは、コンピュータプログラミングにおいて、プログラムの外部から見た動作を変えずにソースコードの内部構造を整理することである。また、いくつかのリファクタリング手法の総称としても使われる。ただし、十分に確立された技術とはいえず、また「リファクタリング」という言葉に厳密な定義があるわけではない。

引用部分にも書かれている通り、リファクタリングという言葉に厳密は定義が無いので、身の回りでも複数の意味合いでリファクタリングという用語が用いられるように感じます。
この記事では、以下の目的を達成するために何らかのコードの改善を行う操作とします。

コードの可読性を向上させる (読みやすくする)
コードを読んだ時に意図を伝わりやすくする (読み誤りを減らす)
不具合の原因になりやすい記述を改める (品質の向上)

この記事では、コードレビューで良く指摘が挙がる事項に対するリファクタリング例を、簡単なソースを交えて紹介します。

注意事項

ソースコードは例示したものです(実際に指摘があったコードではありません)。
以下で行った修正が、必ずしも適切なリファクタリング結果となるとは限りません。
適切なリファクタリングの方針は、そのコードが満たすべき仕様等でケースバイケースです。
用語の細かな解説は割愛しますので、必要に応じて他の記事等も参考にしてください。
あくまで参考に留めて頂ければ幸いです。

リファクタリング例

初期状態

以下のメソッドを修正していきます。
よろしければ、このメソッドの問題点とその改善案を考えてみてください。

public static bool CheckParams(int min, int max)
{
    int result = 0;

    if (min < 0)
    {
        result = 1;
    }

    if (max < 0)
    {
        result = 2;
    }

    if (min > int.MaxValue || max > int.MaxValue)
    {
        result = 9;
    }

    if (min > max)
    {
        result = 3;
    }

    if (min == max)
    {
        result = 4;
    }

    if ((result == 0) == false)
    {
        return true;
    }

    return false;
}

基本的な事項

ドキュメントコメントを付ける
このメソッドがどういった処理を行うのか、取りうるパラメータや戻り値の説明をドキュメントコメントに記載することで明確にします。
Check という用語を避ける
Check という用語は含意が広すぎて意図する意味合いが分かりづらい為、避けた方が良いとされることがあります。
ここでは Validate (検証) と検証の対象を組み合わせて ValidateRange としました。 ¹
bool値の等価評価( == true や == false )を行わない
比較演算子の結果(今回の例では result == 0)は、それ自体が bool値となる為、if文の条件式でさらに true や false と比較する必要はありません。
今回の例では、false との等価評価はせずに、if (result != 0)と書けば成立します。 ²
正常な場合にtrueを返すようにする
メソッドの戻り値がbool型の場合、true/false がそれぞれどういった意味を持つか、あいまいになりがちです。
今回はメソッド名を Validate としましたので、検証結果が正常系の場合に、true を返すことにします。 ³

/// <summary>
/// 範囲を表すパラメータの整合性チェックを行う。
/// </summary>
/// <param name="min">範囲の下限</param>
/// <param name="max">範囲の上限</param>
/// <returns>整合性に問題が無い場合、Trueを返す</returns>
public static bool ValidateRange(int min, int max) // "Check" という用語を避ける
{
    int result = 0;

    if (min < 0)
    {
        result = 1;
    }

    if (max < 0)
    {
        result = 2;
    }

    if (min > int.MaxValue || max > int.MaxValue)
    {
        result = 9;
    }

    if (min > max)
    {
        result = 3;
    }

    if (min == max)
    {
        result = 4;
    }

    if (result == 0) // bool値の等価評価( == true や == false )を行わない
    {
        return true; // 正常な場合にTrueを返すようにする
    }

    return false;
}

`else if`文を利用する

else if を利用して無駄な条件判断を回避する
元のコードでは、いずれかのエラー条件に合致した場合でも後続の判定を行っている為、無駄な条件判断を行っている可能性があります。
メソッドの仕様を確認した上で、後続の判定が不要であれば else if を利用して、実行される条件判断が最小限になるようにします。
ただし、複数の条件に合致した場合に最終的な結果(今回の例ではresultへ設定される値)が変わる場合がある為、条件判断を行う順序を見直す必要がある場合があります(メソッドの仕様をよく確認してください)。 ⁴

public static bool ValidateRange_WithElseIf(int min, int max)
{
    int result = 0;

    if (min < 0)
    {
        result = 1;
    }
    else if (max < 0)
    {
        result = 2;
    }
    else if (min > int.MaxValue || max > int.MaxValue)
    {
        result = 9;
    }
    else if (min > max)
    {
        result = 3;
    }
    else if (min == max)
    {
        result = 4;
    }

    if (result == 0)
    {
        return true;
    }

    return false;
}

変数の初期化

無駄な初期化を行わないようにする
変数の宣言時点で不必要な初期化が行われている場合、初期化を行わないようにします。
変数の再代入を行わないようにする
同じ変数に何度も値が代入されると変数の値が追いづらくなる為、特に必要なケースでなければ変数への値の代入は1度のみ行うようにします。
今回の例では、変数に格納すべき値が確定してから初めて代入するようにしました。

public static bool ValidateRange_WithElseIf_NotInit(int min, int max)
{
    int result; // この時点では初期化しない

    if (min < 0)
    {
        result = 1;
    }
    else if (max < 0)
    {
        result = 2;
    }
    else if (min > int.MaxValue || max > int.MaxValue)
    {
        result = 9;
    }
    else if (min > max)
    {
        result = 3;
    }
    else if (min == max)
    {
        result = 4;
    }
    else
    {
        result = 0; // 正常であることが判明してから初めて 0 を代入する
    }

    if (result == 0)
    {
        return true;
    }

    return false;
}

マジックナンバーの排除

定数を宣言してマジックナンバーを排除する
コード上に現れる、特別な意味を持ったリテラルをマジックナンバーと言います。
result に設定される値が int値の場合、その値だけを見ても意味をくみ取ることが出来ない為、マジックナンバーは極力避けた方が良いです。
今回は、メソッドの先頭で定数を宣言し、マジックナンバーに名前(定数名)を付与することで、resultに設定された値の意図をくみ取りやすくしました。 ⁵

public static bool ValidateRange_WithConst(int min, int max)
{
    const int NORMAL = 0;
    const int MIN_MINUS = 1;
    const int MAX_MINUS = 2;
    const int OVER_CAPACITY = 9;
    const int REVERSAL = 3;
    const int SAME = 4;

    int result = NORMAL;

    if (min < 0)
    {
        result = MIN_MINUS;
    }

    if (max < 0)
    {
        result = MAX_MINUS;
    }

    if (min > int.MaxValue || max > int.MaxValue)
    {
        result = OVER_CAPACITY;
    }

    if (min > max)
    {
        result = REVERSAL;
    }

    if (min == max)
    {
        result = SAME;
    }

    if (result == NORMAL)
    {
        return true;
    }

    return false;
}

ガード節

ガード節を利用する
今回のコードでは、resultに設定した値はメソッドが返す(returnする)値を判定することにのみ使用しています。
言い換えれば、if文の各条件に合致した時点でメソッドが false を返すことが確定することになります。
こういったケースでは、returnすべき値が確定した時点でreturnすることでコードの可読性が向上します。

public static bool ValidateRange_GuardClause(int min, int max)
{
    if (min < 0)
    {
        return false; // エラーが確定した時点で false を返す
    }

    if (max < 0)
    {
        return false;
    }

    if (min > int.MaxValue || max > int.MaxValue)
    {
        return false;
    }

    if (min > max)
    {
        return false;
    }

    if (min == max)
    {
        return false;
    }

    return true;
}

ローカルメソッド

条件判断処理をローカルメソッドに置き換える
result に設定した値を後続処理で使用する必要がある場合、ガード節を利用してメソッドをreturnすることが出来ない場合があります。
その場合、条件判断処理をローカルメソッド(または、通常のメソッド)に置き換えることで、ガード節を使用しつつ result に設定した値を活用することが出来ます。

public static bool ValidateRange_WithLocalMethod(int min, int max)
{
    const int NORMAL = 0;
    const int MIN_MINUS = 1;
    const int MAX_MINUS = 2;
    const int OVER_CAPACITY = 9;
    const int REVERSAL = 3;
    const int SAME = 4;

    int errors()
    {
        if (min < 0)
        {
            return MIN_MINUS;
        }

        if (max < 0)
        {
            return MAX_MINUS;
        }

        if (min > int.MaxValue || max > int.MaxValue)
        {
            return OVER_CAPACITY;
        }

        if (min > max)
        {
            return REVERSAL;
        }

        if (min == max)
        {
            return SAME;
        }

        return NORMAL;
    }

    int result = errors(); // ローカルメソッドの戻り値で、result に設定する値を受け取る

    // ここで result に対して処理が必要な場合

    if (result == NORMAL)
    {
        return true;
    }

    return false;
}

パターンマッチング

パターンマッチングを利用する
パターンマッチングを利用することで可読性を向上させることが出来る場合があります。
今回のケースでは利用できませんので、参考まで(以下のコード例は元のコードとは関係ありません)。

public static bool ValidateRange_WithPatternMatching(int value)
{
    const int NORMAL = 0;
    const int UNDER_MIN = 1;
    const int OVER_MAX = 2;

    int result = value switch
    {
        < 0 => UNDER_MIN,
        > 100 => OVER_MAX,
        _ => NORMAL
    };

    if (result == NORMAL)
    {
        return true;
    }

    return false;
}

余談

今回のコードで例示したエラー条件のうち、以下は絶対に成立しません。

if (min > int.MaxValue || max > int.MaxValue)
{
    result = 9;
}

int.MaxValue は int型の値が取りうる最も大きい値を表す定数ですが、min、max は int型の引数なので int.MaxValue より大きくなることはあり得ないためです。
こう言った成立し得ない条件判断は、何らかのバグによることがありますので、仕様をよく確認されることをおすすめします。
単にありえない条件判断が記述されただけであれば、この条件判断を除外してもコードの実行結果には影響がありません。

また、今回は対象としていませんが、コード上に実装コメントを付記することでコードの意図が伝わりやすくなります。
例えば条件判断(if文)では、コードから実装の意図が十分に読み取れるか、コメントで補足する必要があるか、検討することをおすすめします。

使用すべきまたは使用すべきで無い用語はプロジェクトごとにルールがある場合、そのルールに従ってください。 ↩
4点目の「正常な場合にtrueを返すようにする」修正に合わせる形で、コード例では if (result == 0) としています(returnする値が変わります)。 ↩
どちらを true とみなすかはプロジェクトごとにルールがある場合があります。その場合は、ルールに従って true/false の戻り値を定義してください。 ↩
今回のコードでは、result に設定された値が 0 であるか否かのみを問題としている為、条件判断の順序は問題になりませんでした。 ↩
例ではメソッドの中に定数を宣言しましたが、必要に応じてメソッドが属するクラスや、システムで共通の定数クラス等に定義することが望ましい場合もあります。 ↩