如何提高具有许多if语句的方法的可读性和长度？

Question

我有一个包含195个ifs的方法。这是一个更短的版本：

private BigDecimal calculateTax(String country, BigDecimal amount) throws Exception {
    if(country.equals("POLAND")){
        return new BigDecimal(0.23).multiply(amount);
    }
    else if(country.equals("AUSTRIA")) {
        return new BigDecimal(0.20).multiply(amount);
    }
    else if(country.equals("CYPRUS")) {
        return new BigDecimal(0.19).multiply(amount);
    }
    else {
        throw new Exception("Country not supported");
    }
}

我可以将if语句改为switch语句：

private BigDecimal calculateTax(String country, BigDecimal amount) throws Exception {
    switch (country) {
        case "POLAND":
            return new BigDecimal(0.23).multiply(amount);
        case "AUSTRIA":
            return new BigDecimal(0.20).multiply(amount);
        case "CYPRUS":
            return new BigDecimal(0.19).multiply(amount);
        default:
            throw new Exception("Country not supported");
    }
}

但是195个案例还是太多了。我该如何提高方法的可读性和简洁性？在这种情况下，哪种模式最好？

- Michu93

---

使用 java.util.Map ... ? - Oleksandr Pyrohov

是的，在这里使用地图或字典可能是最好的选择。 - ncbvs

警告：避免在BigDecimal中使用浮点数。BigDecimal（0.23）可能与BigDecimal（23）/ BigDecimal（100）不同。后者是23％的正确表示方式。 - gudok

我建议关闭这个问题，原因是关于优化现有、可工作代码的问题应该发到Code Review。请注意，这里只需要翻译，不可以添加任何额外的内容或解释。 - Jonathon Reinhart

你绝对不应该使用地图，虽然对于这个玩具问题它可以工作，但在将来的可扩展性方面非常有限（当你需要更多情况、除税率之外更多的负载等等）。这就是面向对象的用途。请参见我的回答。 - Alexander

Answer 1

创建一个将国家名称映射到其相应税率的Map<String,Double>:

Map<String,Double> taxRates = new HashMap<> ();
taxRates.put("POLAND",0.23);
...

使用该Map的方法如下：

private BigDecimal calculateTax(String country, BigDecimal amount) throws Exception {
    if (taxRates.containsKey(country)) {
        return new BigDecimal(taxRates.get(country)).multiply(amount);
    } else {
        throw new Exception("Country not supported");
    }
}

- Eran

---

这似乎不是抛出checked异常的合理位置。我知道OP的代码这样做了，但我仍然不想传播它。 - Ben P.

如果收到意外或不支持的输入，抛出一个受检异常是没有问题的。我可能会使用一些自定义异常而不是基本的Exception类。 - Eran

使用 IllegalArgumentException 应该可以，但是普通的 Exception 不行。 - Marco13

你绝对不应该使用map，虽然在这个玩具问题中它可以工作，但在未来的可扩展性方面（当你需要更多情况、更多有效载荷以及税率之外的内容时）它非常有限。这就是面向对象编程的用途。请参见我的答案。 - Alexander

小小的建议：Map.ofEntries() 可能比重复所有 put 调用更好。大一点的问题是，仍然有太多的代码重复，并且其他评论者说得对，我的意见是将数据拆分成文件或数据库中，并从中填充地图或直接查询数据源。还有就是异常处理是大问题了，确实需要注意 :) - Ray Toal

Answer 2

将数据放入XML文件或数据库中，然后使用它来填充字典。这样可以轻松更改数据，并将数据与应用逻辑分离。或者，可以在您的方法中执行SQL查询。

- EJoshuaS - Stand with Ukraine

Answer 3

不要这样做！

目前，你的calculateTax方法就像一个包含四个实际calculateTax方法的容器，分别是三个国家和一个无效情况。按照这种方式制作的任何其他方法都会是这样。遵循这个模式，您将在许多方法中使用许多开关（检查相同的一组情况），其中每种情况都包含案例的具体内容。但是这正是多态以更好的方式实现的！

像这样的模式非常明显地表明您没有充分利用面向对象，并且在没有其他原因的情况下，您绝对应该这样做。毕竟，这是Java，也是整个玩意儿的核心。

创建像TaxPolicy这样的接口：

interface TaxPolicy {
    BigDecimal calculateTaxFor(BigDecimal saleAmount);
}

创建一个实现它的类：

class NationalSalesTaxPolicy implements TaxPolicy  {
    String countryName;
    BigDecimal salesTaxRate;

    // Insert constructor, getters, setters, etc. here

    BigDecimal calculateTaxFor(BigDecimal saleAmount) {
        return saleAmount.multiply(salesTaxRate);         
    }
}

然后，根据需要支持的每个国家创建此类的对象。我们可以将此列表封装到一个新类NationalSalesTaxCalculator中，这将成为我们计算任何国家销售税的一站式商店：

