什么时候应该尝试消除switch语句？

Question

我在我正在处理的代码库中遇到了一个switch语句，我试图弄清楚如何从switch statements are considered a code smell更好地替换它。然而，通过several在flipoverflow上关于replacingswitchstatements的帖子我似乎无法想到一个有效的方式来取代这个特定的开关语句。

它让我想知道这个特定的switch语句是否正常，以及是否存在switch语句被认为合适的特定情况。

在我的情况下我与挣扎的代码（自然略有混淆）是这样的：

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

任何人都可以提出一个方法来消除这种开关？或者这是一个适当的使用开关？如果是的话，switch语句还有其他适当的用法吗？我真的很想知道它们在哪里是合适的，所以我不会浪费太多时间来试图消除我遇到的每个开关语句，只是因为在某些情况下它们被认为是嗅觉。

更新：在的Michael建议我做了一些搜索出该逻辑重复的，发现有人在另一个类中，有效地使整个switch语句创建冗余的逻辑。所以在这段代码中，switch语句是不必要的。然而，我的问题更多的是关于switch语句在代码中的适用性，以及我们是否应该总是试图在发现它们时进行替换，因此在这种情况下，我倾向于接受这个switch语句合适的答案。

Answer 1

这是一个适用于开关状态的选项，因为它使选项可读，并且易于添加或减少选项。

See this link.

Answer 2

我不会用一个if。 if将不如switch明确。 switch告诉我你在比较同一件事。

只是为了吓唬人，这是不是你的代码不太清楚：

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; 

这switch可能是OK，你可能想看看@Talljoe建议。

Answer 3

这个switch语句很好。你们没有任何其他的错误吗？大声笑

但是，有一件事我注意到了......你不应该在IReader []对象索引器上使用索引序号......如果列顺序改变怎么办？尝试使用字段名即阅读器[“id”]和阅读器[“名称”]

Answer 4

开关是否位于整个代码中的折扣类型？添加新的折扣类型是否需要您修改几个这样的交换机？如果是这样的话，你应该考虑把交换出去。如果没有，在这里使用开关应该是安全的。

如果有很多折扣特定行为传遍你的程序，你可能要重构这个样：

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

然后贴现对象包含所有相关搞清楚折扣的属性和方法。

Answer 5

开关语句（尤其是长的）被认为是不好的，不是因为它们是switch语句，而是因为它们的存在表明需要重构。

switch语句的问题是它们在代码中创建了一个分叉（就像if语句一样）。每个分支必须单独进行测试，并且每个分支内的每个分支......以及您的想法。

这就是说，下面的文章对使用switch语句的一些好的做法：

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

在你的代码的情况下，在上面的链接的文章表明，如果你执行这个从一个枚举到另一个枚举的转换类型，您应该将自己的开关放在自己的方法中，并使用return语句而不是break语句。我这样做过，并且代码看起来更清洁：

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

Answer 6

我认为改变代码的缘故更改代码不是那些时间充分利用。改变代码使其[更可读，更快，更高效等等]是有道理的。不要仅仅因为有人说你在做一些“臭”的事情而改变它。

-Rick

Answer 7

在我看来，这不是将那些气味语句​​，它的里面有什么他们。这个开关语句对我来说没问题，直到它开始添加更多的情况。那么它可能是值得创建一个查找表：

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

根据您的观点，这可能是或多或少可读。

事情开始变臭的地方是你的案件内容超过一两行。

Answer 8

我认为这取决于如果你正在创建MType添加许多不同的地方或只在这个地方。如果你在许多地方创建MType，总是必须切换为dicsount类型的其他检查，那么这可能是一种代码异味。

我会尝试在你的程序中的一个地方获得MTypes的创建也许在MType本身的构造函数或某种工厂方法，但有随机部分的程序分配值可能会导致某人不知道价值观应该如何，做错了什么。

所以开关是好的，但也许切换需要移动更多的类型

Answer 9

是的，这看起来像switch语句的正确用法的创建部分内。

但是，我有另一个问题给你。

为什么你不包含默认标签？在默认标签中投掷异常将确保当您添加新的discountTypeIndex并忘记修改代码时，程序将正常失败。

此外，如果您想将字符串值映射到Enum，则可以使用Attributes和reflection。

喜欢的东西：

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

Answer 10

当您设计语言，终于有机会以消除整个语言中最丑陋，最不直观容易出错的语法。

这是当您尝试并删除switch语句。

只是要清楚，我的意思是语法。这是从C/C++中取得的，它应该被改变以符合C＃中更现代的语法。我完全同意提供开关的概念，以便编译器可以优化跳转。

Answer 11

我并不完全反对switch语句，但在你提出的情况下，我至少已经消除了指定DiscountType的重复;我可能会写一个函数返回给定字符串的DiscountType。该功能可以简单地为每个案例返回陈述，而不需要休息。我发现切换案件之间的中断需要非常危险。

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 
} 

很快，我已经注意到FindDiscountType（）真的属于DiscountType类并移动了函数。

Answer 12

罗伯特哈维和Talljoe提供了很好的答案 - 你在这里是一个从字符代码到枚举值的映射。这最好表示为一个映射，其中映射的细节在一个地方提供，无论是在地图中（如Talljoe所建议的），还是在使用switch语句的函数中（如Robert Harvey所建议的）。

这两方面的技术是在这种情况下可能很好，但我想提醒你注意一个设计原则可能是有用的在这里或在其他类似的情况。 的打开/关闭本金：

• http://en.wikipedia.org/wiki/Open/closed_principle
• http://www.objectmentor.com/resources/articles/ocp.pdf（！请务必阅读本）

如果映射可能会改变随着时间的推移，或可能延长运行时（例如，通过插件系统或通过读取数据库中的映射部分），那么使用注册表模式将帮助您遵守开放/关闭主体，实际上允许扩展映射而不影响任何公司de使用映射（正如他们所说 - 打开扩展，关闭修改）。

我认为这是关于注册表模式的一篇很好的文章 - 看看注册表如何保存从某个键到某个值的映射？用这种方式，它与您的映射表示为switch语句相似。当然，你的情况，你将不会被注册，所有实现一个共同的接口的对象，但你应该得到的要点：

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

因此，要回答原来的问题 - case语句是不好的形式，因为我预计从应用程序的多个位置需要从字符代码到枚举值的映射，因此应该将其分解出来。我提到的两个答案为您提供了如何做到这一点的好建议 - 请选择您喜欢的方式。但是，如果映射可能随时间而改变，请考虑使用注册表模式作为隔离代码免受此类更改影响的一种方式。

Answer 13

你有权怀疑这个switch语句：任何switch语句取决于某种类型的东西可能表示缺少多态性（或缺少子类）。

但是，TallJoe的字典是一个很好的方法。

请注意，如果您的枚举和数据库值是整数而不是字符串，或者如果您的数据库值与枚举名称相同，则反射会起作用。鉴于

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

然后

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

就足够了。