Is it wrong to use multiple cases for the same action on the switch?

Asked

Viewed 1,357 times

12

It’s wrong to do that with the switch? I programmed like this with him, it’s wrong?

switch(estado){
case "AL":
case "BA":
case "CE":
case "MA":
case "PB":
case "PE":
case "PI":
case "RN":
case "SE":
//FAZ ALGO AQUI PARA TODOS OS CASES DO NORDESTE
break;
case "ES":
case "MG":
case "RJ":
case "SP":
//FAZ ALGO AQUI PARA TODOS OS CASES DO SUDESTE
break;
etc...
  • 1

    So né... haha ai depends, I find it aesthetically ugly hehe so in my case I prefer to use if and even comparisons, but it goes of the taste of each one =)

  • 4

    Usually the switch is a bad choice. But to know what would be best in your case, you would have to say what you want to do with those states.

  • set a different color for each region. That’s all.

  • 1

    It’s not really an answer, but it seems to me you’re modeling regions implicitly through your switch statement. An alternative to its solution would be to include the regions in the model (e. g., enum Regiao with NORDESTE, SUDESTE, etc). It seems to me cleaner to treat 5 regions than one switch with all states + DF.

  • 1

    @Matheus but in question of many validations, use if would not make the execution slower than the function switch? The switch has the possibility to brake (break) the code if the value corresponds, already the if would cause unnecessary processing as if I found what I needed I would not need to continue processing. Or if also has a break internally?

  • 1

    For simple cases you can use that approach is valid for any language with some adaptations. For more complex uses see the other answers ;)

  • 1

    In programming, as in any other aspect of life, one thing is only wrong if there is a reason why it is wrong. Good BTW question.

  • 1

    @Tiagoboeing yes this is true, so I quoted that it goes from the taste of each =), if we use Else if we will also stop going through unnecessary codes

Show 3 more comments

3 answers

18


No, this is perfectly valid and usual if that’s what you want.

If all these states must perform the same action it must be so.

In general will have better performance than to do the same with if, out that makes more sense in this case.

I’d just tag and line up a little better:

switch (estado) {
    case "AL":
    case "BA":
    case "CE":
    case "MA":
    case "PB":
    case "PE":
    case "PI":
    case "RN":
    case "SE":
        //FAZ ALGO AQUI PARA TODOS OS CASES DO NORDESTE
        break;
    case "ES":
    case "MG":
    case "RJ":
    case "SP":
        //FAZ ALGO AQUI PARA TODOS OS CASES DO SUDESTE
        break;
    etc...
}

Or

switch (estado) {
case "AL":
case "BA":
case "CE":
case "MA":
case "PB":
case "PE":
case "PI":
case "RN":
case "SE":
    //FAZ ALGO AQUI PARA TODOS OS CASES DO NORDESTE
    break;
case "ES":
case "MG":
case "RJ":
case "SP":
    //FAZ ALGO AQUI PARA TODOS OS CASES DO SUDESTE
    break;
etc...
}

I put in the Github for future reference.

In such a case I would never exchange compilation time for runtime. The list is defined during development, finite and small and has virtually no changes, so this is correct. I even like other solutions presented , but not for this case. It’s more code, more complex, less readable, less performative, I mean, it’s just supposed to be Clever without producing advantages.

Of course, one can create an abstraction that treats what each state should do, but this usually leaves the code complex, so it’s only worth it if it’s really complex, and this additional complexity serves to help manage the overall complexity.

Note that the control of the state itself must be very simple, what will have to be in specific classes, after all can always add new behaviors that are related to the states, but that are part of a different subsystem. One is used by marketing, another for tax bookkeeping, etc.

  • 1

    +1, ps: I am the only one who positions the break in the alignment of xD cases

  • 1

    Ufa that good. I did so and it goes into production. :)

  • @Guilhermenascimento Yeah, it’s not common, but it’s valid, it even has some sense. What I really wanted was that I didn’t need, I should have a different syntax.

  • 2

    After much the Eclipse insist, I got used to putting the cases at the same level as switch, but it becomes much more readable with this extra level

  • 1

    Also use with the extra level. And the cascade switch, when well used, is a great tool. It is worth remembering that in many languages the switch with a series of if ... else, because the implementation is very different from the two structures.

  • 1

    Just to leave my two cents, I use cascading too and almost always use xD keys

Show 1 more comment

9

Approach with Map

Since your purpose is to set a different color for each region, then:

private static final Map<String, String> regioes = new HashMap<>(27);
private static final Map<String, Color> cores = new HashMap<>(27);

