How can I reorganize the mother of all C # Switch operators

Question

I am trying to reorganize the mother of all switches and am not quite sure how to do this. Here is the existing code:

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; } 

Is there any need to do this in general or is this super switch really needed?

BIT MORE CONTEXT I do not claim that I understand all the magic that another developer approaches, but basically the line "Loan.FieldName" comes from some metadata tagged with an HTML input tag. This is used in this switch to bind a specific field to a data table / entity data structure. Although this comes from a strongly typed view, for reasons beyond human control, this juxtaposition has become the glue that contains it all together.

Answer 1

I know that you just need to answer your own question, but here is how my boss solved this problem using reflection and a dictionary. Ironically, he finished his decision a few minutes after we finished Mother of All Switches. Nobody wants to see how the text is printed meaningless in the afternoon, but this solution is much smoother.

  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; } }, }; 

Answer 2

Usually, when I refactor, it somehow reduces the complexity of the code or makes it easier to understand. In the code you posted, I have to say that it doesn't seem so complicated (although there can be many lines, it looks pretty repeating and straightforward). So, besides the aesthetics of the code, I'm not sure how much you are going to get by reorganizing the switch.

Having said that, it might be tempting to create a dictionary in which they have a field value, and that value is a delegate that contains code for each case (each method will probably return a bool with a FieldWasSaved value, and for some 4 other values ​​will have some external parameters). Then your method will simply use the field to search for the delegate from the dictionary and then call it.

Of course, I would just leave the code as it is. The dictionary approach may not be as obvious to other developers, and it probably makes the code less comprehensible.

Update . I also agree with nightwatch that the best refactoring will probably include code that is not shown - maybe this code belongs to other classes (maybe there will be a Loan class that encapsulates all loan fields or something like that ...) .

Answer 3

If the names in the case argument match the properties in the class, I would change everything to use reflection.

For example, here is a shortened version of the core of our core business record that we use to move data to and from databases, forms, web services, etc.

  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 } } 

In the above example, Global.ValueFromDB is a large switch statement that safely converts a value to the specified type. Here is a partial version of this:

  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); 

In cases where a specific FromDB data type looks something like this:

  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; } } 

In the short term, it may not seem like you save a lot of code, but you will find many, many uses for this as soon as it is implemented (we certainly have).

Answer 4

One possibility is to create a Dictionary with the field name as the key and the delegate as the value. Sort of:

 delegate bool FieldSaveDelegate(Claim claim, string value); 

Then you can write separate methods for each field:

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

And to initialize:

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

Dispatcher, then:

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

This is likely to be more maintainable than the switch statement, but it is still not particularly beautiful. It is nice that the code for filling out the dictionary can be automatically generated.

As mentioned in the previous answer, you can use reflection to search for the field name to make sure it is valid and check the type (also with reflection). This would reduce the number of handler methods you write, just a few: one for each individual type.

Even if you did not use reflection, you could reduce the number of methods needed to store the type, not the delegate, in the dictionary. Then you will have a search to get the type for the field and a small switch statement for that type.

This assumes, of course, that you are not doing any special processing in the field. If you need to perform a special check or additional processing in some fields, you will need either one method for each field, or you will need additional information for each field.

Answer 5

Depending on your application, you can override the claim as a dynamic object:

 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; } } 

If possible, you can use the following syntax to indirectly work with the relevant loan objects through the Claim object:

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

In the event of unsuccessful cases where the input string cannot be parsed, you will unfortunately get a RuntimeBinderException and not a good return value for the function, so you need to think about whether this will work well for you.