Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Replace Switch/Case with Pattern

I have code very similar to this example three times in a code behind. Each time the switch is toggling off of an option that is sent to it. Each time the code inside the case is exactly the same except for a parameter based off of the case. Is using a switch/case and methods the best way to do this? Should I think of using sometype of design pattern to avoid the repeative switch/case structure?

string option = dropDownList.SelectedValue.ToString();
switch (option.ToUpper())
{
    case "ALPHA":
        // do repeative code method here; only change is a parameter
        break;
    case "BRAVO":
        // do repeative code method here; only change is a parameter
        break;
    case "CHARLIE":
        // do repeative code method here; only change is a parameter
        break;
    case "DELTA":
        // do repeative code method here; only change is a parameter
        break;
    default:
        break;
}
like image 583
Chris Avatar asked Dec 01 '09 22:12

Chris


People also ask

What can I use instead of switch case?

Luckily, JavaScript's object literals are a pretty good alternative for most switch statement use-cases I can think of. The idea is to define an object with a key for each case you would have in a switch statement. Then you can access its value directly using the expression you would pass to the switch statement.

Is switch an anti pattern?

Switch-statements are not an antipattern per se, but if you're coding object oriented you should consider if the use of a switch is better solved with polymorphism instead of using a switch statement.

Is switch case a code smell?

Sometimes, it is considered in the category of code smell. Switch case is not a bad syntax, but its usage in some cases categorizes it under code smell. It is considered a smell, if it is being used in OOPS. Thus, Switch case should be used very carefully.


6 Answers

You can construct a table to convert string to parameter value.

var lookup = new Dictionary<string, ParaType> ()  
{
    { "ALPHA", a  },
    { "BETA", b },
    ....
};

ParaType para;
if (lookup.TryGetValue(option, out para))   // TryGetValue, on popular request
{       
   // do repeative code method here; with para
}
like image 73
Henk Holterman Avatar answered Oct 05 '22 23:10

Henk Holterman


Hm, I think that very quick solution is using dictionary. Dictionary is very fast structure, when you are using a key -> value logic. So, in your case, you can use this:

var processingValues = new Dictionary<string, Func<string, string>>() {
     {"ALPHA", yourProcessor.Alpha},
     {"BRAVO", yourProcessor.Bravo},
      ...
}

after this, you can create your functions for ALPHA, BRAVA, ...

public class Processor
{
   public string Alpha(string data)
   {
        return "do something";
   }

   public string Bravo(string data)
   {
        return "do something";
   }
...
}

and finally return something function:

public string SomethingToReturn(string key, string value)
  {
       if (proccessValues.ContainsKey(name))
       {
           return proccessValues[name].Invoke(value);
       }
       return string.Empty;
   }

Hope it help.

like image 28
Petr Tomášek Avatar answered Oct 06 '22 00:10

Petr Tomášek


Compilers are very good at optimizing switch/case constructs; the CLR will likely turn it into a lookup table or something similarly fast, so hand-rolling your own version such as Henk Holterman suggests is not what I would recommend. The CLR can do a better job than you can at choosing the best algorithm.

If it's an issue of elegance or maintainability, and you have several switch/cases strewn about the same class performing similar functions, then one way to improve it is to encapsulate all of the functionality related to a single "case" into its own class instance, like so:

class MyOption
{
    public static readonly MyOption Alpha = new MyOption(1, 10, "Alpha Text");
    public static readonly MyOption Bravo = new MyOption(2, 100, "Bravo Text");
    public static readonly MyOption Charlie = new MyOption(3, 1000, "Charlie Text");
    // ... Other options ...
    public static readonly MyOption Default = new MyOption(0, 0, null);

    public MyOption(int id, int value, string text)
    {
        this.ID = id;
        this.Value = value;
        this.Text = text;
    }

    public int ID { get; private set; }
    public int Value { get; private set; }
    public string Text { get; private set; }
}

Then in your class/control/page:

