When should one try to eliminate a switch statemen

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

回答3:

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

回答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"]

回答5:

In my opinion, it's not switch statements that are the smell, it's what's inside them. This switch statement is ok, to me, until it starts adding a couple of more cases. Then it may be worth creating a lookup table:

private static Dictionary<string, DiscountType> DiscountTypeLookup = 
  new Dictionary<string, DiscountType>(StringComparer.Ordinal)
    {
      {"A", DiscountType.Discountable},
      {"B", DiscountType.Loss},
      {"O", DiscountType.Other},
    };

Depending on your point-of-view, this may be more or less readable.

Where things start getting smelly is if the contents of your case are more than a line or two.

回答6:

Robert Harvey and Talljoe have provided excellent answers - what you have here is a mapping from a character code to an enumerated value. This is best expressed as a mapping where the details of the mapping are provided in one place, either in a map (as Talljoe suggests) or in a function that uses a switch statement (as suggested by Robert Harvey).

Both of those techniques are probably fine in this case, but I'd like to draw your attention to a design principal that may be useful here or in other similar cases. The the Open/Closed principal:

http://en.wikipedia.org/wiki/Open/closed_principle
http://www.objectmentor.com/resources/articles/ocp.pdf (make sure you read this!)

If the mapping is likely to change over time, or possibly be extended runtime (eg, through a plugin system or by reading the parts of the mapping from a database), then a using the Registry Pattern will help you adhere to the open/closed principal, in effect allowing the mapping to be extended without affecting any code that uses the mapping (as they say - open for extension, closed for modification).

I think this is a nice article on the Registry Pattern - see how the registry holds a mapping from some key to some value? In that way it's similar to your mapping expressed as a switch statement. Of course, in your case you will not be registering objects that all implement a common interface, but you should get the gist:

http://sinnema313.wordpress.com/2009/03/01/the-registry-pattern/

So, to answer the original question - the case statement is poor form as I expect the mapping from the character code to an enumerated value will be needed in multiple places in your application, so it should be factored out. The two answers I referenced give you good advice on how to do that - take your pick as to which you prefer. If, however, the mapping is likely to change over time, consider the Registry Pattern as a way insulating your code from the effects of such change.

回答7:

I wouldn't use an if. An if would be less clear than the switch. The switch is telling me that you are comparing the same thing throughout.

Just to scare people, this is less clear than your code:

if (string) reader[discountTypeIndex]) == "A")
   p.DiscountType = DiscountType.Discountable;
else if (string) reader[discountTypeIndex]) == "B")
   p.DiscountType = DiscountType.Loss;
else if (string) reader[discountTypeIndex]) == "O")
   p.DiscountType = DiscountType.Other;

This switch may be OK, you might want to look at @Talljoe suggestion.

回答8:

Are switches on discount type located throughout your code? Would adding a new discount type require you to modify several such switches? If so you should look into factoring the switch out. If not, using a switch here should be safe.

If there is a lot of discount specific behavior spread throughout your program, you might want to refactor this like:

p.Discount = DiscountFactory.Create(reader[discountTypeIndex]);

Then the discount object contains all the attributes and methods related to figuring out discounts.

回答9:

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.

回答10:

Yes, this looks like a correct usage of switch statement.

However, I have another question for you.

Why haven't you included the default label? Throwing an Exception in the default label will make sure that the program will fail properly when you add a new discountTypeIndex and forget to modify the code.

Also, if you wanted to map a string value to an Enum, you can use Attributes and reflection.

Something like:

public enum DiscountType
{
    None,

    [Description("A")]
    Discountable,

    [Description("B")]
    Loss,

    [Description("O")]
    Other
}

public GetDiscountType(string discountTypeIndex)
{
    foreach(DiscountType type in Enum.GetValues(typeof(DiscountType))
    {
        //Implementing GetDescription should be easy. Search on Google.
        if(string.compare(discountTypeIndex, GetDescription(type))==0)
            return type;
    }

    throw new ArgumentException("DiscountTypeIndex " + discountTypeIndex + " is not valid.");
}

回答11:

I think this depends if you are creating MType add many different places or only at this place. If you are creating MType at many places always having to switch for the dicsount type of have some other checks then this could be a code smell.

I would try to get the creation of MTypes in one single spot in your program maybe in the constructor of the MType itself or in some kind of factory method but having random parts of your program assign values could lead to somebody not knowing how the values should be and doing something wrong.

So the switch is good but maybe the switch needs to be moved more inside the creation part of your Type

回答12:

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.

回答13:

When you design a language and finally have a chance to remove the ugliest, most non-intuitive error prone syntax in the whole language.

THAT is when you try and remove a switch statement.

Just to be clear, I mean the syntax. This is something taken from C/C++ which should have been changed to conform with the more modern syntax in C#. I wholeheartedly agree with the concept of providing the switch so the compiler can optimise the jump.