static {
    String[] norte = {"AM", "AP", "AC", "RO", "RR", "PA", "TO"};
    String[] sul = {"PR", "SC", "RS"};
    String[] sudeste = {"SP", "MG", "RJ", "ES"};
    String[] nordeste = {"BA", "SE", "AL", "PE", "PB", "RN", "CE", "PI", "MA"};
    String[] centroOeste = {"MS", "MT", "GO", "DF"};

    for (String s : norte) regioes.put(s, "norte");
    for (String s : sul) regioes.put(s, "sul");
    for (String s : sudeste ) regioes.put(s, "sudeste");
    for (String s : nordeste ) regioes.put(s, "nordeste");
    for (String s : centroOeste ) regioes.put(s, "centro-oeste");

    cores.put("norte", Color.RED);
    cores.put("sul", Color.GREEN);
    cores.put("centro-oeste", Color.YELLOW);
    cores.put("nordeste", Color.BLUE);
    cores.put("sudeste", Color.ORANGE);
}

private static Color corDoEstado(String sigla) {
    String regiao = regioes.get(sigla);
    return cores.get(regiao);
} 

And then in your code, you just have to do this:

algumaCoisa.setCor(corDoEstado(estado));

The advantages of this above approach is that:

  • Mapping is built only once in class loading.
  • The mapping is reusable elsewhere, and you get rid of having to repeat this switch appalling whenever dealing with states.

Object-oriented approach

However, this approach still has its problems. The main problem is that this is all about string-oriented programming, by treating these things as strings and not as objects that should be.

So the ideal is to do something like this:

package com.example;
import java.awt.Color;

public enum RegiaoBrasileira {
    SUL(Color.GREEN),
    SUDESTE(Color.ORANGE),
    CENTRO_OESTE(Color.YELLOW),
    NORTE(Color.RED),
    NORDESTE(Color.BLUE);

    private final Color cor;

    private RegiaoBrasileira(Color cor) {
        this.cor = cor;
    }

    public Color getCor() {
        return cor;
    }

    public static List<EstadoBrasileiro> getEstados() {
        return LocalizaEstados.porRegiao(this);
    }
}
package com.example;
import java.util.Locale;
import static com.example.RegiaoBrasileira.*;
import com.example.util.StringUtils;

public enum EstadoBrasileiro {
    RIO_GRANDE_DO_SUL("RS", SUL),
    SANTA_CATARINA("SC", SUL),
    PARANÁ("PR", SUL),
    SÃO_PAULO("SP", SUDESTE),
    RIO_DE_JANEIRO("RJ", SUDESTE),
    MINAS_GERAIS("MG", SUDESTE),
    ESPÍRITO_SANTO("ES", SUDESTE),
    BAHIA("BA", NORDESTE),
    SERGIPE("SE", NORDESTE),
    ALAGOAS("AL", NORDESTE),
    PERNAMBUCO("PE", NORDESTE),
    PARAÍBA("PB", NORDESTE),
    RIO_GRANDE_DO_NORTE("RN", NORDESTE),
    CEARÁ("CE", NORDESTE),
    PIAUÍ("PI", NORDESTE),
    MARANHÃO("MA", NORDESTE),
    PARÁ("PA", NORTE),
    AMAPÁ("AP", NORTE),
    ACRE("AC", NORTE),
    AMAZONAS("AM", NORTE),
    RONDÔNIA("RO", NORTE),
    RORAIMA("RR", NORTE),
    TOCANTINS("TO", NORTE),
    MATO_GROSSO("MT", CENTRO_OESTE),
    MATO_GROSSO_DO_SUL("MS", CENTRO_OESTE),
    GOIÁS("GO", CENTRO_OESTE),
    DISTRITO_FEDERAL("DF", CENTRO_OESTE),

    private final String nome;
    private final String sigla;
    private final RegiaoBrasileira regiao;

    private EstadoBrasileiro(String sigla, RegiaoBrasileira regiao) {
        this.sigla = sigla;
        this.regiao = regiao;
        this.nome = StringUtils.toTitleCase(name().toLowerCase(Locale.ROOT).replace("_", " "));
    }

    public String getNome() {
        return nome;
    }

    public String getSigla() {
        return sigla;
    }

    public RegiaoBrasileira getRegiao() {
        return regiao;
    }
}
package com.example;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

class LocalizaEstados {

    private static final Map<String, EstadoBrasileiro> SIGLAS_POR_ESTADO = new HashMap<>(27);
    private static final Map<RegiaoBrasileira, List<EstadoBrasileiro>> ESTADO_POR_REGIAO = new HashMap<>(27);