static MyOption GetOption(string optionName)
{
    switch (optionName)
    {
        case "ALPHA":
            return MyOption.Alpha;
        case "BRAVO":
            return MyOption.Bravo;
        case "CHARLIE":
            return MyOption.Charlie;
        // ... Other options ...
        default:
            return MyOption.Default;
    }
}

private MyOption GetOptionFromDropDown()
{
    string optionName = GetOptionNameFromDropDown();
    return GetOption(optionName);
}

private string GetOptionNameFromDropDown()
{
    // ... Your code ...
}

After that you can start churning out events and other methods:

private void control1_SomeEvent(object sender, EventArgs e)
{
    MyOption option = GetOptionFromDropDown();
    DoSomething(option.ID);
}

private void control2_SomeEvent(object sender, EventArgs e)
{
    MyOption option = GetOptionFromDropDown();
    DoSomethingElse(option.Value);
}

Of course, this is only a useful pattern if you have several of these switch/cases that you want to refactor into one. If you've only got one switch/case, you're just going to end up with a lot more code this way, so leave it alone!

Other possibilities to improve maintainability include:

  • Changing the string to an Enum type (convert optionName using Enum.Parse);
  • Moving all the MyOption/GetOption stuff into its own class (if you have several classes/controls/pages that all need to operate on the same set of choices);
  • Add a method delegate to the MyOption class if you actually need to call a different method for each;
  • Have your DropDownList or other control store a direct reference to the MyOption instance, if possible.

That's about it. It's simple to write, it's easy to understand, it's easy to maintain, it will save you time if you have a lot of switch/case constructs, and it still allows the CLR to perform the best possible optimizations. The only cost is the small amount of memory required to hold those readonly fields.

like image 26
Aaronaught Avatar answered Oct 05 '22 22:10

Aaronaught


I think I would move the switch statement into a separate function and have it return the parameter value for each case:

    private static string GetParameterForAllCases(string option)
    {
        switch (option.ToUpper())
        {
            case "ALPHA":
                return "ALPHA Parameter";
            case "BRAVO":
                return "BRAVO Parameter";
            case "CHARLIE":
                return "CHARLIE Parameter";
            case "DELTA":
                return "DELTA Parameter";
            default:
                return "Default Parameter";
        }
    }

Then you can just call your work method once:

        string option = dropDownList.SelectedValue.ToString();

        WorkMethod(GetParameterForAllCases(option);

If you don't want to execute your work method for the default cause, or if you have more than one parameter value, then you could change the GetParameter method to use output parameters:

    private static bool GetParameter(string option, out string value)
    {
        switch (option.ToUpper())
        {
            case "ALPHA":
                value = "ALPHA Parameter";
                return true;
            case "BRAVO":
                value = "BRAVO Parameter";
                return true;
            case "CHARLIE":
                value = "CHARLIE Parameter";
                return true;
            case "DELTA":
                value = "DELTA Parameter";
                return true;
            default:
                value = null;
                return false;
        }
    }

And call it like this:

        string option = dropDownList.SelectedValue.ToString();

        string value;
        if (GetParameter(option, out value))
            WorkMethod(value);
like image 32
Dr. Wily's Apprentice Avatar answered Oct 05 '22 22:10

Dr. Wily's Apprentice


The key to this problem is making the objects stored in the dropDownList provide the parameter (either directly or by indexing into an array). Then the switch statement may be removed completely.

If the parameter is a property of the object that appears in the drop down list, then this will provide the value very efficiently.

If the values in the drop down list can provide a numeric index into an array of parameter values, then this will beat a series of string comparisons in terms of runtime efficiency.

Either of these options are cleaner, shorter, and easier to maintain than switching on a string.

like image 32
Jeffrey L Whitledge Avatar answered Oct 05 '22 23:10

Jeffrey L Whitledge


Perhaps a transform from option to the method parameter?

This would allow you to drop the switch statement and simply feed the method the transformed parameter.

like image 23
micahtan Avatar answered Oct 05 '22 22:10

micahtan