class NationalSalesTaxCalculator {
    static Map<String, NationalSalesTaxPolicy> SUPPORTED_COUNTRIES = Stream.of(
        new NationalSalesTaxPolicy("POLAND", "0.23"),
        new NationalSalesTaxPolicy("AUSTRIA", "0.20"),
        new NationalSalesTaxPolicy("CYPRUS", "0.19")
    ).collect(Collectors.toMap(NationalSalesTaxPolicy::getCountryName, c -> c));

    BigDecimal calculateTaxFor(String countryName, BigDecimal saleAmount) {
        NationalSalesTaxPolicy country = SUPPORTED_COUNTRIES.get(countryName);
        if (country == null) throw new UnsupportedOperationException("Country not supported");

        return country.calculateTaxFor(saleAmount);
    }
}

我们可以像这样使用它:

NationalSalesTaxCalculator calculator = new NationalSalesTaxCalculator();
BigDecimal salesTax = calculator.calculateTaxFor("AUSTRIA", new BigDecimal("100"));
System.out.println(salesTax);

需要注意的一些关键优点：

1. 如果您要添加要支持的新国家，您只需创建一个新对象。 所有可能需要该对象的方法都会自动"做正确的事情"，而无需手动找到它们所有的位置，以便添加新的if语句。
2. 您可以根据需要调整功能。 例如，在我居住的地方（加拿大安大略省），不会为杂货收取销售税。 因此，我可以制作自己的NationalSalesTaxPolicy子类，具有更细致入微的逻辑。

3. 甚至还有改进的空间。 注意NationalSalesTaxCalculator.calculateTaxFor()包含一些特定于处理不受支持的国家的代码。 如果我们在该类中添加新操作，则每个方法都需要相同的null检查和错误抛出。

   相反，可以重构为使用null object pattern。您可以实现一个UnsuppoertedTaxPolicy，这是一个通过抛出异常来实现所有接口方法的类。 就像这样：

   class UnsuppoertedTaxPolicy implements TaxPolicy {
       public BigDecimal calculateTaxFor(BigDecimal saleAmount) {
           throw new UnsupportedOperationException("Country not supported");
       }
   }
   

   然后您可以执行

   TaxPolicy countryTaxPolicy = Optional
       .ofNullable(SUPPORTED_COUNTRIES.get(countryName))
       .orElse(UNSUPPORTED_COUNTRY);
   return countryTaxPolicy.calculateTaxFor(saleAmount);
   

   这个"集中化"所有的异常到一个地方，使得它们更容易找到（因此更容易设置断点），更容易编辑（如果您想迁移异常类型或更改消息），并且它减少了其余代码的混乱程度，只需关注 happy case。

这是一个可工作的演示：https://repl.it/@alexandermomchilov/Polymorphism-over-ifswitch

- Alexander

---

问题在于，这本身不太可扩展。要添加功能，比如“在加拿大（或更糟糕的安大略省）为杂货不收取销售税”，你需要改变接口。通常需要面向对象编程来解决问题，但你唯一可以做的就是增加大量实际上没有作用的代码。在你有理由这样做之前，将代码改为面向对象并不能节省开发时间。当然，一旦出现这样的需求（比如你的变量销售税），这便是正确的方式。在此之前，简单方法更好。 - Luaan

也不要这样做。您仍然需要硬编码许多值，这些值可能在几个月内发生变化。将数据编写为JSON / Sqlite / XML将是更好的选择。 - Eric Duminil

大的挑剔：YAGNI。小的挑剔：在第一部分中加入（精神上的）“多态是解决方案”并不真正合适。“你最终会有很多开关”的问题可以通过Map来解决，就像被接受的答案一样。只有在最后（作为解决不同问题的解决方案！），您才提供“对不同类型实体的单个接口”（NationalSalesTaxPolicy vs. UnsupportedTaxPolicy）。 - Raphael Schmitz

这真的是企业级编码。在某种程度上，我同意：作为一个经验法则，每个 switch 甚至大多数 if 都应该至少被考虑作为实现多态性的候选对象。但是此时，接口是微不足道的 BigDecimal compute(String, BigDecimal)。即使那些结构已经就位，我也会将您的使用示例提取到一个（私有静态）方法中，该方法具有完全相同的签名，其中创建了 calculator 实例。现在，您可以开始进行依赖注入... - Marco13

@Marco13 这是我认为最合适的地方之一。一般而言，我不是企业 Java 的粉丝，但是这个领域变化非常快，值得采用，依我之见。 - Alexander

Answer 4

作为一种框架挑战，如果它们正在做什么以及为什么要清楚，而且每种情况下的代码都很简洁，那么195个案例就不算太长。是的，它很长，但因为您知道它在做什么，所以它非常易读。长度并不一定意味着不可读。

正如其他答案所说，这可能是代码异味，表明您没有正确使用OO。但单独来看，它只是长而已，不是不可读的。

- Graham

---

这里唯一有点问题的就是重复的“amount * taxRate”。不仅很容易解决，而且也会让人难以发现那些做了不同处理的情况（这种情况几乎肯定会在某个时候出现）。 - Luaan

