重複した条件記述の断片の統合

  • 2014/6/29 10:59
条件分岐全てに同じコードが存在する場合は、そのコードを条件分岐の外に移動します。
 
たとえば「価格を登録する」というメソッドが以下のように定義されていたとします。
 
        // 価格を登録します
        private void RegisterPrice(ConsumptionTax taxType)
        {
            // 内税の場合
            if (taxType == ConsumptionTax.TaxIncluded)
            {
                // 内税の価格を計算
                CalculateTaxIncludedPrice();

                // 登録する
                Register();
            }
            else
            {
                // 外税の価格を計算
                CalculateIncludeItemPrice();

                // 登録する
                Register();
            }
        }
 
ソースを見ると、登録するための「Register」メソッドが条件分岐全てに存在しますので、条件分岐の外へ移動します。
 
        // 価格を登録します
        private void RegisterPrice(ConsumptionTax taxType)
        {
            // 内税の場合
            if (taxType == ConsumptionTax.TaxIncluded)
            {
                // 内税の価格を計算
                CalculateTaxIncludedPrice();
            }
            else
            {
                // 外税の価格を計算
                CalculateIncludeItemPrice();
            }

            // 登録する
            Register();
        }
 
こうしておけば条件により分岐する処理と、条件によらず必ず実行しなければならない処理が明確になりますので、保守性が向上します。
 

引数の追加

  • 2014/6/21 07:21
メソッドに渡す引数が足りない場合は追加します。
 
「消費税を取得する」メソッドが以下のように定義されていたとします。
 
        // 消費税を取得します
        public decimal GetConsumptionTax()
        {
            return UnitPrice * 0.08M;
        }
 
これは内部で一時変数を使用していますので、なくすためのリファクタリングをしなければなりません。
 
今回は「引数の追加」で対応します。
 
        // 消費税を取得します
        public decimal GetConsumptionTax(decimal taxRate)
        {
            return UnitPrice * taxRate;
        }
 
ただし、やみくもに引数を追加してはいけません。引数が増えすぎるようなら「引数オブジェクトの導入」を検討してください。
 

引数の削除

  • 2014/6/19 07:07
メソッドに渡した引数が使われていない場合は削除します。
 
「setメソッドの削除」で作成した絵本クラスに、「消費税を取得する」メソッドを追加したとします。
 
        // 税率
        private const decimal TAX_RATE = 0.08M;

        // 消費税を取得します
        public decimal GetConsumptionTax(int taxRate)
        {
            return UnitPrice * TAX_RATE;
        }
 
消費税を取得する「GetConsumptionTax」メソッドに税率「taxRate」の引数を渡していますが、消費税の計算式では絵本クラスで宣言した定数の税率を使用しているようです。
 
この場合、引数の税率は無意味なので削除します。
 
        // 消費税を取得します
        public decimal GetConsumptionTax()
        {
            return UnitPrice * TAX_RATE;
        }
 

setメソッドの削除

  • 2014/6/15 16:03
初期化の際に設定し、その後変更してはいけないフィールドにsetメソッドや、プロパティのsetアクセサが定義されている場合は削除します。
 
たとえば「絵本」を管理するためのクラスを作成し、コンストラクタで単価を設定したとします。
 
    // 絵本
    public class PictureBooks
    {
        // 単価
        private int _unitPrice;

        // コンストラクタ
        public PictureBooks(int unitPrice)
        {
            this._unitPrice = unitPrice;
        }
    }
 
その後、他のクラスで絵本の単価を知る必要があり、単価プロパティを用意したとします。
 
        // 単価
        public int UnitPrice
        {
            get
            {
                return this._unitPrice;
            }
            set
            {
                this._unitPrice = value;
            }
        }
 
思わずsetアクセサも定義してしまいましたが、これでは絵本クラスの外から単価を編集できてしまいます。
 
これはマズイので「setメソッドの削除」を用いてsetアクセサを削除しましょう。
 
        // 単価
        public int UnitPrice
        {
            get
            {
                return this._unitPrice;
            }
        }
 
