logo标签列表

何时应该尝试消除 switch 语句？

c#refactoringswitch-statement
8
我在正在处理的代码库中遇到了一个switch语句，并尝试着找出如何用更好的方式替换它，因为 switch语句被认为是一种代码异味。然而，我阅读了几篇关于替换 switch 语句的stackoverflow帖子，似乎想不出一种有效的方法来替换这个特定的switch语句。
这让我想知道这个特定的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;
}

有人能建议一种消除这个开关的方法吗？或者这是使用开关的适当方式吗？如果是，还有其他适当使用开关语句的方法吗？我真的很想知道它们何时适用，以便我不会因为在某些情况下被认为是一种味道而试图消除我遇到的每一个开关声明。

更新：在 Michael的建议下，我搜索了一下这个逻辑的重复，并发现其他类中的某个人已经创建了逻辑，有效地使整个开关语句变得多余。因此，在这个特定代码的上下文中，开关语句是不必要的。然而，我的问题更多地涉及到开关语句在代码中的适当性以及我们是否应该总是尝试替换它们，无论何时发现它们，所以在这种情况下，我倾向于接受这个开关语句是适当的答案。

1
你能添加一个标签来包含这个代码所写的编程语言吗？虽然代码在做什么方面很清楚，但我认为区分它是有帮助的。显然这不是Java，因为Java中没有“string”类。 - Amir Afghani
@Amir 我已经在标签中确认了代码是C#。我之前没有这样做的原因是因为我不想特别针对C#提出问题，因为我的问题更多地涉及使用switch语句的适当性问题... - mezoid
1
我猜你的经验证实了你对switch语句的怀疑。这个代码块本身还好，但你四处看了看，发现了其他的问题。所以值得进一步调查。 - jrcs3
13个回答
16

使用switch语句是合适的，因为它使得选择更易读，并且容易添加或删除一个选项。

查看此链接。

+1 感谢分享链接，Lance...非常有价值的阅读。 - mezoid
我不确定switch语句是否应该嵌套在DoSomething()方法中... - Daniel Paull
14

Switch语句（尤其是长的）被认为是不好的，不是因为它们是Switch语句，而是因为它们的存在暗示着需要重构代码。

Switch语句的问题在于它们在代码中产生了一个分歧（就像if语句一样）。每个分支必须单独进行测试，每个分支内的每个分支等等...你知道的。

话虽如此，以下文章提供了一些关于使用Switch语句的良好实践：

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

针对你的代码，在上述链接中的文章建议，如果正在执行从一个枚举到另一个枚举的转换类型操作，则应将Switch语句放在自己的方法中，并使用return语句而不是break语句。我以前做过这件事，代码看起来更加整洁：

private DiscountType GetDiscountType(string discount)
{
    switch (discount)
    {
        case "A": return DiscountType.Discountable;
        case "B": return DiscountType.Loss;
        case "O": return DiscountType.Other;
    }
}
如果您认为switch语句不好或建议进行重构，那么很好奇如何重构此代码。我相信这是他最初的问题... - bbqchickenrobot
我在答案里提供的文章中有一个这样重构的例子。但真正的重构是当您可以以一种不再需要switch语句的方式重新设计代码时发生的。这是件具体情况具体分析的事情。一些switch语句是“坏味道”代码，因为它们存在于设计不良的情况下。 - Robert Harvey
你可以就像那样省略 break; 语句吗？ - scottm
谢谢提供链接。我一定会考虑将switch语句移动到未来代码中的方法中的建议。 - mezoid
是的，您可以消除break语句。实际上，如果您不这样做，编译器会抱怨，因为它是无法到达的代码。 - Robert Harvey
这里不需要使用break语句，因为return语句已经替代了它的作用。 - Robert Harvey
4

我认为仅仅为了改变代码而改变代码并不是最佳利用时间的方式。将代码更改为[更易读、更快、更高效等等]是有意义的。不要仅仅因为有人说你正在做某些“难闻”的事情而改变它。

-Rick

