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

  • 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;
    }
 
コードはすっきりしましたが、まだ分かりづらいですね。次回は「説明用変数の導入」を試してみましょう。
 

制御フラグの削除

  • 2014/4/28 07:03
前回の「変数名の変更」で説明したコードは、まだリファクタリングの余地があります。
 
今回は「制御フラグの削除」のリファクタリングをしてみましょう。
 
●
前回のコード
    private bool ContainsTargetValue(int[] values, int targetValue)
    {
        bool foundTargetValue = false;

        for (int i = 0; i < values.Length; i++)
        {
            if (values[i] == targetValue)
            {
                foundTargetValue = true;
            }
        }

        return foundTargetValue;
    }
 
このメソッドは「int型配列の中に対象の値が含まれるかを調べる」メソッドで、対象の値が含まれていたら「true」を返します。
 
ですから「foundTargetValue = true;」実行後はループする必要がありません。それではfor文をbreakしましょう。
 
●
for文break後のコード
    private bool ContainsTargetValue(int[] values, int targetValue)
    {
        bool foundTargetValue = false;

        for (int i = 0; i < values.Length; i++)
        {
            if (values[i] == targetValue)
            {
                foundTargetValue = true;
                break;
            }
        }

        return foundTargetValue;
    }
 
ここでメソッドをもう一度読み返してみます。
 
「foundTargetValue」という変数は、初期値が「false」で目的の値が含まれていたら「true」になり、最後return文でその値が返されます。それ以外のことは何もしていません。
 
だとすると目的の値が含まれていたらreturn文で「true」を返す、含まれていなかったらreturn文で「false」を返すという構成でも問題ないことに気付きます。
 
そうですこのメソッドでは、そもそも制御フラグが必要なかったのです。それでは制御フラグを削除した形にリファクタリングしてみます。
 
●
制御フラグ削除後のコード
    private bool ContainsTargetValue(int[] values, int targetValue)
    {
        for (int i = 0; i < values.Length; i++)
        {
            if (values[i] == targetValue)
            {
                return true;
            }
        }

        return false;
    }
 
コードがかなり簡潔に表現できたと思います。return文が2つ以上あることを嫌う人はいますが、後々の保守性が向上しますので無駄な制御フラグは必ず削除してください。
 
コードを保守する人は、コード全体を漠然と追いかけるのではなく「この場合はどう動くのか？」にスコープを当てて追いかけます。適宜return文で復帰してあるとそれ以上「無駄なコードを読まなくてよい」というメリットがあるのです。
 
私の経験上そのメリットの恩恵は計り知れません。トラブル対応で一分一秒が惜しいときに、整理されたメソッドにどれだけ助けられたことか、逆に意味不明な制御フラグにどれだけ苦しめられたことか。。
 
全ての制御フラグを削除しろとは言いませんが、明らかに無駄な制御フラグは削除するように心がけましょう。
 

変数名の変更

  • 2014/4/27 17:22
マーチン ファウラー（著）リファクタリングでは名言されていませんが、分かりづらい変数名を適切な名前に変更することは、とても有効なリファクタリングだと思います。
 
たとえばint型配列の中に対象の値が含まれるかを調べるメソッドが、以下のように定義されていたとします。
 
    private bool ContainsTargetValue(int[] values, int targetValue)
    {
        bool flag = false;

        for (int i = 0; i < values.Length; i++)
        {
            if (values[i] == targetValue)
            {
                flag = true;
            }
        }

        return flag;
    }
 
メソッドがこのくらいの長さであれば、「flag」が何を意味するのかが感覚的に分かりますが、メソッドがもっと長かったり複雑だったりすると何を意味するのかが分かりません。
 
この場合は「flag」という変数名を「foundTargetValue」（対象の値を見つけた）などに変えてあげると、後からコードを読む人が変数の意味を誤解することがなくなります。
 
    private bool ContainsTargetValue(int[] values, int targetValue)
    {
        bool foundTargetValue = false;

        for (int i = 0; i < values.Length; i++)
        {
            if (values[i] == targetValue)
            {
                foundTargetValue = true;
            }
        }

        return foundTargetValue;
    }
 
このコードはまだリファクタリングできますが、それは次回に説明いたします。
 

シンボリック定数に列挙型を利用する

  • 2014/4/26 23:20
前回は「シンボリック定数によるマジックナンバーの置き換え」を投稿しましたが、.NETではシンボリック定数を列挙型で表現できます。
 
以下のように処理ステータスを表わす列挙型を宣言します。
 
    // 処理ステータス
    public enum ProcessingStatus : int
    {
        // 停止
        Stop = 0,
 
        // 実行中
        WhileRunning
    }
 
問題のあったif文を列挙型を用いて書き直すと、
 