こうしておけば絵本クラスの外から単価が編集されることがなくなります。
 
もし絵本クラス内での単価変更を許可する場合は、setアクセサのアクセス属性で制御します。
 
        // 単価
        public int UnitPrice
        {
            get
            {
                return this._unitPrice;
            }
            private set
            {
                this._unitPrice = value;
            }
        }
 
上記例では、単価を絵本クラスの外に公開しますが、設定は絵本クラスのみが行えます。
 

メソッドの隠蔽

  • 2014/6/8 21:11
クラス内部で使用するメソッドが「public」で宣言されている場合は「メソッドの隠蔽」を検討します。
 
たとえば「売価を設定する」メソッドが「public」で宣言されていたとします。
 
        // 売価を設定します
        public void SetSellingPrice()
        {
            ・・・
        }
 
このメソッドを調べてみると、クラス内部でしか使用されていませんでした。その場合は「メソッドの隠蔽」を用いてアクセス属性を「private」に変更します。
 
        // 売価を設定します
        private void SetSellingPrice()
        {
            ・・・
        }
 
必要最低限のメソッドのみ公開することで、後からクラスを利用する人が、どのメソッドを呼び出せばよいのか分かりやすくなります。
 
※
メソッドが派生クラスで使用されている場合は、アクセス属性を「protected」にしてください。
 

フィールドのカプセル化

  • 2014/6/1 21:04
フィールドがpublicになっていて外部から直接アクセスできてしまう場合は、「フィールドのカプセル化」を検討します。
 
たとえば単価というフィールドがpublicに設定されていたとします。
 
        // 単価
        public int UnitPrice;
 
現在進行中のプロジェクトでは「単価には10円未満の値を設定できない」ルールがあったとすると、上記フィールドだと困ってしまいますよね？
 
単価がpublicなフィールドになっているため、本来設定されてはいけない9円や、あまつさえ-10円という数値まで代入できてしまいます。
 
これでは使用する個所全てで「単価が10円未満の場合」のチェックが必要になります。
 
        // 単価の下限
        private const int UNIT_PRICE_MINIMUM = 10;

        // 売価を設定します
        private void SetSellingPrice()
        {
            if (UnitPrice < UNIT_PRICE_MINIMUM)
            {
                throw new Exception(string.Format(“単価が{0}円未満です。”,
                    UNIT_PRICE_MINIMUM));
            }

            ・・・

        }

これはいくらなんでも非効率です。「単価には10円未満の値を設定できない」ルールを守ればよいのですから、代入時点で例外をスローしたいところですね。
 
そこで有用なのがフィールドのカプセル化です。幸いC#にはプロパティの概念がありますので、プロパティを用いて書き換えてみましょう。
 
        // 単価
        private int _unitPrice = 10;

        // 単価
        public int UnitPrice
        {
            get
            {
                return this._unitPrice;
            }
            set
            {
                if (value < UNIT_PRICE_MINIMUM)
                {
                    throw new Exception(string.Format(“単価が{0}円未満です。”,
                        UNIT_PRICE_MINIMUM));
                }

                this._unitPrice = value;
            }
        }
 
こうしておけば単価に10円未満の値が代入されることはありませんので、プログラマーは業務ロジックに専念できます。
 

メソッドの抽出

  • 2014/5/25 21:31
1つのメソッドに重複したコードが存在する場合や、長すぎて読みづらい場合は「メソッドの抽出」を行います。
 
たとえば「2つのファイルを比較して、内容が一致しているかを判定する」メソッドを以下のように定義したとします。
 
●
リファクタリング前のコード
 
    // 2つのファイルを比較して、内容が一致しているかを判定します
    private bool IsMatchContents(string filePath1, string filePath2)
    {
        StreamReader sr1 = new StreamReader(filePath1);

        string fileContents1 = sr1.ReadToEnd();

        StreamReader sr2 = new StreamReader(filePath2);

        string fileContents2 = sr2.ReadToEnd();

        sr1.Close();
        sr2.Close();

        return fileContents1 == fileContents2;
    }
 
