Clean code for removing switch condition(using polymorphism)

Question

As SOLID principles say, it's better to remove switch conditions by converting them to classes and interfaces. I want to do it with this code:

Note: This code is not real code and I just put my idea into it.

MessageModel message = getMessageFromAnAPI();
manageMessage(message);
...
void manageMessage(MessageModel message){        
    switch(message.typeId) {
        case 1: justSave(message); break;
        case 2: notifyAll(message); break;
        case 3: notify(message); break;
    }
}

Now I want to remove switch statement. So I create some classes for it and I try to implement a polymorphism here:

interface Message{
    void manageMessage(MessageModel message);
}
class StorableMessage implements Message{

    @Override
    public void manageMessage(MessageModel message) {
        justSave(message);
    }
}
class PublicMessage implements Message{

    @Override
    public void manageMessage(MessageModel message) {
        notifyAll(message);
    }
}
class PrivateMessage implements Message{

    @Override
    public void manageMessage(MessageModel message) {
        notify(message);
    }
}

and then I call my API to get my MessageModel:

MessageModel message = getMessageFromAnAPI();

Now my problem is here. I have my model and I want manage it using my classes. As SOLID examples, I should do something like this:

PublicMessage message = new Message();
message.manageMessage(message);

But how can I know which type is related to this message to make an instance from it(PublicMessage or StorableMessage or PrivateMessage)?! Should I put switch block here again to do it or what?

Gobbet

To throw in some thoughts. Is it really clean? Or just inflated by classes and interfaces, have your action split up over x classes instead of one simple switch? This becomes incredibly hard to follow. So it could be a surprising end, if you start thinking about how clean this really is. – Microfilm

@Microfilm As I mentioned in my question, this is just a template of a regular problem and is not a real code. In the real world, this can be more complex and should split into several classes. – Gobbet

Even if it is just a template, one may think a bit forward about the relation between justSave/notify/notifyAll. If you want to save PublicMessage and PrivateMessage too, they will need to access the functionality for justSave, which is either moved to a separate class, StorableMessage and became tricky to access, or they still reside in a common Manager class, which makes the existence of StorableMessage a bit questionable. The same for notifyAll, which likely does the very same thing as notify, in a loop. These classes exist just to exist – Scopula

You are doing the mistake of being dogmatic where pragmatic suffices. The end result you want to achieve is readable code. The code you display is fine, and you don't achieve much in the way of adding factories or other ways of moving the switch logic. Adding 2 classes without gaining readability is not a win. Where this SOLID principle comes well into play is when you have the same branching logic repeated. That is your call to think "Hmm... replace conditional by polymorphism" refactoring. Then it makes total sense to introduce a supclass with 2+ methods. – Alphabetize

Don't use principles just to be able to use principles. Understand when they are called for and when it doesn't apply. There is always a COST/BENEFIT to any refactoring. – Alphabetize

Answer 1

You can use a factory in this case to get the instance of Message. The factory would have all instances of Message and returns the appropriate one based on the MessageModel's typeId.

class MessageFactory {
    private StorableMessage storableMessage;
    private PrivateMessage privateMessage;
    private PublicMessage publicMessage;
    //You can either create the above using new operator or inject it using some Dependency injection framework.

    public getMessage(MessageModel message) {
        switch(message.typeId) {
            case 1: return storableMessage; 
            case 2: return publicMessage;
            case 3: return privateMessage
            default: //Handle appropriately
        }
    }
}

The calling code would look like

MessageFactory messageFactory; //Injected 
...
MessageModel messageModel = getMessageFromAnAPI();

Message message = messageFactory.getMessage(messageModel);
message.manageMessage(messageModel);

As you can see, this did not get rid of the switch entirely (and you need not as using switch is not bad in itself). What SOLID tries to say is to keep your code clean by following SRP (Single Responsibility Principle) and OCP (Open-Closed Principle) here. What it means here is that you code shouldn't have the actual processing logic to handle for each typeId in one place.

With the factory, you have moved the creation logic to a separate place and you have already moved the actual processing logic to respective classes.

