Does anyone disagree with the statement: “using sw

Question

I have seen it written in multiple threads/comments on stackoverflow that using switch is just bad OOP style. Personally I disagree with this.

There will be many cases where you cannot add code (i.e. methods) to enum classes you want to switch on because you don't control them, perhaps they are in a 3rd-party jar file. There will be other cases where putting the functionality on the enum itself is a bad idea because it violates some separation-of-concerns considerations, or it is actually a function of something else as well as the enumeration.

Last of all, switches are concise and clear:

boolean investable;
switch (customer.getCategory()) {
    case SUB_PRIME:
    case MID_PRIME:
        investible = customer.getSavingsAccount().getBalance() > 1e6; break;
    case PRIME:
        investible = customer.isCeo(); break;
}

I'm not defending every use of switch and I'm not saying it's always the way to go. But statements like "Switch is a code smell" are just wrong, in my opinion. Does anyone else agree?

Answer 1

Case statements can almost always be replaced with polymorphism

And also

boolean investable = customer.isInvestable();

Since the call to isInvestable is polymorphic, the actual algorithm used to make the call is determined by the type of customer.

I think you are both wrong. What if this is just the "investibility" logic for a customer wishing for a business loan. Perhaps the innvestibility decision of a customer for another product is really quite different, and possibly not based on a "category" but where they live, whether they are married, what job sector they work in?

Also, what if there are new products coming out all the time, each with different investibility decisions and I don't want to be updating my core Customer class every time this happens?

Like I said, I'm not saying switch is always fine - but equally it can be perfectly legitimate. If used well, it can be an extremely clear way of writing application logic.

Answer 2

Switches are a code smell when used in pure OO code. This doesn't mean they are wrong by definition, just that you need to think twice about using them. Be extra careful.

My definition of switch here also includes if-then-else statements that can easily be rewritten as switch statements.

Switches can be a sign that you are not defining behaviour close to the data on which it operates, and are not taking advantage of subtype polymorphism for example.

When using an OO language, you are not forced to program in an OO way. So if you choose to use a more functional or object-based style of programming (e.g. using DTOs that only contain data but no behaviour, as opposed to richer domain models) there is nothing wrong with using switches.

Finally, when writing OO programs, switches come in very handy at the "edge" of your OO model, when something enters your OO model from the non-OO outside world and you need to convert this external entity into an OO notion. You best do this as early as possible. For example: an int from a database can be converted into an object using a switch.

int dbValue = ...;

switch (dbValue)
{
  case 0: return new DogBehaviour();
  case 1: return new CatBehaviour();
  ...
  default: throw new IllegalArgumentException("cannot convert into behaviour:" + dbValue);  
}

EDIT after reading some of the responses.

Customer.isInvestable: great, polymorphism. But now you are tying this logic to the customer and you need a subclass for each type of customer just to implement the different behaviour. Last time I checked, this is not how inheritance should be used. You would want the type of customer to be a property of Customer, or have a function that can decide the type of a customer.

Double dispatch: polymorphism twice. But your visitor class is essentially still a big switch and it has some of the same problems as explained above.

Besides, following the example of the OP, the polymorphism should be on the category of the customer, not on Customer itself.

Switching on a value is fine: ok, but switch statements are in the majority of cases used to test on a single int, char, enum, ... value, as opposed to if-then-else where ranges and more exotic conditions can be tested. But if we dispatch on this single value, and it is not at the edge of our OO model as explained above, then it seems switches are often used to dispatch on type, and not on a value. Or: if you can not replace the conditional logic of an if-then-else by a switch, then you are probably ok, else you are probably not. Therefore I think switches in OOP are code smells, and the statement

Switching on type is bad OOP style, switching on a value is fine.

is itself oversimplified.

And to come back to the starting point: a switch is not bad, it's just not always very OO. You don't have to use OO to solve your problem. If you do use OOP, then switches are something you need to give extra attention.

Answer 3

Working around your libraries is also a code smell. You may not have a choice, but that doesn't make it a good practice.

Answer 4

Data coming in from external sources inherently can't be truly object oriented as you aren't bringing in the code. If it contains cases you're going to have cases. Period.

Beyond that, OOP isn't a silver bullet. There are times it's the answer, there are times it's not.

Answer 5

And now what? Sure, there is a nice OOP way to do it:

str.request(this);

Too bad that String cannot be extended and now you're considering writing a HttpInstruction class with 8 subclasses for each HttpInstruction. Honestly, especially when talking about parsers, it is just substantially difficult.

Ever try C# extension methods? String can be extended.

Answer 6

Robert Martin's article on Open Closed Principle provides another viewpoint:

SOFTWARE ENTITIES (CLASSES, MODULES, FUNCTIONS, ETC.) SHOULD BE OPEN FOR EXTENSION, BUT CLOSED FOR MODIFICATION.

In your code example, you are effectively switching on the customer 'Category Type'

boolean investible ;
switch (customer.getCategory()) {
    case SUB_PRIME:
    case MID_PRIME:
        investible = customer.getSavingsAccount().getBalance() > 1e6; break;
    case PRIME:
        investible = customer.isCeo(); break;
}

In this current climate, new customer categories might be springing up ;-). This means having to open this class, and continually modify it. It might be OK if you only have a single switch statement, but what happens if you want to use similar logic elsewhere.

Rather than other suggestions, where isInvestible is made a method on Customer, I would say that Cagtegory should become a fully-fledged class, and used for making these decisions:

boolean investible ;
CustomerCategory category = customer.getCategory();
investible = category.isInvestible(customer);

class PrimeCustomerCategory extends CustomerCategory {
    public boolean isInvestible(Customer customer) {
        return customer.isCeo();
    }
}