2011-12-10 13 views
4

私はすべてのスイッチの母親をリファクタリングしようとしています。ここに既存のコードがあります:すべてのC#Switchステートメントの母親をどのようにリファクタリングすることができますか

bool FieldSave(Claim claim, string field, string value) 
    { 
     //out vars for tryparses 
     decimal outDec; 
     int outInt; 
     bool outBool; 
     DateTime outDT; 

     //our return value 
     bool FieldWasSaved = true; 

     //World Greatest Switch - God help us all. 
     switch (field) 
     { 
      case "Loan.FhaCaseNumber": 
       GetLoan(claim).FhaCaseNumber = value; 
       break; 
      case "Loan.FhaInsurance": 
       if (bool.TryParse(value, out outBool)) 
       { 
        GetLoan(claim).FhaInsurance = outBool; 
        FieldWasSaved = true; 
       } 
       break; 
      case "Loan.UnpaidPrincipalBalance": 
       if (decimal.TryParse(value, out outDec)) 
       { 
        GetLoan(claim).UnpaidPrincipalBalance = outDec; 
        FieldWasSaved = true; 
       } 
       break; 
      case "Loan.Mortgagor_MortgagorID": 
       if(Int32.TryParse(value, out outInt)){ 
        GetLoan(claim).Mortgagor_MortgagorID = outInt; 
        FieldWasSaved = true;       
       } 
       break; 
      case "Loan.SystemDefaultDate": 
       if (DateTime.TryParse(value, out outDT)) 
       { 
        GetLoan(claim).SystemDefaultDate = outDT; 
        FieldWasSaved = true; 
       }      
       break; 
      //And so on for 5 billion more cases 
     } 

     db.SaveChanges(); 
     return FieldWasSaved; 
    } 

実際にこのスーパースイッチが必要ですか?

もう少しCONTEXT 私は他のDEVが最大であるすべての魔法を理解することは主張しませんが、基本的には文字列「Loan.FieldNameは、」HTMLの入力タグにタグ付けされた上で、いくつかのメタデータから来ています。これは、特定のフィールドをエンティティフレームワークのデータテーブル/プロパティコンボにリンクするためにこのスイッチで使用されます。これは強く型付けされたビューから来ていますが、人間のケンを超える理由から、このマッピングはすべてのものを一緒に保持する接着剤になっています。

+1

...しかし、あなたは正しい、それは私がそれに何かを参照してください「いけない、特に巧妙な – Murph

+1

を見ていません。 n個のケースがあり、n個の異なるハンドリングが必要な場合、これは妥当と思われます。理解しにくい、重複したコードがあるなどの理由ではありません。 –

+2

@Murphあなたは "//そして50億件以上の事件のためにそうしていますか?" ;) – DaveShaw

答えて

2

あなた自身の質問に答えるのはうれしいですが、私の上司は反射と辞書を使ってこの問題を解決しました。私は皮肉なことに、「すべてのスイッチの母」を終えた直後に彼のソリューションを完成させました。レンダリングした無意味な点を入力する午後は誰も見たくないのですが、この解決策ははるかに滑らかです。すべてのスイッチの母親であることにも、近くではありません