EDIT: Just to reiterate - My answer focuses on the SOLID aspect of the OP. By having separate handler classes (an instance of Message from the OP) you achieve the SRP. If one of the handler classes changes, or when you add a new message typeId (message.typeId) (i.e, add a new Message implementation) you need not modify the original and hence you achieve OCP. (On assumption that each of these does not contain trivial code). These are already done in the OP.

The real point of my answer here is to use a Factory to get a Message. The idea is to keep the main application code clean and limit the usages of switches, if/else and new operators to instantiation code. (Similar to @Configuration classes/ the classes that instantiate Beans when using Spring or Abstract modules in Guice). The OO principles do not say using switches are bad. It depends on where you use it. Using it in the application code does violate the SOLID principles and that is what I wanted to bring out.

I also like the idea from daniu@ to use a functional way and the same can even be used in the above factory code (or can even use a simple Map to get rid of the switch).

Tortuous

Yes I think so, but I want to ensure this is the best solution for this type of regular problem – Gobbet

The author wants to get rid of the switch-case statement and dont want to capsulate it elsewhere i think .. :x – Mintamintage

@Mintamintage Yes. But my point is there is no need to get rid of it and doing this way is fine (and that's what factories are for). You can do something like what daniu@ has done. But it just hides it in a different way and you are not going to get any additional (maintenance or extensibility) benefit. – Tortuous

@user7 have a look on my response. There you dont have to enhance the switch-case statement when you are adding new functionality. You just need to enhance the enum where you can get all Managers. In this, you can get rid of the switch-case and in my opinion express the functionality in a clean way. :) – Mintamintage

