How can I refactor the mother of all C# Switch Statements

Question

I am trying to refactor the mother of all switches and am not really sure how to do it. 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 anyway to do this in a generic way or is this super switch actually necessary?

A BIT MORE CONTEXT I don't claim to understand all the magic the other dev is up to, but basically the string "Loan.FieldName" is coming from some metadata tagged on to an HTML input tag. This is used in this switch to link a particular field to an entity framework data table / property combo. Although this is coming from a strongly typed view, for reasons beyond the ken of man this mapping has become the glue holding the whole thing together.

Answer 1

I know it is gosh to answer your own question, but here is how my boss solved this problem using reflection and a dictionary. I ironically, he finished his solution just minutes after we finished the "Mother of All Switches". No one wants to see an afternoon of typing rendered pointless, but this solution is a lot more slick.

    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

Depending on your application, you might be able to redefine Claim to be 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 this is possible, you should be able to use the following syntax to work indirectly with the corresponding Loan objects through a Claim object:

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

For the failed cases, when the input string can't be parsed, you will unfortunately get a RuntimeBinderException rather than a nice function return value, so you need to consider if this will be a good fit for you case.

Answer 3

Usually when I refactor, it's to reduce the complexity of the code somehow, or make it easier to understand. In the code you posted, I have to say it doesn't seem all that complex (although there might be a lot of lines, it looks pretty repetitive and straight-forward). So other than the code aesthetics, I'm not sure how much you are going to gain by refactoring a switch.

Having said that, I might be tempted to create a dictionary where they key is field and the value is a delegate which contains the code for each case (each method would probably return a bool with the FieldWasSaved value, and would have some out-params for those 4 other values). Then your method would just use field to look up the delegate from the dictionary and then call it.

Of course, I would probably just leave the code as-is. The dictionary approach might not be as obvious to other devs, and probably makes the code less obvious to understand.

Update: I also agree with nightwatch that the best refactor will probably involve code which is not shown -- perhaps a lot of this code belongs in other classes (maybe there would be a Loan class which encapsulates all the Loan fields, or something like that...).

Answer 4

One possibility is to create a Dictionary that has the field name as the key, and a delegate as the value. Something like:

delegate bool FieldSaveDelegate(Claim claim, string value);

You can then write separate methods for each field:

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

And to initialize it:

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

The dispatcher, then:

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

This would probably be more maintainable than the switch statement, but it's still not especially pretty. The nice thing is that the code to populate the dictionary could be automatically generated.

As a previous answer said, you can use reflection to look up the field name to ensure that it's valid, and check the type (also with reflection). That would reduce the number of handler methods you write to just a handful: one for each individual type.

Even if you didn't use reflection, you could reduce the number of methods required by saving the type, rather than the delegate, in the dictionary. Then you'd have a lookup to get the type for a field, and a small switch statement on the type.

That assumes, of course, that you don't do any special per-field processing. If you have to do special validation or additional processing on some fields, then you'll either need the one method per field, or you'll need some additional per-field information.

Answer 5

If the names in the case statement match with properties in the class, I would change it all to use reflection.

For example, here is a trimmed down version of the core of our base business record, which we use to move data in and out of 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, Global.ValueFromDB is a big switch statement that safely converts the value to the specified type. Here is a partial version of that:

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

Where the datatype specific FromDBs look something like:

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

It may not seem like your saving much code in the short term, but you will find many, many uses for this once it is implemented (we certainly have).