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?

Answer 1

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.

Answer 2

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