    static {
        for (RegiaoBrasileira s : RegiaoBrasileira.values()) {
            ESTADO_POR_REGIAO.put(s, new ArrayList<>(9));
        }
        for (EstadoBrasileiro s : EstadoBrasileiro.values()) {
            SIGLAS_POR_ESTADO.put(s.sigla, s);
            ESTADO_POR_REGIAO.get(s.getRegiao()).add(s);
        }
        for (RegiaoBrasileira s : RegiaoBrasileira.values()) {
            ESTADO_POR_REGIAO.put(s, Collections.unmodifiableList(ESTADO_POR_REGIAO.get(s));
        }
    }

    private LocalizaEstados() {}

    static EstadoBrasileiro porSigla(String sigla) {
        return SIGLAS_POR_ESTADO.get(sigla);
    }

    static List<EstadoBrasileiro> porRegiao(RegiaoBrasileira regiao) {
        return ESTADO_POR_REGIAO.get(regiao);
    }
}
package com.example.util;

class StringUtils {

    // Fonte: https://stackoverflow.com/a/1086134/540552
    public static String toTitleCase(String input) {
        StringBuilder titleCase = new StringBuilder();
        boolean nextTitleCase = true;

        for (char c : input.toCharArray()) {
            if (Character.isSpaceChar(c)) {
                nextTitleCase = true;
            } else if (nextTitleCase) {
                c = Character.toTitleCase(c);
                nextTitleCase = false;
            }

            titleCase.append(c);
        }

        return titleCase.toString();
    }
}

It may seem like a great job, but:

  • You separate the concepts of Brazilian states and regions from other parts of the code.

  • All this is reusable.

  • If you need to change concepts of Brazilian states and regions (for example, add the concept of state capitals), it is not difficult to change the Enum.

  • Concepts of states, Brazilian regions and corresponding colors no longer pollute other parts of the code.

Completion

Use the switch is an awful thing. It tends to pollute the code leaving it confused and difficult to maintain. Polymorphism and proper data tabulation in suitable data structures are better alternatives to it in most cases.

It’s true that this all seems more complicated than using one switch. The problem is that you rarely end up using only one and yes you end up using two, three, five, twenty times those switches scattered in a lot of code places.

So stick to the case of XY problem.

  • 1

    Your conclusion reminded me of the anti movement-if

  • 3

    @Jeffersonquesado The anti movement-if has similarities with the anti-switch. Entrentanto, while the if is often in fact the best solution (but not always), the same does not occur with the switch, that is almost never the best solution.

  • Wow... : O. Our...scary but I will try to implement yes to better understand your code.

  • There’s something in your code that’s too advanced for me :/

  • 2

    If you have questions about any of the concepts or keywords used in the code, @Aline, don’t be afraid to ask a new question about it, always attentive to the suggestions of duplicates. Many things like "What is Enum", "What is a Map" etc. have already been very well explained here on the site. :)

6

As already answered, is valid and works.

My choice is usually the aesthetic part of the code, in this case your performance is by no means a concern.

Another approach(yes, with much more code to write):

Actionprovider.java:

public interface ActionProvider {

    public void takeAction(String state);

}

Nordesteactionprovider.java:

public class NordesteActionProvider implements ActionProvider {

    public void takeAction(String state) {
        System.out.println("Nordeste taking action");       
    }

}

Sudesteactionprovider.java:

public class SudesteActionProvider implements ActionProvider {

    public void takeAction(String state) {
        System.out.println("Sudeste taking action");        
    }

}

Statestrategy.java:

public enum StateStrategy {

    NORDESTE(new NordesteActionProvider(), Arrays.asList("AL", "BA", "CE", "MA", "PB", "PE", "PI", "RN", "SE")),
    SUDESTE(new SudesteActionProvider(), Arrays.asList("ES", "MG", "RJ", "SP"));

    private List<String> states;
    private ActionProvider provider;

    StateStrategy(ActionProvider strategy, List<String> states) {
        this.states = states;
        this.provider = strategy;
    }

    public static Optional<StateStrategy> strategy(final String state) {
        return Arrays.asList(values())
                .stream()
                .filter(s -> { return s.states.contains(state); })
                .findFirst();               
    }

    public ActionProvider getActionProvider() {
        return provider;
    }

}

Using:

String al = "AL";
StateStrategy strategy = StateStrategy.strategy(al).orElseThrow(() -> new RuntimeException("Estado inválido"));
strategy.getActionProvider().takeAction(al);

String sp = "SP";
strategy = StateStrategy.strategy(sp).orElseThrow(() -> new RuntimeException("Estado inválido"));
strategy.getActionProvider().takeAction(sp);

Obs.: da para melhorar...

  • Thanks for the input, Tom, but it would be more constructive if you explained the concepts employed in your code and why it’s better or worse than the original approach.

Browser other questions tagged

You are not signed in. Login or sign up in order to post.