このメソッドは「ファイルの内容を読み込む」部分が重複していますので、重複部分をメソッドとして抽出し「ReadFileContents」と命名してみましょう。
 
●
リファクタリング後のコード
 
    // 2つのファイルを比較して、内容が一致しているかを判定します
    private bool IsMatchContents(string filePath1, string filePath2)
    {
        return ReadFileContents(filePath1) == ReadFileContents(filePath2);
    }

    // ファイルを読み込んで内容を返します
    private string ReadFileContents(string filePath)
    {
        StreamReader sr = new StreamReader(filePath);

        try
        {
            return sr.ReadToEnd();
        }
        finally{
            sr.Close();
        }
    }
 
「ファイルを開いたら必ず閉じる」プログラムの基本中の基本ですが、最初のコードでは非常に書きづらかった概念が、リファクタリング後は簡単に書くことができました。
 
このように単機能ごとにメソッドを抽出すると、可読性が上がるだけではなく、例外処理や必ず実行しなければならない処理が書きやすくなるメリットもあります。
 
ただし振る舞いが変わるような「メソッドの抽出」を行ってはいけません。ですから「メソッドの抽出」前後のテストは必須です。
 

メソッド名の変更

  • 2014/5/18 19:30
メソッド名が分かりづらい場合は、分かりやすい名前に変更します。
 
「問い合わせによる一時変数の置き換え」や「メソッドの抽出」など、リファクタリングを進めていくと小さな単位のメソッドが多くなっていきます。
 
メソッドの単位が小さいと理解しやすいですし、再利用性も上がりますので良いスタイルだと思いますが、メソッド名が分かりづらいと後からコードを読む人が混乱するので逆効果です。
 
その場合は「メソッド名の変更」を行います。
 
たとえば「与信限度額を取得する」メソッドの名前が「GetCLA」だったとします。これは「Credit Limit Amount」の頭文字だけをつないだようですね。
 
よくやる手法ではありますが、これでは後からコードを読む人がまったく理解できません。ここはベタに「GetCreditLimitAmount」というメソッド名にするのがベターでしょう。
 
私のローカルルールでは、単語は省略せずにそのまま連結します。メソッド名が50文字を超えたあたりから「メソッドの抽出」でもっと小さな単位のメソッドにできないかを検討します。
 
メソッド名が長いということは、そのメソッドのやることが多いため、説明が長いことを意味するからです。
 
メソッドを分けた方が分かりやすいか、メソッド名で説明した方が分かりやすいかを検討し、より分かりやすいと思える方を選択します。
 
なのでごくまれに画面の半分くらいがメソッド名といったコードができてしまいますが、そこは自信を持って「良し」としています。
 

ガード節を用いるメリット

  • 2014/5/17 21:02
私は「ガード節による入れ子条件記述の置き換え」がもっとも重要なリファクタリングだと考えています。
 
理由は「保守性が向上し仕様変更に対応しやすく」なるからです。
 
以前にも書きましたが、コードを保守する人は「この場合はどう動くのか？」にスコープを当てて追いかけます。ガード節で適宜returnしてあげると、それ以上無駄なコードを読む必要がなくなりますので保守性が向上します。
 
次に仕様変更への対応ですが、たとえば会員管理の登録処理で「住所、氏名は必須入力。男性は年収が必須入力、女性は趣味が必須入力」という仕様から、「住所は男性のみ必須にする」という変更が入ったとします。
 
●
仕様変更前のコード
 
        // 登録できるかを判定します
        private bool CanRegister()
        {
            // 住所が空の場合
            if (string.IsNullOrEmpty(AddressTextBox.Text) == true)
            {
                return false;
            }

            // 氏名が空の場合
            if (string.IsNullOrEmpty(NameTextBox.Text) == true)
            {
                return false;
            }

            // 男性の場合
            if (MenRadioButton.Checked == true)
            {
                // 年収が空の場合
                if (string.IsNullOrEmpty(AnnualIncomeTextBox.Text) == true)
                {
                    return false;
                }
            }
            else
            {
                // 趣味が空の場合
                if (string.IsNullOrEmpty(HobbyTextBox.Text) == true)
                {
                    return false;
                }
            }

            return true;
        }
 
