読者です 読者をやめる 読者になる 読者になる

今日やったリファクタリング - 1 : 連番付きフィールド

C# リファクタリング

あまりの糞コードに遭遇し、怒りのリファクタリングを行った。

ついでに「こんなリファクタリングしたよ」と残しておくとなんか良さそうだなと思い付き、
久しぶりにブログを書き始めた。

今日したのは、言語はC#で、
フィールドに連番の付いたフィールドがあり、そのどれかに値が入っていることを調べる処理のリファクタリング

モデルクラスは以下、

public class Model {
    // モデルのフィールドの構造は諸事情により、変更不可とする。
    // メソッドなどは追加してOK。
    public string Value1 { get; set; }
    public string Value2 { get; set; }
    public string Value3 { get; set; }
    public string Value4 { get; set; }
    public string Value5 { get; set; }
    public string Value6 { get; set; }
    public string Value7 { get; set; }
    public string Value8 { get; set; }
    public string Value9 { get; set; }
    public string Value10 { get; set; }
}

このモデルに対し、別の処理でこのどれかに値が入ってくる。 どのValueNに値が入っているか、Nの数値を取得したい。

Before

public class BusinessLogic {
    
    public int GetValueIndex(Model model) {
        if(! string.IsNullOrEmpty(model.Value10)) {
            return 10;
        }
        else if(! string.IsNullOrEmpty(model.Value9)) {
            return 9;
        }
        else if(! string.IsNullOrEmpty(model.Value8)) {
            return 8;
        }
        else if(! string.IsNullOrEmpty(model.Value7)) {
            return 7;
        }
        else if(! string.IsNullOrEmpty(model.Value6)) {
            return 6;
        }
        else if(! string.IsNullOrEmpty(model.Value5)) {
            return 5;
        }
        else if(! string.IsNullOrEmpty(model.Value4)) {
            return 4;
        }
        else if(! string.IsNullOrEmpty(model.Value3)) {
            return 3;
        }
        else if(! string.IsNullOrEmpty(model.Value2)) {
            return 2;
        }
        else if(! string.IsNullOrEmpty(model.Value1)) {
            return 1;
        }
        else
        {
            return 0;
        }
    }
}

あぁ!もう!!!

このIF文乱舞で嫌気が差さない場合は自分を疑ったほうが良い。

私が修正した結果がこちら

After

まず、ModelにValue達をDictionaryで返すメソッドを追加した。

public class Model {
    
    // モデルの構造は諸事情により、変更不可とする。
    public string Value1 { get; set; }
    public string Value2 { get; set; }
    public string Value3 { get; set; }
    public string Value4 { get; set; }
    public string Value5 { get; set; }
    public string Value6 { get; set; }
    public string Value7 { get; set; }
    public string Value8 { get; set; }
    public string Value9 { get; set; }
    public string Value10 { get; set; }

    public Dictionary<int, string> GetValues() {
        return new Dictionary<int, string>() {
            {1, Value1},
            {2, Value2},
            {3, Value3},
            {4, Value4},
            {5, Value5},
            {6, Value6},
            {7, Value7},
            {8, Value8},
            {9, Value9},
            {10, Value10},
        }
    }
}

このメソッドを利用して、BudinessLogicもこう修正した。

public class BusinessLogic {
    
    public int GetValueIndex(Model model) {
        return model.GetValues().Where(v => !string.IsNullOrEmpty(v)).Select(v => v.Key).Max();
    }
}

だいぶマシになった。

ついでにこの後、メソッド名もGetValueIndexでは意味が違っているので、GetMaxValueIndexに変更した。

リファクタリングはテストコードを書いて、現状のメソッド仕様を把握・テスト出来る状態にしてから取り掛かったほうが良いね。

以上。