Does anyone disagree with the statement: "using switch is bad OOP style"?

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

It's bad OOP style.

Not all problems are best solved with OO. Some you want pattern matching, which switch is the poor man's version of.

Answer 2

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 3

Yes, I'm fed up with people telling you it's bad style.

Edit: This made more sense before the question was fixed.

Answer 4

Case statements can almost always be replaced with polymorphism.

public class NormalCustomer extends Customer {
    public boolean isInvestible() {
        return getSavingsAccount().getBalance() > 1e6;
    }
}

public class PreferredCustomer extends Customer {
    public boolean isInvestible() {
        return isCeo();
    }
}

This approach will simplify the client code. The client code doesn't have to know the details of how "investibility" is calculated and it no longer has to break the Law of Demeter by digging into the state of the Customer object.

Answer 5

I find nothing wrong with using the switch statement in OO code. My only criticism is that I would have made a new method on Customer called IsInvestible which hid this logic. There is 0 wrong with using an switch statement as the internal implementation of this method. As you've said you can't add methods to enum's but you can add more methods to Customer.

In the case you don't have access to the source, I'd say a non-instnance method is fine. An OOP purest would require a brand new object but that seems like it may be overkill in this case.

Answer 6

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 7

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 8

Î know where you are coming from. Some languages force you to do this.

String str = getStr();
switch(str) {
case "POST" : this.doPost(); break;
case "GET" : this.doGet(); break;
//and the other http instructions
}

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.

It isn't good OOP, that for sure, but good code is not always ... possible.

Let me disgress for a moment. I'm writing my thesis. I personally don't like the usual setup of recursive functions. You normally have like funcRec(arg1,arg), and func(arg1):=func(funcRec(arg1,0));

so I defined it in my thesis with default arguments. Not everybody knows the concept of a default argument. My thesis uses pseudo-code, but the professor had me change the algorithm to the traditional way, because you don't come across default arguments very often, so don't use them. Don't surprise your reader unnecessarily. I think he's right.

But the result is that now I'm stuck with a function whose sole purpose is to ship around default arguments - which could be prettier.

So, the bottom line is: Truely beautiful programs require excellent libraries, excellent code browsers and tools, bugtrackers of the qualitiy of FogBugz, at least, better more integrated, version management of git quality, and so forth. And, umm, people around you who can use all these things and know how to handle all of these things. And most of all: a beaauuuuutiful language that allows elegant solutions to tricky problems.

So, chances are, you're stuck with Java which makes it difficult to come up with a good replacement for switches in all situations. Self would have an elegant solution. But you're not using Self, and if you were, your colleagues wouldn't be able to read it, so forget that.

And now find a compromise.

It's sad, I know.

Answer 9

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 10

There are cases when you need to make a decision based on several options and polymorphism is overkill (YAGNI). In this case, switch is fine. Switch is just a tool and can be used or abused just as easily as any other tool.

It depends on what you're trying to do. The point is, however, that you should think twice when using switch as it may be an indication of a bad design.

Answer 11

If anything, I'm fed up with people describing this style of programming - in which a bunch of getters are added to the "low hanging" types (Customer, Account, Bank) and the useful code is sprayed around the system in "controllers", "helpers" and "utility" classes - as object orientated. Code like this is a smell in an OO system, and you should be asking why instead of getting offended.

Answer 12

Taking your followup:

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 ... 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?

and one of your comments:

I'm not entirely sure about holding logic close to the data on which it operates. The real world doesn't work like this. When I ask for a loan, the bank decides whether I qualify. They don't ask me to decide for myself.

you are right, as far as this goes.

boolean investable = customer.isInvestable();

is not the optimal solution for the flexibility you're talking about. However, the original question didn't mention the existence of a separate Product base class.

Given the additional information now available, the best solution would appear to be

boolean investable = product.isInvestable(customer);

The investability decision is made (polymorphically!) by the Product, in accordance with your "real world" argument and it also avoids having to make new customer subclasses each time you add a product. The Product can use whatever methods it wants to make that determination based on the Customer's public interface. I'd still question whether there are appropriate additions which could be made to Customer's interface to eliminate the need for switch, but it may still be the least of all evils.

In the particular example provided, though, I'd be tempted to do something like:

if (customer.getCategory() < PRIME) {
    investable = customer.getSavingsAccount().getBalance() > 1e6;
} else {
    investable = customer.isCeo();
}

I find this cleaner and clearer than listing off every possible category in a switch, I suspect it's more likely to reflect the "real world" thought processes ("are they below prime?" vs. "are they sub-prime or mid-prime?"), and it avoids having to revisit this code if a SUPER_PRIME designation is added at some point.

Answer 13

I think statements like

Using a switch statement is bad OOP style.

and

Case statements can almost always be replaced with polymorphism.

are oversimplifying. The truth is that case statements that are switching on type are bad OOP style. These are the ones you want to replace with polymorphism. Switching on a value is fine.

Answer 14

Sure switches are poor OO, you shouldn't put a return in the middle of a function, magic values are bad, references should never be null, conditional statements must go in {braces}, but these are guidelines. They shouldn't be followed religiously. Maintainability, refactorability, and understandability are all very important, but all second to actually getting the job done. Sometimes we don't have time to be a programming idealist.

