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

 

制御フラグの削除

前回の「変数名の変更」で説明したコードは、まだリファクタリングの余地があります。
 
今回は「制御フラグの削除」のリファクタリングをしてみましょう。
 
前回のコード
    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文で復帰してあるとそれ以上「無駄なコードを読まなくてよい」というメリットがあるのです。
 
私の経験上そのメリットの恩恵は計り知れません。トラブル対応で一分一秒が惜しいときに、整理されたメソッドにどれだけ助けられたことか、逆に意味不明な制御フラグにどれだけ苦しめられたことか。。
 
全ての制御フラグを削除しろとは言いませんが、明らかに無駄な制御フラグは削除するように心がけましょう。
 

変数名の変更

マーチン ファウラー(著)リファクタリングでは名言されていませんが、分かりづらい変数名を適切な名前に変更することは、とても有効なリファクタリングだと思います。
 
たとえば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;
    }
 
このコードはまだリファクタリングできますが、それは次回に説明いたします。
 

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

前回は「シンボリック定数によるマジックナンバーの置き換え」を投稿しましたが、.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)
    {
        // 実行中の場合の処理
    }
 
となります。
 
シンボリック定数が長くなり少し冗長に感じますが、この列挙型は処理ステータスを表わす全ての処理で利用できますので、列挙型を用いたほうがプログラムの保守性が向上します。
 

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

プログラムを開発する上で数値に意味を持たせる手法はよく使われますが、具体的な数値(マジックナンバー)をそのままソースコードに埋め込んではいけません。
 
たとえば、とある処理のステータスが「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になりました」といった場合でも、シンボリック定数の値を変更するだけで対応できます。
 
最後にシンボリック定数は「分かりやすい名前」を付けましょう。分かりにくい名前を付けるとかえって混乱します。
 

不吉な匂い

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

リファクタリングとは

リファクタリング(refactoring)とは、「外部から見たプログラムの振る舞いを変えずに、プログラム内部の構造を改善すること」です。
 
たとえば「顧客データを削除する」メソッドの名前が「CreateCustomerData」になっていたので、「DeleteCustomerData」に直しておいた。
 
これはもう立派なリファクタリングです。もともとの「CreateCustomerData」という名前を「DeleteCustomerData」に変えただけですので、プログラムの振る舞いは変わっていませんし、今後このメソッドを使う人が誤って顧客データを削除する心配もなくなりましたので、プログラム内部の構造を改善できました。
 
そうリファクタリングとは、プログラマーなら誰でもやっていることを体系的にまとめ、間違いなく実行できるようにしたガイドラインなのです。
 
ガイドラインなのでとくに拘束力はありませんし、敷居はそんなに高くない。いやむしろ低いと思いますので、新人プログラマーこそリファクタリングを学ぶべきだと思います。
g h T
 1,487 Total Views