Switch case statement refactoring

Question

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

What do you think about this? Is it readable or not?

Answer 1

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

• Jesse C. Slicer: +1 for the second version as it completely describes intent and keeps a good bunch of mechanism on the sidelines.
• Sergey K: Yes I agree the second one is short and clear thanks

Answer 2

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

• Andy: This is the easiest answer to follow IMO.
• Sergey K: What does it mean IMO?
• palacsint: "in my opinion"

Answer 3

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

• pstrjds: 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.

Answer 4

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.

• Kai: 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.

Answer 5

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

• Mark Robbins: Oh yes, convoluted on this scale. But truth tables work wonders for larger applications.