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

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 2

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