public JsonResult SaveField(int claimId, string field, string value) 
    { 
     try 
     { 
      var claim = db.Claims.Where(c => c.ClaimID == claimId).SingleOrDefault(); 
      if (claim != null) 
      { 
       if(FieldSave(claim, field, value)) 
        return Json(new DataProcessingResult { Success = true, Message = "" }); 
       else 
        return Json(new DataProcessingResult { Success = false, Message = "Save Failed - Could not parse " + field }); 
      } 
      else 
       return Json(new DataProcessingResult { Success = false, Message = "Claim not found" }); 
     } 
     catch (Exception e) 
     { 
      //TODO Make this better 
      return Json(new DataProcessingResult { Success = false, Message = "Save Failed" }); 
     } 
    } 

    bool FieldSave(Claim claim, string field, string value) 
    { 

     //our return value 
     bool FieldWasSaved = true; 

     string[] path = field.Split('.'); 

     var subObject = GetMethods[path[0]](this, claim); 

     var secondParams = path[1]; 
     PropertyInfo propertyInfo = subObject.GetType().GetProperty(secondParams); 

     if (propertyInfo.PropertyType.IsGenericType && propertyInfo.PropertyType.GetGenericTypeDefinition() == typeof(Nullable<>)) 
     { 
      FieldWasSaved = SetValue[Nullable.GetUnderlyingType(propertyInfo.PropertyType)](propertyInfo, subObject, value); 
     } 
     else 
     { 
      FieldWasSaved = SetValue[propertyInfo.PropertyType](propertyInfo, subObject, value); 
     } 


     db.SaveChanges(); 
     return FieldWasSaved; 
    } 

    // these are used for dynamically setting the value of the field passed in to save field 
    // Add the object look up function here. 
    static Dictionary<string, Func<dynamic, dynamic, dynamic>> GetMethods = new Dictionary<string, Func<dynamic, dynamic, dynamic>>() 
    { 
     { "Loan", new Func<dynamic, dynamic, dynamic>((x, z)=> x.GetLoan(z)) }, 
     // and so on for the 15 or 20 model classes we have 
    }; 

    // This converts the string value comming to the correct data type and 
    // saves the value in the object 
    public delegate bool ConvertString(PropertyInfo prop, dynamic dynObj, string val); 
    static Dictionary<Type, ConvertString> SetValue = new Dictionary<Type, ConvertString>() 
    { 
     { typeof(String), delegate(PropertyInfo prop, dynamic dynObj, string val) 
      { 
       if(prop.PropertyType == typeof(string)) 
       { 
        prop.SetValue(dynObj, val, null); 
        return true; 
       } 
       return false; 
      } 
     }, 
     { typeof(Boolean), delegate(PropertyInfo prop, dynamic dynObj, string val) 
      { 
       bool outBool = false; 
       if (Boolean.TryParse(val, out outBool)) 
       { 
        prop.SetValue(dynObj, outBool, null); 
        return outBool; 
       } 
       return false; 
      } 
     }, 
     { typeof(decimal), delegate(PropertyInfo prop, dynamic dynObj, string val) 
      { 
       decimal outVal; 
       if (decimal.TryParse(val, out outVal)) 
       { 
        prop.SetValue(dynObj, outVal, null); 
        return true; 
       } 
       return false; 
      } 
     }, 
     { typeof(DateTime), delegate(PropertyInfo prop, dynamic dynObj, string val) 
      { 
       DateTime outVal; 
       if (DateTime.TryParse(val, out outVal)) 
       { 
        prop.SetValue(dynObj, outVal, null); 
        return true; 
       } 
       return false; 
      } 
     }, 
    }; 
+0

このコードは狂っているようです:)私はあなたの上司に数週間でそれを見るように頼むことをお勧めします。 –

2

caseステートメントの名前がクラスのプロパティと一致する場合は、すべてをリフレクションを使用するように変更します。

例えば、ここになど私たちは、データベースの中と外のデータを移動するために使用し、当社の基本事業レコード、フォーム、Webサービスでは、

public static void SetFieldValue(object oRecord, string sName, object oValue) 
    { 
     PropertyInfo theProperty = null; 
     FieldInfo theField = null; 
     System.Type oType = null; 

     try 
     { 
      oType = oRecord.GetType(); 

      // See if the column is a property in the record 
      theProperty = oType.GetProperty(sName, BindingFlags.Instance | BindingFlags.IgnoreCase | BindingFlags.Public, null, null, new Type[0], null); 
      if (theProperty == null) 
      { 
       theField = oType.GetField(sName, BindingFlags.Instance | BindingFlags.IgnoreCase | BindingFlags.Public); 
       if (theField != null) 
       { 
        theField.SetValue(oRecord, Global.ValueFromDB(oValue, theField.FieldType.Name)); 
       } 
      } 
      else 
      { 
       if (theProperty.CanWrite) 
       { 
        theProperty.SetValue(oRecord, Global.ValueFromDB(oValue, theProperty.PropertyType.Name), null); 
       } 
      } 
     } 
     catch (Exception theException) 
     { 
      // Do something useful here 
     } 
    } 

