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;
}
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.
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.
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.
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
}
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.
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:
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.
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);
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.
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.
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With