When should one try to eliminate a switch statemen

Question

This question already has answers here:

Is Switch (Case) always wrong? (8 answers)

Closed 7 years ago.

I've come across a switch statement in the codebase I'm working on and I'm trying to figure out how to replace it with something better since switch statements are considered a code smell. However, having read through several posts on stackoverflow about replacing switch statements I can't seem to think of an effective way to replace this particular switch statement.

Its left me wondering if this particular switch statement is ok and if there are particular circumstances where switch statements are considered appropriate.

In my case the code (slightly obfuscated naturally) that I'm struggling with is like this:

private MyType DoSomething(IDataRecord reader)
{
    var p = new MyType
                {
                   Id = (int)reader[idIndex],
                   Name = (string)reader[nameIndex]
                }

    switch ((string) reader[discountTypeIndex])
    {
        case "A":
            p.DiscountType = DiscountType.Discountable;
            break;
        case "B":
            p.DiscountType = DiscountType.Loss;
            break;
        case "O":
            p.DiscountType = DiscountType.Other;
            break;
    }

    return p;
}

Can anyone suggest a way to eliminate this switch? Or is this an appropriate use of a switch? And if it is, are there other appropriate uses for switch statements? I'd really like to know where they are appropriate so I don't waste too much time trying to eliminate every switch statement I come across just because they are considered a smell in some circumstances.

Update: At the suggestion of Michael I did a bit of searching for duplication of this logic and discovered that someone had created logic in another class that effectively made the whole switch statement redundant. So in the context of this particular bit of code the switch statement was unnecessary. However, my question is more about the appropriateness of switch statements in code and whether we should always try to replace them whenever they are found so in this case I'm inclined to accept the answer that this switch statement is appropriate.

Answer 1

This is an appropriate use for a switch statment, as it makes the choices readable, and easy to add or subtract one.

See this link.

Answer 2

I think changing code for the sake of changing code is not best use of ones time. Changing code to make it [ more readable, faster, more efficient, etc, etc] makes sense. Don't change it merely because someone says you're doing something 'smelly'.

-Rick

Answer 3

I'm not absolutely opposed to switch statements, but in the case you present, I'd have at least eliminated the duplication of assigning the DiscountType; I might have instead written a function that returns a DiscountType given a string. That function could have simply had the return statements for each case, eliminating the need for a break. I find the need for breaks between switch cases very treacherous.

private MyType DoSomething(IDataRecord reader)
{
    var p = new MyType
                {
                   Id = (int)reader[idIndex],
                   Name = (string)reader[nameIndex]
                }

    p.DiscountType = FindDiscountType(reader[discountTypeIndex]);

    return p;
}

private DiscountType FindDiscountType (string key) {
    switch ((string) reader[discountTypeIndex])
    {
        case "A":
            return DiscountType.Discountable;
        case "B":
            return DiscountType.Loss;
        case "O":
            return DiscountType.Other;
    }
    // handle the default case as appropriate
}

Pretty soon, I'd have noticed that FindDiscountType() really belongs to the DiscountType class and moved the function.

Answer 4

This switch statement is fine. Do you guys not have any other bugs to attend to? lol

However, there is one thing I noticed... You shouldn't be using index ordinals on the IReader[] object indexer.... what if the column orders change? Try using field names i.e. reader["id"] and reader["name"]

Answer 5

You are right to suspect this switch statement: any switch statement that is contingent on the type of something may be indicative of missing polymorphism (or missing subclasses).

TallJoe's dictionary is a good approach, however.

Note that if your enum and database values were integers instead of strings, or if your database values were the same as the enum names, then reflection would work, e.g. given

public enum DiscountType : int
{
    Unknown = 0,
    Discountable = 1,
    Loss = 2,
    Other = 3
}

then

p.DiscountType = Enum.Parse(typeof(DiscountType), 
    (string)reader[discountTypeIndex]));

would suffice.

Answer 6

Switch statements (especially long ones) are considered bad, not because they are switch statements, but because their presence suggests a need to refactor.

The problem with switch statements is they create a bifurcation in your code (just like an if statement does). Each branch must be tested individually, and each branch within each branch and... well, you get the idea.

That said, the following article has some good practices on using switch statements:

http://elegantcode.com/2009/01/10/refactoring-a-switch-statement/

In the case of your code, the article in the above link suggests that, if you're performing this type of conversion from one enumeration to another, you should put your switch in its own method, and use return statements instead of the break statements. I've done this before, and the code looks much cleaner:

private DiscountType GetDiscountType(string discount)
{
    switch (discount)
    {
        case "A": return DiscountType.Discountable;
        case "B": return DiscountType.Loss;
        case "O": return DiscountType.Other;
    }
}