カテゴリー : リファクタリング

 

フィールドのカプセル化

  • 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を記述することで、後からコードを読む人に「どちらの事象も等しく発生する」ことを伝えることができます。
 

リファクタリングと一時変数

  • 2014/5/7 06:56
リファクタリングでよく質問されるのが「結局一時変数って使っちゃダメなの？」という疑問です。
 
私は「後からコードを読む人が混乱する名前や使い方をせず、コードの可読性が上がるのであれば使用してよい」と答えています。
 
後からコードを読む人が混乱する名前を付けている場合は以下のリファクタリングを検討します。
●
変数名の変更
 
後からコードを読む人が混乱する使い方をしている場合は以下のリファクタリングを検討します。
●
一時変数の分離
●
制御フラグの削除
 
無駄な一時変数を使用している場合は以下のリファクタリングを検討します。
●
一時変数のインライン化
●
問い合わせによる一時変数の置き換え
 
分類はあくまで目安としてお考えください。実際はこれら全てを複合してリファクタリングすることになります。
 
一時変数を「使え」というリファクタリングと、「使うな」というリファクタリングがあり最初は混乱しますが、目的は「コードの可読性を上げる」ですので、あなたの主観で読み易ければそこでリファクタリングを終了しても構いません。
 
全ての一時変数を除去することは不可能です。
 

一時変数の分離

  • 2014/5/6 16:52
一時変数を使いまわしている個所があったら、目的ごとに別々の一時変数を使いましょう。
 
●
リファクタリング前のコード
 
    int temp = right - left;
        ・・・
    temp = bottom - top;
        ・・・
 
上記例では「temp」という一時変数に右の座標から左の座標を引いた値を代入しています。おそらく「幅」を表したいのだと思いますが、コードの下部では下の座標から上の座標を引いた値を再代入しています。
 
こちらは「高さ」を表わしているのでしょうか？これは明らかに用途が違う値を同じ一時変数で管理しているため、後からコードを読む人が混乱します。
 
やはり「幅」と「高さ」の一時変数を分けるべきでしょう。
 
●
一時変数分離後のコード
 
    int width = right - left;
        ・・・
    int height = bottom - top;
        ・・・
 
こうしておけば、後からコードを読む人が「幅」と「高さ」で悩まなくてすみます。
 

問い合わせによる一時変数の置き換え

  • 2014/5/3 23:32
前回のリファクタリング「説明用変数の導入」で冗長になったコードに、「問い合わせによる一時変数の置き換え」を試してみましょう。
 
「問い合わせによる一時変数の置き換え」は意味のある式を一時変数に代入して利用しているため、メソッド内でしかその値を活用できない場合に用いられ、「意味のある式」を問い合わせメソッドとして抽出するリファクタリングです。
 
とくにフィールドから意味のある値を導出している場合に効果を発揮します。
 
●
説明用変数の導入後のコード
 
    // ビットマップを中央に描画
    private void DrawCenter(Bitmap bitmap)
    {
        // ピクチャーボックス設定用ビットマップを生成
        Bitmap canvas = new Bitmap(PictureBox1.Width, PictureBox1.Height);

        // キャンバスのGraphicsオブジェクトを生成
        Graphics g = Graphics.FromImage(canvas);

        // 中央描画用x座標を取得
        int centerDrawXPosition = (canvas.Width / 2) - (bitmap.Width / 2);

        // 中央表示用y座標を取得
        int centerDrawYPosition = (canvas.Height / 2) - (bitmap.Height / 2);

        // キャンバスの中央にビットマップを描画
        g.DrawImage(bitmap, centerDrawXPosition, centerDrawYPosition);

        // Graphicsオブジェクトのリソースを解放
        g.Dispose();

        // ピクチャーボックスに描画
        PictureBox1.Image = canvas;
    }
 
コードを眺めると、やはり中央描画用x座標およびy座標を計算している個所が目につきます。
 
キャンバスの幅と高さを利用しておりますが、これらはもともとピクチャーボックスの幅と高さからビットマップを生成していますので、問い合わせメソッドとして抽出してみる価値がありそうです。
 
●
説明用変数の導入後のコード
 
    // ビットマップを中央に描画
    private void DrawCenter(Bitmap bitmap)
    {
        // ピクチャーボックス設定用ビットマップを生成
        Bitmap canvas = new Bitmap(PictureBox1.Width, PictureBox1.Height);

        // キャンバスのGraphicsオブジェクトを生成
        Graphics g = Graphics.FromImage(canvas);

        // キャンバスの中央にビットマップを描画
        g.DrawImage(bitmap,
            GetCenterDrawXPosition(bitmap), GetCenterDrawYPosition(bitmap));

        // Graphicsオブジェクトのリソースを解放
        g.Dispose();

        // ピクチャーボックスに描画
        PictureBox1.Image = canvas;
    }

    // 中央描画用x座標を取得
    private int GetCenterDrawXPosition(Bitmap bitmap)
    {
        return (PictureBox1.Width / 2) - (bitmap.Width / 2);
    }

    // 中央描画用y座標を取得
    private int GetCenterDrawYPosition(Bitmap bitmap)
    {
        return (PictureBox1.Height / 2) - (bitmap.Height / 2);
    }
 