●
仕様変更後のコード
 
        // 登録できるかを判定します
        private bool CanRegister()
        {
            // 氏名が空の場合
            if (string.IsNullOrEmpty(NameTextBox.Text) == true)
            {
                return false;
            }

            // 男性の場合
            if (MenRadioButton.Checked == true)
            {
                // 住所が空の場合
                if (string.IsNullOrEmpty(AddressTextBox.Text) == true)
                {
                    return false;
                }

                // 年収が空の場合
                if (string.IsNullOrEmpty(AnnualIncomeTextBox.Text) == true)
                {
                    return false;
                }
            }
            else
            {
                // 趣味が空の場合
                if (string.IsNullOrEmpty(HobbyTextBox.Text) == true)
                {
                    return false;
                }
            }

            return true;
        }
 
コードがリファクタリングされていたため、「住所のガード節」を男性の条件分岐の下に移動するだけで対応することができました。
 
このようにリファクタリングされたコードは、仕様変更への対応を容易にしてくれます。
 

ガード節による入れ子条件記述の置き換え

  • 2014/5/11 22:06
if – elseが入れ子になって正常系が分かりづらい場合は、「ガード節による入れ子条件記述の置き換え」のリファクタリングを行います。
 
たとえば会員管理の登録処理で「住所、氏名は必須入力。男性は年収が必須入力、女性は趣味が必須入力」という仕様があったとします。
 
そこで以下のような登録判定のメソッドを作ったのですが、かなり分かりづらいですね。
 
    // 登録できるかを判定します
    private bool CanRegister()
    {
        // 住所が空の場合
        if (string.IsNullOrEmpty(AddressTextBox.Text) == true)
        {
            return false;
        }
        else
        {
            // 氏名が空ではない場合
            if (string.IsNullOrEmpty(NameTextBox.Text) == false)
            {
                // 男性の場合
                if (MenRadioButton.Checked == true)
                {
                    // 年収が空の場合
                    if (string.IsNullOrEmpty(AnnualIncomeTextBox.Text) == true)
                    {
                        return false;
                    }
                    else
                    {
                        return true;
                    }
                }
                else
                {
                    // 趣味が入力されている場合
                    if (string.IsNullOrEmpty(HobbyTextBox.Text) == false)
                    {
                        return true;
                    }
                }
            }
        }

        return false;
    }
 
それでは「ガード節による入れ子条件記述の置き換え」を試してみましょう。
 
●
リファクタリング後のコード
 
        // 登録できるかを判定します
        private bool CanRegister()
        {
            // 住所が空の場合
            if (string.IsNullOrEmpty(AddressTextBox.Text) == true)
            {
                return false;
            }

            // 氏名が空の場合
            if (string.IsNullOrEmpty(NameTextBox.Text) == true)
            {
                return false;
            }

            // 男性の場合
            if (MenRadioButton.Checked == true)
            {
                // 年収が空の場合
                if (string.IsNullOrEmpty(AnnualIncomeTextBox.Text) == true)
                {
                    return false;
                }
            }
            else
            {
                // 趣味が空の場合
                if (string.IsNullOrEmpty(HobbyTextBox.Text) == true)
                {
                    return false;
                }
            }

            return true;
        }
 
異常系である「住所が空」と「氏名が空」はガード節を用いて最初に「return false」しています。
 
次に男性の場合は「年収が空」をガード節を用いて「return false」、女性の場合は「趣味が空」をガード節を用いて「return false」、最後に残った正常系で「return true」を返しています。
 
ここで重要なのは「ガード節は異常系や特殊系に適用し、elseは記述しない」。男性・女性のように「どちらも正常系の場合はelseを記述する」ことです。
 
elseを記述することで、後からコードを読む人に「どちらの事象も等しく発生する」ことを伝えることができます。
g h T
 7,661 Total Views

PAGE NAVI