●
リファクタリング前
    if (status == 0)
    {
        // 停止の場合の処理
    }
    else if (status == 1)
    {
        // 実行中の場合の処理
    }
 
●
リファクタリング後
    if (status == ProcessingStatus.Stop)
    {
        // 停止の場合の処理
    }
    else if (status == ProcessingStatus.WhileRunning)
    {
        // 実行中の場合の処理
    }
 
となります。
 
シンボリック定数が長くなり少し冗長に感じますが、この列挙型は処理ステータスを表わす全ての処理で利用できますので、列挙型を用いたほうがプログラムの保守性が向上します。
 

シンボリック定数によるマジックナンバーの置き換え

  • 2014/4/20 21:13
プログラムを開発する上で数値に意味を持たせる手法はよく使われますが、具体的な数値（マジックナンバー）をそのままソースコードに埋め込んではいけません。
 
たとえば、とある処理のステータスが「0：停止」、「1：実行中」の場合、
 
    if (status == 0)
    {
        // 停止の場合の処理
    }
    else if (status == 1)
    {
        // 実行中の場合の処理
    }
 
といったコードを書いてしまうと、「0」が何を意味するのか？「1」が何を意味するのかをコメントで補足しないと分からなくなります。
 
このような場合は「シンボリック定数によるマジックナンバーの置き換え」を実行しましょう。
 
フィールドにシンボリック定数を用意します。
 
    // 停止
    public const int STOP = 0;
 
    // 実行中
    public const int WHILE_RUNNING = 1;
 
先ほどのマジックナンバーをシンボリック定数で置き換えます。
 
    if (status == STOP)
    {
        // 停止の場合の処理
    }
    else if (status == WHILE_RUNNING)
    {
        // 実行中の場合の処理
    }
 
いかがでしょうか、if文が直観的になったと思いませんか？
 
さらに同じマジックナンバーを使うif文が点在している場合は、そのすべてをシンボリック定数で置き換えます。
 
こうしておけば仕様変更が入り「実行中は2になりました」といった場合でも、シンボリック定数の値を変更するだけで対応できます。
 
最後にシンボリック定数は「分かりやすい名前」を付けましょう。分かりにくい名前を付けるとかえって混乱します。
 

不吉な匂い

  • 2014/4/16 00:46
マーチン ファウラー（著）リファクタリングでは、リファクタリングが必要な個所を「不吉な匂い」と表現しています。
 
これは感覚的でよい表現だと思います。たとえばソースを解析していて「よくわからない」、「直しづらい」と感じる個所は「不吉な匂い」の可能性が高く、リファクタリングの候補と言えるでしょう。
 
以下に書籍「リファクタリング」に記載されている22個の「不吉な匂い」を紹介します。
 
●
重複したコード
 
コードがあちこちでダブっている。
 
●
長すぎるメソッド
 
メソッドが長すぎる。
 
●
巨大なクラス
 
クラスが持っているフィールドやメソッドが多すぎる。
 
●
多すぎる引数
 
メソッドへ渡す引数が多すぎる。
 
●
変更の発散
 
仕様変更がおきたときの修正箇所があちこちにちらばっている。
 
●
変更の分散
 
あるクラスを修正すると、他のクラスもあわせて修正しなければならない。
 
●
属性、操作の横恋慕
 
いつも他のクラスの中身をいじっているクラスがある。
 
●
データの群れ
 
まとめて扱うべき複数のデータが、1つのクラスにまとまっていない。
 
●
基本データ型への執着
 
クラスを作らず、intのような基本データ型ばかり使っている。
 
●
スイッチ文
 
switch文やif文を使って振る舞いを分けている。
 
●
パラレル継承
 
サブクラスを作ると、クラス階層の別のところにもサブクラスを作らなければならない。
 
●
怠け者クラス
 
クラスが大した仕事をしていない。
 
●
疑わしき一般化
 
いつかこういう拡張もするだろうと期待して、一般化しすぎる。
 
●
一時的属性
 
一時的にしか使わないフィールドがある。
 
●
メッセージの連鎖
 
メソッド呼び出しの連鎖が多すぎる。
 
●
仲介人
 
委譲ばかりしていて、自分では仕事をしていないクラスがある。
 
●
不適切な関係
 
その必要がないのに双方向リンクを張っていたり、IS-A関係がないのに継承を使っていたりする。
 
●
クラスのインタフェース不一致
 
クラスのインターフェース（API）が不適切である。
 
●
未熟なクラスライブラリ
 
既存のクラスライブラリが使いにくい。
 
●
データクラス
 
フィールドとプロパティ（getterメソッドとsetterメソッド）しか持っていないクラスがある。
 
●
相続拒否
 
継承しているメソッドなのに、それを呼ぶと問題がおきる。
 
●
コメント
 
コードの不備を補うために、詳しいコメントがある。
g h T
 3,048 Total Views

PAGE NAVI