实际上，195并不是真正的问题。真正的问题在于这些数字是易变的，并且会在特定日期（税率变化时）发生变化。因此，需要有一种一致的更新方法/过程，也允许输入未来的更改。硬编码和手动更改，可能需要个人日历提醒，这不是一个解决方案。（除了玩具程序） - lalala

@Luann 我认为唯一不同的情况在代码中会更加突出？当然，从需求方面忘记覆盖这种情况也是非常容易的！我同意其他人的看法，即 OP 的具体示例有更好的解决问题的方法。总的来说，如果函数的作用很清晰，因此可以轻松地根据设计/需求检查其正确性，那么我并不担心长函数。这才是真正重要的事情，而对于函数长度的建议只是帮助使这一点变得更容易的方法。 - Graham

Answer 5

如果这些值是常量且不经常更改（我怀疑），那么我会使用枚举引入一个静态元模型：

public enum CountryList {

    AUSTRIA(BigDecimal.valueOf(0.20)),
    CYPRUS(BigDecimal.valueOf(0.19)),
    POLAND(BigDecimal.valueOf(0.23));

    private final BigDecimal countryTax;

    CountryList(BigDecimal countryTax) {
        this.countryTax = countryTax;
    }

    public BigDecimal getCountryTax() {
        return countryTax;
    }

    public static BigDecimal countryTaxOf(String countryName) {
        CountryList country = Arrays.stream(CountryList.values())
                .filter(c -> c.name().equalsIgnoreCase(countryName))
                .findAny()
                .orElseThrow(() -> new IllegalArgumentException("Country is not found in the dictionary: " + countryName));

        return country.getCountryTax();
    }
}

那么

private BigDecimal calculateTax(String country, BigDecimal amount) throws Exception {
    return CountryList.countryTaxOf(country).multiply(amount);
}

这段话的意思是：易读、编译时安全，可以很容易地通过添加每个国家的附加元数据进行扩展，而且不需要写很多重复的代码。

- Mikhail Kholodkov

---

为什么使用流？ - user253751

这有什么问题吗？stream()通常比for-loop更快，并且没有副作用。 - Mikhail Kholodkov

“stream()通常比for循环更快” - 有任何证据吗？对于并行流和非常大的数字，这很可能是正确的，但对于像195这样的小数字，设置开销、堆翻转等（如果并行，则为同步）的开销很可能会超过任何潜在的节省。使用流的好理由很多，但在小数字上的性能不是其中之一！ - gidds

有证据吗？基准测试呢？ forEachStream 看起来比同样的 for-loop 快1.3-1.5倍。甚至不用提简洁性和可读性。 - Mikhail Kholodkov

嗯，我也在想为什么你没有使用 CountryList.valueOf(countryName.toUpperCase(Locale.ROOT))（在进行不区分大小写的比较时，请确保指定语言环境！） - user253751

Answer 6

编辑：错过了@Alexander的答案；他也提到了重点：使用面向对象编程。
编辑2：实现了@Luaan的建议。

我可能忽略了一些显而易见的事情，而且在现在这么晚的阶段实现起来可能有点困难，但是对于这种情况来说，我觉得它非常适合使用面向对象编程：

你可以创建一个包含与国家相关的所有内容的Country类，比如name和calculateTax()方法等等，然后你的调用者（例如calculateTotalAmount()）将调用country.calculateTax(amount)而不是calculateTax(country, amount)，整个if/switch结构就消失了。

此外，当你为一个新国家添加支持时（比如说，某个地方爆发内战，一个国家分裂了），你只需要在一个地方添加新国家的所有内容，而不是寻找数以亿计的带有庞大if()链或switch()语句的方法...

- Christian Severin

---

拥有一个公共的taxFactor并不是真正的“面向对象编程”，你基本上是在重新发明过程式/早期函数式编程（这并没有什么错）。更面向对象的方法是将税收计算放在国家对象中，或者拥有一个单独的税收计算器对象，可以为给定的国家创建（并可能接受其他参数）。当你意识到国家通常首先没有统一的税率时，这变得尤为重要。 - Luaan

@Luaan 你说得对，我们可以在我们的国家对象中 calculateTax()，而不是让调用者执行 calculateTax(country, amount)，它只需调用 country.calculateTax(amount)。好多了。如果还有可能的话，我会进行编辑。 - Christian Severin

Answer 7

如果您坚持使用switch语句：自JDK 12以来，switch表达式可以通过允许多个case标签和类似lambda的语法来提高可读性和代码长度：

private BigDecimal calculateTax(String country, BigDecimal amount) throws Exception {
    double rate = switch(country) {
        case "POLAND", "IRELAND" -> 0.23;
        case "AUSTRIA", "FRANCE" -> 0.20;
        case "CYPRUS", "GERMANY" -> 0.19;
        default -> throw new Exception("Country not supported");
    };
    return new BigDecimal(rate).multiply(amount);
}

- Christian S.