のコアのトリムダウンバージョンでありますGlobal.ValueFromDBは、値を指定された型に安全に変換する大きなswitch文です。

public static object ValueFromDB(object oValue, string sTypeName) 
    { 
     switch (sTypeName.ToLower()) 
     { 
      case "string": 
      case "system.string": 
       return StrFromDB(oValue); 

      case "boolean": 
      case "system.boolean": 
       return BoolFromDB(oValue); 

      case "int16": 
      case "system.int16": 
       return IntFromDB(oValue); 

      case "int32": 
      case "system.int32": 
       return IntFromDB(oValue); 

データ型特定FromDBsのようなものに見える:ここではその一部のバージョンであるそれはあなたが短期的に多くのコードを保存するとは思えないかもしれません

public static string StrFromDB(object theValue) 
    { 
     return StrFromDB(theValue, ""); 
    } 
    public static string StrFromDB(object theValue, string sDefaultValue) 
    { 
     if ((theValue != DBNull.Value) && (theValue != null)) 
     { 
      return theValue.ToString(); 
     } 
     else 
     { 
      return sDefaultValue; 
     } 
    } 
    public static bool BoolFromDB(object theValue) 
    { 
     return BoolFromDB(theValue, false); 
    } 
    public static bool BoolFromDB(object theValue, bool fDefaultValue) 
    { 
     if (!(string.IsNullOrEmpty(StrFromDB(theValue)))) 
     { 
      return Convert.ToBoolean(theValue); 
     } 
     else 
     { 
      return fDefaultValue; 
     } 
    } 
    public static int IntFromDB(object theValue) 
    { 
     return IntFromDB(theValue, 0); 
    } 
    public static int IntFromDB(object theValue, int wDefaultValue) 
    { 
     if ((theValue != DBNull.Value) && (theValue != null) && IsNumeric(theValue)) 
     { 
      return Convert.ToInt32(theValue); 
     } 
     else 
     { 
      return wDefaultValue; 
     } 
    } 

は、しかし、あなたは多くを見つけるでしょう、いったん実装されると、これは多くの用途に使用されます(確かにあります)。

4

通常、私がリファクタリングするときは、コードの複雑さを何らかの形で減らすか、理解しやすくすることです。あなたが投稿したコードでは、それは複雑ではないようです(多くの行があるかもしれませんが、それはかなり繰り返しやすく、まっすぐに見えます)。コードの美しさ以外にも、スイッチをリファクタリングすることでどのくらいの利益を得ようとしているのか分かりません。

私は彼らキーがfieldであり、値が各ケースのコードが含まれているデリゲートである(各メソッドは、おそらくFieldWasSaved値とブール値を返します辞書を作成するために誘惑されるかもしれない、と言った、とましたこれらの4つの他の値に対していくつかのアウトパラメータを持つだろう)。あなたのメソッドはフィールドを使って辞書からデリゲートを検索し、それを呼び出します。

もちろん、私はコードをそのまま残しておきます。辞書のアプローチは、他の開発者にはあまり明らかではないかもしれませんし、おそらくコードを分かりにくくするでしょう。

更新:私はまた、最高のリファクタリングはおそらく示されていないコードを含むことになるナイトウォッチに同意する - おそらく、このコードの多くは、他のクラスに属している(多分すべてのローンをカプセル化ローンのクラスが存在することになりますフィールド、またはそのようなもの...)。

+0

からのフィールドはどこですか?何度も呼び出されるとパフォーマンスに影響する可能性のある辞書ルックアップによってオーバーヘッドが追加されます。この場合には必要ではない。しかし、スイッチケースのサイズによっては結局それが良いかもしれないことに同意します。 –

+1

@ Slade:辞書ルックアップのためにかなりのオーバーヘッドはありません。私が思い出すように、コンパイラは大きなswitch文の辞書を作成します。代わりは 'if ... else if ... else if'文の束になります。辞書ははるかに速くなります。 –

+0

@ JimMischel +1 - コンパイラがswitch文をどのように処理したかはわかりませんでした。 –

1

フィールド名がキーで、デリゲートが値であるDictionaryを作成する可能性があります。あなたはその後、フィールドごとに別々の方法で書くことができ

delegate bool FieldSaveDelegate(Claim claim, string value); 

bool SystemDefaultDateHandler(Claim cliaim, string value) 
{ 
    // do stuff here 
} 

をし、それを初期化する:

FieldSaveDispatchTable = new Dictionary<string, FieldSaveDelegate>() 
{ 
    { "Loan.SystemDefaultDate", SystemDefaultDateHandler }, 
    // etc, for five billion more fields 
} 

ディスパッチャは、:何かのよう

FieldSaveDelegate dlgt; 
if (!FieldSaveDispatchTable.TryGetValue(fieldName, out dlgt)) 
{ 
    // ERROR: no entry for that field 
} 
dlgt(claim, value); 

これはおそらくmorでしょうeはswitchステートメントよりもメンテナンス可能ですが、それでも特にあまりよくありません。素敵なことは、辞書を埋めるためのコードが自動的に生成されるということです。

前回の回答では、リフレクションを使用して、フィールド名を調べて有効であることを確認し、タイプを確認しました(リフレクションも含む)。これは、ハンドラのメソッドの数を、それぞれの型ごとに1つずつ、ほんの一握りに減らします。

リフェラルを使用していない場合でも、代理人ではなくタイプを辞書に保存することで、必要なメソッドの数を減らすことができます。次に、フィールドの型を取得するためのルックアップと、型の小さなswitch文を取得します。

これはもちろん、フィールドごとの特別な処理を行わないことを前提としています。一部のフィールドで特別な検証や追加処理が必要な場合は、フィールドごとに1つのメソッドが必要になるか、フィールドごとに追加の情報が必要になります。

1

アプリケーションによって、あなたは動的オブジェクトであることを請求を再定義することができるかもしれない:

class Claim : DynamicObject 
{ 
    ... // Current definition 

    public override bool TrySetMember(SetMemberBinder binder, object value) 
    { 
     try 
     { 
      var property = typeof(Loan).GetProperty(binder.Name); 
      object param = null; 

      // Find some way to parse the string into a value of appropriate type: 
      // (This will of course need to be improved to handle more types) 
      if (property.PropertyType == typeof(Int32)) 
      { 
       param = Int32.Parse(value.ToString()); 
      } 

      // Set property in the corresponding Loan object 
      property.SetValue(GetLoan(this), param, null); 
      db.Save(); 
     } 
     catch 
     { 
      return base.TrySetMember(binder, value); 
     } 

     return true; 
    } 
} 

これが可能であるならば、あなたはと間接的に動作するように、次の構文を使用することができるはずです障害が発生した場合について

dynamic claim = new Claim(); 
// Set GetLoan(claim).Mortgagor_MortgagorID to 1723 and call db.Save(): 
claim.Mortgagor_MortgagorID = "1723"; 

、入力STRI:オブジェクトを通してローンオブジェクトを対応あなたが残念なことにRuntimeBinderExceptionを返すのは良い関数の戻り値ではないので、これがあなたの場合に適しているかどうかを検討する必要があります。

関連する問題