# Switch case statement refactoring

I have the following code with switch case statement:

public string IrregularCouponLabel
{
get
{
switch ((ctrl.IrregularCouponFirst ? 10 : 0) + (ctrl.IrregularCouponLast ? 1 : 0))
{
case 11: return LocalString.Evaluate("label.Both");
case 10: return LocalString.Evaluate("label.First");
case 1: return LocalString.Evaluate("label.Last");
default: return LocalString.Evaluate("label.None");
}
}
}


How should I refactor this code to be more clear and readable or how could I otherwise improve this code snippet?

UPDATE According to valuable answers I have updated the code in the following way,

    public string IrregularCouponLabel
{
get
{
return LocalString.Evaluate("label." + GetIrregularCouponValue());
}
}

private string GetIrregularCouponValue()
{
bool first = ctrl.IrregularCouponFirst;
bool last = ctrl.IrregularCouponLast;
bool both = first && last;
return both  ? "Both" :
first ? "First" :
last  ? "Last" :
"None";

}


You can use an if statement and the conditional operator:

string label;
if (ctrl.IrregularCouponFirst) {
label = ctrl.IrregularCouponLast ? "label.Both" : "label.First";
} else {
label = ctrl.IrregularCouponLast ? "label.Last" : "label.None";
}
return LocalString.Evaluate(label);


You can also use only conditional operators. This way of chaining conditional operators checks take a bit of work to grasp the first time, but it's very compact:

return LocalString.Evaluate(
ctrl.IrregularCouponFirst && ctrl.IrregularCouponLast ? "label.Both" :
ctrl.IrregularCouponFirst ? "label.First" :
ctrl.IrregularCouponLast ? "label.Last" :
"label.None"
);

• +1 for the second version as it completely describes intent and keeps a good bunch of mechanism on the sidelines. – Jesse C. Slicer Jan 20 '12 at 14:50
• Yes I agree the second one is short and clear thanks – Serghei Jan 23 '12 at 13:34

I'd write something like this:

string labelName;
if (ctrl.IrregularCouponFirst && ctrl.IrregularCouponLast) {
labelName = "label.Both"
} else if (ctrl.IrregularCouponFirst) {
labelName = "label.First";
} else if (ctrl.IrregularCouponLast) {
labelName = "label.Last";
} else {
labelName = "label.None";
}
return LocalString.Evaluate(labelName);

• This is the easiest answer to follow IMO. – Andy Jan 22 '12 at 13:31
• What does it mean IMO? – Serghei Jan 23 '12 at 13:34
• "in my opinion" – palacsint Jan 23 '12 at 13:47

I'd use enums.

My suggestion presumes you are able to refactor your existing code a bit. If your control can be modified to keep the position value in one property of the following enum type, this will work.

[Flags]
public enum LabelEnum
{
None = 0,
First = 1,
Last = 2,
Both = 3
}

//....
ctrl.IrregularCoupon = LabelEnum.Both;
// or
ctrl.IrregularCoupon = LabelEnum.First | LabelEnum.Last

labelName = "label." + ctrl.IrregularCoupon.ToString();


This might be off topic, but if you use switch statements for more complex functionality, you should read this before going further. http://sourcemaking.com/refactoring/replace-conditional-with-polymorphism

• Wish I could upvote more than once. Enums are far more readable than magic numbers and if the code can be refactored to just use a single enum value then you end up with the clear ToString operation on the value itself, no conditional parsing needed. My first thought when reading the code was Enum. – pstrjds Jan 26 '12 at 17:42

I think I would store the IrregularCouponFirst and IrregularCouponLast as flag-enums to improve readability.

So first define the enum:

[Flags]
enum Position
{
None  = 0,
First = 1,
Last  = 2,
Both  = 3
}


Then to keep compatibility with previous code, I'd consider implementing properties for IrregularCouponFirst and IrregularCouponLast like this:

public Position IrregularCouponPosition;

public bool IrregularCouponFirst
{
get { return _irregularCouponPosition & Position.First != 0; }
set { _irregularCouponPosition |= Postion.First; }
}

public bool IrregularCouponLast
{
get { return _irregularCouponPosition & Position.Last != 0; }
set { _irregularCouponPosition |= Postion.Last; }
}


And finally your switch would look like this:

public string IrregularCouponLabel
{
get
{
switch (ctrl.IrregularCouponPosition)
{
case Position.First: return LocalString.Evaluate("label.First");
case Position.Last: return LocalString.Evaluate("label.Last");
case Position.Both: return LocalString.Evaluate("label.Both");
default: return LocalString.Evaluate("label.None");
}
}
}


Which is pretty much the same switch you had to begin with. Though I find it a bit more readable now.

• If you were going to do it this way, I think you'd want to use your enum constants in the switch statement - not the "magic" constants 1, 2, and 3. – Kai Jan 22 '12 at 23:31

I would likely use:

string type="None";
if (false) {
}else if (ctrl.IrregularCouponFirst && ctrl.IrregularCouponLast){type="Both";
}else if (ctrl.IrregularCouponFirst                            ){type="First";
}else if (ctrl.IrregularCouponLast                             ){type="Last";
}
return LocalString.Evaluate("label."+type);


But then there is always:

var types = new Dictionary<int, string> {
{ 0, "None" }
,{ 1, "Last" }
,{ 10, "First" }
,{ 11, "Both" }
};

int key=(ctrl.IrregularCouponFirst ? 10 : 0) + (ctrl.IrregularCouponLast ? 1 : 0);
return LocalString.Evaluate("label."+types[key]);

• -1: convoluted. – ANeves Jan 27 '12 at 17:20
• Oh yes, convoluted on this scale. But truth tables work wonders for larger applications. – Mark Robbins Jan 29 '12 at 16:58