メソッドの数が増えてしまいましたが、DrawCenterメソッドは簡潔に表現できるようになりましたし、中央描画用x座標およびy座標を取得するメソッドは再利用が可能になりました。
 
「一時変数のインライン化」に始まり、「説明用変数の導入」、今回の「問い合わせによる一時変数の置き換え」は、よく使われるリファクタリングなので必ず覚えておきましょう。
 

説明用変数の導入

  • 2014/5/2 22:16
前回のリファクタリングでは「一時変数のインライン化」を行いましたが、まだ分かりづらいので「説明用変数の導入」を試します。
 
●
一時変数のインライン化後のコード
 
    // ビットマップを中央に描画
    private void DrawCenter(Bitmap bitmap)
    {
        // ピクチャーボックス設定用ビットマップを生成
        Bitmap canvas = new Bitmap(PictureBox1.Width, PictureBox1.Height);

        // キャンバスのGraphicsオブジェクトを生成
        Graphics g = Graphics.FromImage(canvas);

        // キャンバスの中央にビットマップを描画
        g.DrawImage(bitmap, (canvas.Width / 2) - (bitmap.Width / 2),
            (canvas.Height / 2) - (bitmap.Height / 2));

        // Graphicsオブジェクトのリソースを解放
        g.Dispose();

        // ピクチャーボックスに描画
        PictureBox1.Image = canvas;
    }
 
やはり中央にビットマップを描画する個所の計算式が分かりづらいので、中央描画用x座標「centerDrawXPosition」と中央表示用y座標「centerDrawYPosition」の説明用変数を導入してみます。
 
●
説明用変数の導入後のコード
 
    // ビットマップを中央に描画
    private void DrawCenter(Bitmap bitmap)
    {
        // ピクチャーボックス設定用ビットマップを生成
        Bitmap canvas = new Bitmap(PictureBox1.Width, PictureBox1.Height);

        // キャンバスのGraphicsオブジェクトを生成
        Graphics g = Graphics.FromImage(canvas);

        // 中央描画用x座標を取得
        int centerDrawXPosition = (canvas.Width / 2) - (bitmap.Width / 2);

        // 中央表示用y座標を取得
        int centerDrawYPosition = (canvas.Height / 2) - (bitmap.Height / 2);

        // キャンバスの中央にビットマップを描画
        g.DrawImage(bitmap, centerDrawXPosition, centerDrawYPosition);

        // Graphicsオブジェクトのリソースを解放
        g.Dispose();

        // ピクチャーボックスに描画
        PictureBox1.Image = canvas;
    }
 
DrawImageメソッドの引数は分かりやすくなりましたが、今度はコードが冗長になりました。次回は「問い合わせによる一時変数の置き換え」を試してみます。
 

一時変数のインライン化

  • 2014/5/2 00:06
一時変数が簡単な式で初期化され、その後に代入されることがない場合は「一時変数のインライン化」を検討します。
 
以下にピクチャーボックスの中央に画像を表示するサンプルを示します。
 
    // ビットマップを中央に描画
    private void DrawCenter(Bitmap bitmap)
    {
        // ピクチャーボックス設定用ビットマップを生成
        Bitmap canvas = new Bitmap(PictureBox1.Width, PictureBox1.Height);

        // キャンバスのGraphicsオブジェクトを生成
        Graphics g = Graphics.FromImage(canvas);

        // キャンバスの幅と高さを設定
        int canvasWidth = canvas.Width;
        int canvasHeight = canvas.Height;

        // ビットマップの幅と高さを設定
        int bitmapWidth = bitmap.Width;
        int bitmapHeight = bitmap.Height;

        // キャンバスの中央にビットマップを描画
        g.DrawImage(bitmap, (canvasWidth / 2) - (bitmapWidth / 2),
            (canvasHeight / 2) - (bitmapHeight / 2));

        // Graphicsオブジェクトのリソースを解放
        g.Dispose();

        // ピクチャーボックスに描画
        PictureBox1.Image = canvas;
    }
 
この例ですと「canvasWidth」、「canvasHeight」、「bitmapWidth」、「bitmapHeight」の一時変数が「簡単な式で初期化され、その後に代入されることがない場合」に該当すると思います。
 
それでは「一時変数のインライン化」を実施してみましょう。
 
●
一時変数のインライン化後のコード
 
    // ビットマップを中央に描画
    private void DrawCenter(Bitmap bitmap)
    {
        // ピクチャーボックス設定用ビットマップを生成
        Bitmap canvas = new Bitmap(PictureBox1.Width, PictureBox1.Height);

        // キャンバスのGraphicsオブジェクトを生成
        Graphics g = Graphics.FromImage(canvas);

        // キャンバスの中央にビットマップを描画
        g.DrawImage(bitmap, (canvas.Width / 2) - (bitmap.Width / 2),
            (canvas.Height / 2) - (bitmap.Height / 2));

        // Graphicsオブジェクトのリソースを解放
        g.Dispose();

        // ピクチャーボックスに描画
        PictureBox1.Image = canvas;
    }
 
コードはすっきりしましたが、まだ分かりづらいですね。次回は「説明用変数の導入」を試してみましょう。
g h T
 4,310 Total Views

PAGE NAVI