java - ¿Cómo puedo mejorar la legibilidad y la duración de un método con muchas declaraciones if?

design-patterns

Question

Tengo un método con 195 si es. Aquí hay una versión más corta:

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

Puedo cambiar si es para cambiar:

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

Pero 195 casos sigue siendo tan largo. ¿Cómo podría mejorar la legibilidad y la duración de ese método? ¿Qué patrón sería el mejor en este caso?

Answer 1

Coloque los datos en un archivo o base de datos XML, luego utilícelos para completar un diccionario. De esa manera, puede cambiar los datos fácilmente y separarlos de la lógica de la aplicación. O simplemente ejecuta una consulta SQL en tu método.

Answer 2

Como un desafío de marco ...

195 casos no es demasiado largo si está claro lo que están haciendo y por qué, y si el código dentro de cada caso es mínimo. Sí, es largo, pero es perfectamente legible porque sabes exactamente lo que está haciendo. La longitud no implica necesariamente ilegibilidad.

Como han dicho otras respuestas, por supuesto, esto puede ser un olor a código que indica que no está usando OO correctamente. Pero por sí solo, es solo largo, no ilegible.

Answer 3

Cree un Map<String,Double> que asigne nombres de países a sus tasas de impuestos correspondientes:

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

Utilice ese Map siguiente manera:

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

Answer 4

Si los valores son constantes y no están destinados a ser cambiados regularmente (lo cual dudo). Yo introduciría un metamodelo estático usando Enum:

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

Entonces

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

Es legible, seguro de compilación, fácilmente ampliable con metadatos adicionales por país y menos repetitivo.

Answer 5

EDIT: se perdió la respuesta de Alexander; Puede que sea un poco excesivo, pero también está tocando el punto principal: usar OOP.
EDIT 2: implementado las sugerencias de @Luaan

Probablemente me esté perdiendo algo obvio, y podría ser un poco difícil de implementar en esta etapa tan tardía, pero esto me parece un caso perfecto para la programación orientada a objetos :

Crea una clase de Country que contiene todo lo que pertenece a un país, como un name y un método calculaTax calculateTax() y otras cosas, y luego su llamante ( calculateTotalAmount() , o lo que sea) llamará a country.calculateTax(amount) lugar de calculateTax(country, amount) , y toda la construcción if / switch acaba de desaparecer.

Además, cuando agrega soporte para un nuevo país (por ejemplo, hay otra guerra civil en algún lugar y un país se divide), simplemente agrega todo para el nuevo país en un solo lugar en lugar de perseguir a millones de métodos con gigantescas cadenas if() o switch() es ...

Answer 6

¡No hagas esto!

Tal como está ahora, su método calculateTax es como un contenedor para cuatro métodos calculateTax reales, uno para cada uno de los 3 países, y uno para el caso no válido. Cualquier otro método que hagas en este sentido será así. Siguiendo este patrón, terminará con muchos interruptores (verificando el mismo conjunto de casos) dentro de muchos métodos, donde cada caso contiene los detalles de un caso. ¡Pero eso es exactamente lo que hace el polimorfismo, de una manera mucho mejor!

Este tipo de patrones son una indicación muy clara de que no se está aprovechando de la orientación a objetos y, a menos que tenga otras razones para hacerlo, definitivamente debería hacerlo. Es Java después de todo, y eso es todo el esquema.

Crear una interfaz como TaxPolicy :

interface TaxPolicy {
    BigDecimal calculateTaxFor(BigDecimal saleAmount);
}

Crea una clase que lo implemente:

class NationalSalesTaxPolicy implements TaxPolicy  {
    String countryName;
    BigDecimal salesTaxRate;

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

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

Luego, cree objetos de esta clase, uno por país que desee apoyar. Podemos incluir esta lista en una nueva clase, NationalSalesTaxCalculator , que será nuestra ventanilla única para calcular el impuesto a las ventas para cualquier país:

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

Y podemos usarlo como:

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

Algunos beneficios clave a tener en cuenta:

1. Si agrega un nuevo país que desea admitir, solo tiene que crear un nuevo objeto. Todos los métodos que puedan necesitar ese objeto, automáticamente "hacen lo correcto", sin necesidad de encontrarlos manualmente, para agregar nuevas instrucciones if.
2. Tienes espacio para adaptar la funcionalidad según sea necesario. Por ejemplo, donde vivo (Ontario, Canadá), el impuesto a las ventas no se cobra por los comestibles. Así que podría hacer mi propia subclase de NationalSalesTaxPolicy que tiene una lógica más matizada.

3. Incluso hay más espacio para mejorar. Tenga en cuenta que NationalSalesTaxCalculator.calculateTaxFor() contiene algún código específico para manejar un país no compatible. Si agregamos nuevas operaciones a esa clase, todos los métodos necesitarán la misma comprobación nula y el mismo error.

   En su lugar, eso podría ser refactorizado en el uso del patrón de objeto nulo . Usted implementa un UnsuppoertedTaxPolicy , que es una clase que implementa todos los métodos de interfaz lanzando excepciones. Al igual que:

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

   Entonces puedes hacer

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

   Esto "centraliza" todas sus excepciones en un solo lugar, lo que las hace más fáciles de encontrar (por lo tanto, es más fácil establecer puntos de ruptura), más fácil de editar (en caso de que alguna vez quiera migrar los tipos de excepción, o cambie el mensaje), y desordena el resto del código, de modo que solo tiene que preocuparse por el caso feliz.

Aquí hay una demostración de trabajo: https://repl.it/@alexandermomchilov/Polymorphism-over-ifswitch