A 3-way switch became a 3-way switch and 4 classes, every single one of them having a misleading name on top of that (ok, they originate from OP's question, but still). The 'f-word' of Java could not stay away either. – Scopula

@Scopula So you're counting code complexity by the number of classes? Well one approach. A more sensible one would be to count the complexity inside those classes: Having 3 really simple classes and a class that decides which one to use instead of one that contains the code of those 3 plus the switching code would be a plus by most code analysis tools for obvious reasons. – Antacid

@Voo, I have another comment related to that below the question itself. Those "3 really simple classes" are likely going to either intertwine in a really bad way or rely on a single big class providing all of their functionality, and which exists already. By the way, code analysis tools do not maintain/develop code, people do. – Scopula

Answer 2

You can do this:

static final Map<Integer,Consumer<MessageModel>> handlers = new HashMap<>();
static {
    handlers.put(1, m -> justSave(m));
    handlers.put(2, m -> notifyAll(m));
    handlers.put(3, m -> notify(m));
}

This will remove your switch to

Consumer<Message> consumer = handlers.get(message.typeId);
if (consumer != null) { consumer.accept(message); }

Integration Operation Segregation Principle

You should of course encapsulate this:

class MessageHandlingService implements Consumer<MessageModel> {
    static final Map<Integer,Consumer<MessageModel>> handlers = new HashMap<>();
    static {
        handlers.put(1, m -> justSave(m));
        handlers.put(2, m -> notifyAll(m));
        handlers.put(3, m -> notify(m));
    }
    public void accept(MessageModel message) {
        Consumer<Message> consumer = handlers.getOrDefault(message.typeId, 
                m -> throw new MessageNotSupportedException());
        consumer.accept(message);
    }
}

with your client code

message = getMessageFromApi();
messageHandlingService.accept(message);

This service is the "integration" part (as opposed to the "implementation": cfg Integration Operation Segregation Principle).

With a CDI framework

For a production environment with a CDI framework, this would look something like this:

interface MessageHandler extends Consumer<MessageModel> {}
@Component
class MessageHandlingService implements MessageHandler {
    Map<Integer,MessageHandler> handlers = new ConcurrentHashMap<>();

    @Autowired
    private SavingService saveService;
    @Autowired
    private NotificationService notificationService;

    @PostConstruct
    public void init() {
        handlers.put(1, saveService::save);
        handlers.put(2, notificationService::notifyAll);
        handlers.put(3, notificationService::notify);
    }

    public void accept(MessageModel m) {  // as above }
}

Behavior can be changed at Runtime

One of the advantages of this vs the switch in @user7's answer is that the behavior can be adjusted at runtime. You can imagine methods like

public MessageHandler setMessageHandler(Integer id, MessageHandler newHandler);

which would install the given MessageHandler and return the old one; this would allow you to add Decorators, for example.

An example for this being useful is if you have an unreliable web service supplying the handling; if it is accessible, it can be installed as a handlelr; otherwise, a default handler is used.

Grati

This is a cute solution for removing the switch. But I think it's not a clean code. This problem is a regular problem I I'm looking for best solution – Gobbet

@SiamakFerdos: Why do you think it isn't clean? – Isthmus

@EricDuminil One of the important parts of the clean code is readability. I think that my problem is a regular and simple problem which can have an easy to understand code. Using Map and HashTable is more suitable for some complex problems. So I found using factory more suitable and general. Ofcourse, may be I do mistake! – Gobbet

@SiamakFerdos I update the answer to add a service hiding the actual Map/Consumer thing. – Grati

Much cleaner than other answers. If using Java8 it could even use method references too, eg. handlers.put(1, MessageHandlingService::justSave) etc. – Hemi

A factory is more general, but generality isn't needed here. A factory also adds a lot more LOC and classes than this solution. – Hemi

Just rename your MessageHandlingService to MessageFactory. That will go better with the spirit of this entire topic (useless names, and OP's hardon related to the f-word) – Scopula

@NathanAdams It's bound to be Java8, or I wouldn't be using lambdas ;) Anyway, these lambdas really are just placeholders; in practice I'd expect this to be delegated to other services, so more likely to be something like (1, saveService::save), (2, notifyService::notifyAll), (3, notifyService::notify). – Grati

@Siamak Why would you think that one of the most basic datastructures out there is not for simple problems? Seems perfectly readable and simple. – Antacid

@Antacid For simple problem the OP code makes sense. But the question here is what if those individual methods are complex and more typeIds are added in future (OCP) – Tortuous

@user7 I expanded the answer. As you see, it is in fact far easier to add more typeIds in this solution. – Grati

Answer 3

The main point here is that you separate instantiation and configuration from execution.

Even with OOP we cannot avoid to distinguish between different cases using if/else cascades or switch statements. After all we have to create instances of specialized concrete classes.
But this should be in initialization code or some kind of factory.

Within the business logic we want to avoid if/else cascades or switch statements by calling generic methods on interfaces where the implementer know better themselves how to behave.

Scare

Yes, I think this is the best solution to do it – Gobbet

Answer 4

The usual clean code approach is for the MessageModel to contain its behavior.

interface Message {
    void manage();
}

abstract class MessageModel implements Message {
}

public class StoringMessage extends MessageModel {
    public void manage() {
        store();
    }
}
public class NotifyingMessage extends MessageModel {
    public void manage() {
        notify();
    }
}

Your getMessageFromApi then returns the proper type, and your switch is

MessageModel model = getMessageFromApi();
model.manage();

This way, you essentially have the switch in the getMessageFromApi() method because it has to decide which message to generate.

However, that is fine because it does fill the message type id anyway; and the client code (where your switch currently resides) is resistent to changes to the messages; ie adding another message type will be be handled correctly.

Grati

Answer 5

The real problem you have is that MessageModel isn't polymorphic. You need to convert the MessageModels to a polymorphic Message class, but you shouldn't put any of the logic of what to do with the messages in this class. Instead, it should contain the actual contents of the message, and use the visitor pattern, as shown in Eric's Answer, so that other classes can operate on a Message. You don't need to use an anonymous Visitor; you can create implementing classes like a MessageActionVisitor.

To convert MessageModels to various Messages, you can use a factory, as shown in user7's answer. In addition to selecting which type of Message to return, the factory should fill in the fields of each type of Message using the MessageModel.

Fiftieth

Answer 6

You can use the Factory Pattern. I would add an enum which has the values:

public enum MessageFacotry{
    STORING(StoringMessage.TYPE, StoringMessage.class),
    PUBLIC_MESSAGE(PublicMessage.TYPE, PublicMessage.class),
    PRIVATE_MESSAGE(PrivateMessage.TYPE, PrivateMessage.class);
    Class<? extends Message> clazz;
    int type;
    private MessageFactory(int type, Class<? extends Message> clazz){
        this.clazz = clazz;
        this.type = type;
    }

    public static Message getMessageByType(int type){

         for(MessageFactory mf : values()){
              if(mf.type == type){
                   return mf.clazz.newInstance();
              }
         }
         throw new ..
    }
}

Then you can call the static method of that enum and create an instance of the Message you want to manage.

Mintamintage

I don't get how this is better especially when you are using clazz.newInstance(). Why create a new instance each time? what if some dependency has to be injected in that?... It only comes to adding a switch case in a factory class (and not in application code) vs Adding enum. – Tortuous

If you dont want to create a new instance each time, you can just create the instance in the constructor-call of the enum-init-values. then just return the instances instead of class. This is not about implementation details i think, this is about concepts and possibillities. :) All answers are great and show different approaches to the same problem. I voted for yours as well :P ^^ – Mintamintage

Sure. That's not my concern. Dependency injection and enum do not look that good (my opinion). What I wanted to say is SOLID principles do not say to eliminate switch/if,else at all. It just strives to keep the application code clean. – Tortuous

Answer 7

You can use the Factory pattern and Visitor pattern together.

you can create a factory like this:

class MessageFactory {
    public Message getMessage(MessageModel message) {
        switch(message.typeId) {
            case 1: return new StorableMessage((MessageModelType1) message);
            case 2: return new PrivateMessage((MessageModelType2) message);
            case 3: return new PublicMessage((MessageModelType3) message);
            default: throw new IllegalArgumentException("unhandled message type");
        }
    }
}

and declare your messages like this:

interface Message {
    void accept(Visitor visitor);
}

class StorableMessage implements Message {

    private final MessageType1 message;

    public StorableMessage(MessageModelType1 message) {
        this.message = message;
    }

    @Override
    public <Result> Result accept(Visitor<Result> visitor) {
        return visitor.visit(this);
    }

    public MessageModelType1 getMessage() {
        return message;
    }
}

class PublicMessage implements Message {
    ...
}

class PrivateMessage implements Message {
    ...
}

and declare a Visitor like this:

interface Visitor {
    void visit(StorableMessage message);
    void visit(PublicMessage message);
    void visit(PrivateMessage message);
}

and replace your switch statements with this:

Message message = ....;

message.accept(new Visitor() {
    @Override
    public void visit(StorableMessage message) {
        justSave(message.getMessage());
    }

    @Override
    public void visit(PublicMessage message) {
        notifyAll(message.getMessage());
    }

    @Override
    public void visit(PrivateMessage message) {
        notify(message.getMessage());
    }
});

If you want, instead of writing an anonymous class, you can create a class MessageModelFactory that has a private Visitor, and use that instead. in that case, it might be better to make the Visitor interface like this:

interface Visitor<Result> {
    Result visit(StorableMessage message);
    Result visit(PublicMessage message);
    Result visit(PrivateMessage message);
}

Benares

This doesn't answer "how can I know which type is related to this message to make an instance from it?" – Fiftieth

The visitor pattern would work if MessageModel accepted visitors, but since message model stores it's type in an integer, not in its class type, a switch statement (or if else ladder) is needed somewhere. – Fiftieth

oh i see... so the problem is that the MessageModel is from a 3rd party library, and we can't modify it to accept visitors... :/ fair enough. – Benares

I think your answer reveals the real problem though: trying to operate directly on the MessageModel. Really, the MessageModel should be converted to a polymorphic Message by use of a factory or constructor with a switch statement. Then, instead of the Message class having the logic, the visitor pattern should be used on Messages, like you show. – Fiftieth

@Fiftieth ah, yes, i agree. that sounds sensible. – Benares