Large Switch Statements: Bad Oop

Large Switch statements: Bad OOP?

You may get some benefit out of a Command Pattern.

For OOP, you may be able to collapse several similar commands each into a single class, if the behavior variations are small enough, to avoid a complete class explosion (yeah, I can hear the OOP gurus shrieking about that already). However, if the system is already OOP, and each of the 100+ commands is truly unique, then just make them unique classes and take advantage of inheritance to consolidate the common stuff.

If the system is not OOP, then I wouldn't add OOP just for this... you can easily use the Command Pattern with a simple dictionary lookup and function pointers, or even dynamically generated function calls based on the command name, depending on the language. Then you can just group logically associated functions into libraries that represent a collection of similar commands to achieve manageable separation. I don't know if there's a good term for this kind of implementation... I always think of it as a "dispatcher" style, based on the MVC-approach to handling URLs.

Switch statements are bad?

A switch is like any other control structure. There are places where it's the best/cleanest solution, and many more places where it's completely inappropriate. It's just abused way more than other control structures.

In OO design, it's generally considered preferable in a situation like yours to use different message types/classes that inherit from a common message class, then use overloaded methods to "automatically" differentiate between the different types.

In a case like yours, you could use an enumeration that maps to your action codes, then attach an attribute to each enumerated value that will let you use generics or type-building to build different Action sub-class objects so that the overloading method will work.

But that's a real pain.

Evaluate whether there's a design option such as the enumeration that is feasible in your solution. If not, just use the switch.

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

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.

Design pattern for a large nested switch statements

I think it depends what kind of code improvement you're trying to do.

If you have the luxury of actually making big design changes, then I'd suggest polymorphism:

Create an abstract Packet class.

Create a class for each packet type.

Create a factory method, that receives a raw server packet, and creates the right packet class object.

Each packet class type will have its own implementation of the job it needs to do.

If you don't have the luxury of doing large design changes (which is often the case):

• If you want to improve readability :

Keep the switch, each switch case will call a properly named function that will do what it needs to.

• If you want to increase performance:

Create a matrix, that for each cell [T,C] will hold a reference to a function that will handle a Packet with Type T and Code C.

The matrix should be initiated once (hard-coded, no way around that) at startup of program or class.

This will give you better performance than a long switch block (direct access to code, no logical comparisons)

How to refactor this huge switch statement?

Since your question includes no code, the answer can't really either. I think the best thing to do is to point you to page 82 of one of the all-time best software books: Refactoring: Improving the Design of Existing Code.

"One of the most obvious symptoms of object-oriented code is its comparative lack of switch statements. Most times you see a switch statement you should consider polymorphism."

He then lists some of the specific patterns to be used to help make this happen.

Ways to eliminate switch in code

Switch-statements are not an antipattern per se, but if you're coding object oriented you should consider if the use of a switch is better solved with polymorphism instead of using a switch statement.

With polymorphism, this:

foreach (var animal in zoo) {
    switch (typeof(animal)) {
        case "dog":
            echo animal.bark();
            break;

        case "cat":
            echo animal.meow();
            break;
    }
}

becomes this:

foreach (var animal in zoo) {
    echo animal.speak();
}

Large switch statement or scatter with a virtual void

In 99% of scenarios I recommend maintainability over efficiency. Given that this is C# I will assume that we don't have some hard deadline based loop times or some other mission critical requirement that is usually associated with lower level languages or real time embedded systems.

I will say that a 25,000 line file is un-maintainable and needs to be decomposed. Whether that means to decompose it into a set of virtual classes or use some more traditional decomposition such as trying to abstract pieces with inheritance I could not give you a recommendation without more insight.

(However I usually lean on using inheritance with abstract classes. I rarely find a good use case for virtualization outside of extending generated code)

To answer the initial question, scattering with virtual void is a better solution than a single large switch statement.

Best of luck!

C# shorthand switch statement

You can't get much shorter, but you can use a switch expression introduced in C# 8.0, which may save you a couple of printable characters:

public string SomeMethod(int type)
    => type switch
    {
       2 => "SELF DECOUPLING",
       3 => "AUTO",
       4 => "SEMI PNEUMATIC",
       _ => "MANUAL"
    };

Or an if then else:

if (type == 2) Type = "SELF DECOUPLING";
else if (type == 3) Type = "AUTO";
else if (type == 4) Type = "SEMI PNEUMATIC";
else Type = "MANUAL";

---

Related Topics