If any programmer is to be deemed competent, it should be assumed he can follow guidelines and use the tools available with discretion and it should be accepted that he will not always make the best decision. He may choose a less-than-optimal route or make a mistake and run into a hard-to-debug problem because he chose a switch when maybe he shouldn't have or passed around too many null pointers. That's life, and he learns from the mistake, because he is competent.

I don't religiously follow programming dogma. I consider guidelines in the context of myself as a programmer and apply them as seems reasonable. We shouldn't harp on these sorts of programming practices unless they are fundamental to the problem at hand. If you want to assert your opinion on good programming practices, it's best to do so in a blog or an appropriate forum (such as right here).

Answer 15

I view switch statements as a more readable alternative to if/else blocks.

I find that if you can boil down your logic to a structure that can be evaluated integrally, the code is likely to be providing a level of encapsulation that is required in OOP.

At some point real (messy) logic has to be written for a practical program to ship. Java and C# are not strictly OOP languages, given that they inherit from C. If you want to enforce strictly OOP code, then you'll need to use a language that does not provide idioms which violate that mindset. My view is that both Java and C# are intended to be flexible.

One of the things that made VB6 so successful, oddly enough, is that it was Object-based, not Object-oriented. So, I would say that pragmattic programmers will invariably combine concepts. Switch can also lead to more manageable code, as long as there is decent encapsulation already programmed in.

Answer 16

I do believe switching on type is a code smell. However I share your concerns about separation-of-concerns in code. But those can be solved in many ways that allow you to still use polymorphism, e.g. the visitor pattern or something similar. Read up on "Design Patterns" by the Gang of Four.

If your core objects like Customer stays fixed most of the time but operations change often, then you can define operations as objects.

    interface Operation {
      void handlePrimeCustomer(PrimeCustomer customer);
      void  handleMidPrimeCustomer(MidPrimeCustomer customer);
      void  handleSubPrimeCustomer(SubPrimeCustomer customer);    
    };

    class InvestibleOperation : public Operation {
      void  handlePrimeCustomer(PrimeCustomer customer) {
        bool investible = customer.isCeo();
      }

      void  handleMidPrimeCustomer(MidPrimeCustomer customer) {
        handleSubPrimeCustomer(customer);
      }

      void  handleSubPrimeCustomer(SubPrimeCustomer customer) {
        bool investible = customer.getSavingsAccount().getBalance() > 1e6;    
      }
    };

    class SubPrimeCustomer : public Customer {
      void  doOperation(Operation op) {
        op.handleSubPrimeCustomer(this);
      }
    };

   class PrimeCustomer : public Customer {
      void  doOperation(Operation op) {
        op.handlePrimeCustomer(this);
      }
    };

This looks like overkill, but it can easily save you a lot of coding when you need to handle operations as collections. E.g. display all of them in a list and let user select one. If operations are defined as functions you easily end up with a lot of hard coded switch-case logic, multiple places which needs to be update each time you add another operation, or product as I see it referred to here.

Answer 17

"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?"

This springs to mind:

interface Investable
{
    boolean isIvestible(Customer c);
}

class FooInvestible 
    implements Investible
{
    public boolean isInvestible(final Customer c)
    {
        // whatever logic, be it switch or other things
    }
}

The "problem" with the original use of swtich and adding new types of decisions is that you likely wind up with some huge rats nest of code that is impossible to maintain in a sane way. Splitting the decisions up into classes forces the decision making to be split up. Then, even if you use switch, the code is likely to stay saner and maintainable.

Answer 18

I think whether a switch statement is poor OOP practice depends on where you are using the switch statement.

In a factory method for instance, it might be a very simple alternative to writing complicated and potentially buggy reflection-based code.

In most circumstances though, I think switches are just simply design. Quite often hiding operational complexity in different objects with the same method leads to more understandable and possibly even faster code. For instance, if you have a switch that executes lot then having pre-packaged objects could actually save some CPU cycles.

Answer 19

My issue with switch statements is that in real world application there are rarely switch statements that exist in isolation.

A lot of the code that required refactoring in my company's codebase would have entire classes riddled with multiple switch statements throughout such that you had to know of the existence of every single switch statement.

Ultimately, the cleanest refactoring the entire system into a Strategy pattern with a Factory controlling the creation of the strategies based on the single remaining copy of the switch statement.

Due to time constraints we didn't take it any farther because this served our needs. There was still a big giant switch statement, but there was only one, so adding additional strategies only required ipmlementing the interface and adding the creation step to the master switch statement.

Answer 20

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 21

First, your goal should not be to achieve "good OO style" but good code. And "good" means at least correct, clear, readable and as simple as possible.

So I will reformulate the quesion to: "Is using switch a sign of bad code?", because that's really what I care about. Now I'll proceed to answer it.

Hm, that's a good question :) Usually, using a switch once is not a sign of bad code. But if you switch on the same thing at several points in your class, it's good to consider an alternative design in which you represent the switch alternatives with child classes - and when you consider that, ask yourself especially if classes created like that would be a specialization of the current class and would have an is-a relationship. If so, this gives more points to using inheritance.

One last comment: "Using [language feature X] at all is bad" is dangerously close to "The language designers were stupid to include [language feature X] in it".

Answer 22

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