1
+1，如果 switch 语句不好的话，我认为接近天才的 C# 创作者们会把它留出来。就像他们对待多重继承一样。当然，像任何东西一样，包括泛型或接口，一个概念可以被完全滥用和/或过度使用。 - bbqchickenrobot
@Rick 我同意你的观点。然而，我重构这个方法的原因是因为我感觉该方法的其他部分（出于简洁起见，我没有在代码片段中展示）需要进行一些清理，而这个 switch 语句只是我考虑清理的事项列表中的一个。 - mezoid
2
在我看来，问题并不在于 switch 语句本身，而是在于 switch 语句内部的内容。在我看来，只有当 switch 语句中添加了更多的 case 之后，才需要考虑是否值得创建一个查找表（lookup table）。
private static Dictionary<string, DiscountType> DiscountTypeLookup = 
  new Dictionary<string, DiscountType>(StringComparer.Ordinal)
    {
      {"A", DiscountType.Discountable},
      {"B", DiscountType.Loss},
      {"O", DiscountType.Other},
    };

根据您的角度，这可能更或少可读。

当您的案例内容超过一行或两行时，事情开始变得复杂。

2

这个switch语句没问题。你们没有其他bug要处理吗？哈哈

然而，我注意到了一件事情...你不应该在IReader[]对象索引器上使用索引序号...如果列顺序改变了会怎么样？尝试使用字段名，例如reader["id"]和reader["name"]。

1
从我的代码片段中并不明显，但是代码确实使用了字段名称...discountTypeIndex在一个名为PopulateOrdinals的方法中被赋值，如下所示：discountTypeIndex = reader.GetOrdinal("discount_type")。 - mezoid
1
调用 GetOrdinal() 一次并重复使用索引 (就像 @mezoid 所做的那样) 比每次调用字符串索引器更有效。这是否值得努力取决于你的情况和你预计返回多少条记录。 - Talljoe
2
Robert Harvey和Talljoe提供了出色的答案——您在此处拥有的是从字符代码到枚举值的映射。最好将其表达为一种映射，其中映射的细节都在一个地方提供，可以是一个map（如Talljoe建议的）或使用switch语句的函数（如Robert Harvey建议的）。
这些技术在这种情况下都可能很好，但我想吸引您关注一个设计原则，在这里或其他类似情况下可能会有用。该原则是开放/封闭原则： 如果映射可能随时间而变化，或者可能在运行时扩展（例如通过插件系统或从数据库中读取映射的部分），那么使用注册表模式将有助于您遵循开放/封闭原则，实际上允许扩展映射而不影响使用映射的任何代码（正如他们所说的-开放以扩展，关闭以修改）。
我认为这是关于注册表模式的一篇不错的文章-看看注册表如何将某个键与某个值进行映射？这样它就类似于您表达为switch语句的映射。当然，在您的情况下，您不会注册所有实现公共接口的对象，但是您应该理解要点： 因此，回答最初的问题 - case语句是不良形式，因为我预计从字符代码到枚举值的映射在您的应用程序中将需要多次使用，因此应将其分解。我参考的两个答案都给出了如何实现这一点的好建议 - 选择您喜欢的。但是，如果映射可能随时间而变化，请考虑使用注册表模式来隔离代码免受此类更改的影响。
1

我不会使用 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 可能没问题，但你可能想看看 @Talljoe 的建议。

1

你的代码中是否到处都是打折类型的开关？如果添加一个新的打折类型需要修改多个这样的开关，那么你应该考虑将开关分离出来。如果不是，那么在这里使用开关应该是安全的。

如果你的程序中有很多特定于打折的行为分散在各处，你可能需要像这样重构：

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

那么折扣对象包含所有与计算折扣相关的属性和方法。

目前我不知道它在其他地方是否被使用...但这是因为我对代码库的这个特定部分不熟悉...如果我看到重复的情况，我会记住你的建议... - mezoid
1

是的，这看起来像是正确使用 switch 语句。

不过，我还有一个问题要问你。

为什么你没有包含默认标签？在默认标签中抛出异常将确保当您添加新的 discountTypeIndex 并忘记修改代码时程序能够正确失败。

此外，如果您想将字符串值映射到枚举类型，可以使用属性和反射。

类似于：

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.");
}
1
你怀疑这个switch语句是正确的：任何以某个东西的类型为前提的switch语句可能表明缺少多态性（或缺少子类）。
TallJoe的字典是一个很好的方法，然而。
请注意，如果你的枚举和数据库值不是字符串而是整数，或者如果你的数据库值与枚举名称相同，则反射将起作用，例如给定
public enum DiscountType : int
{
    Unknown = 0,
    Discountable = 1,
    Loss = 2,
    Other = 3
}

那么

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

就足够了。

网页内容由stack overflow 提供, 点击上面的
可以查看英文原文，
原文链接