views:

548

answers:

5

I have the following situation where a client class executes different behavior based on the type of message it receives. I'm wondering if there is a better way of doing this since I don't like the instanceof and the if statements.

One thing I thought of doing was pulling the methods out of the client class and putting them into the messages. I would put a method like process() in the IMessage interface and then put the message specific behavior in each of the concrete message types. This would make the client simple because it would just call message.process() rather than checking types. However, the only problem with this is that the behavior contained in the conditionals has to do with operations on data contained within the Client class. Thus, if I did implement a process method in the concrete message classes I would have to pass it the client and I don't know if this really makes sense either.

public class Client {
    messageReceived(IMessage message) {
        if(message instanceof concreteMessageA) {
            concreteMessageA msg = (concreteMessageA)message;
            //do concreteMessageA operations
        }
    }
        if (message instanceof concreteMessageB) {
            concreteMessageb msg = (concreteMessageB)message;
            //do concreteMessageB operations
        }
}
+5  A: 

The simple way to avoid instanceof testing is to dispatch polymorphicly; e.g.

public class Client {
    void messageReceived(IMessage message) {
        message.doOperations(this);
    }
}

where each message class defines an appropriate doOperations(Client client) method.

EDIT: second solution which better matches the requirements.

An alternative that replaces a sequence of 'instanceof' tests with a switch statement is:

public class Client {
    void messageReceived(IMessage message) {
        switch (message.getMessageType()) {
        case TYPE_A:
           // process type A
           break;
        case TYPE_B:
           ...
        }
    }
}

Each IMessage class needs to define an int getMessageType() method to return the appropriate code. Enums work just as well ints, and are more more elegant, IMO.

Stephen C
The 2nd approach might still require a cast if the different *process* blocks required to access properties only available on message subtypes
oxbow_lakes
I don't like the idea of the client defining how the message should be processed. What happens if you need to change doOperations - how do you get this updated code out to all of the clients? I guess a visitor pattern would be better in this case. So that the client visitor is accepted by the message and can sneak around to do what it wants.
pjp
@pgp: erm ... I think you are misunderstanding the problem. The way that the problem is posed, the logic of processing the messages >>is<< specific to the Client class.
Stephen C
I initially thought of using polymorphism and doing exactly what Stephen says in his first code sample, but I thought it might be violating encapsulation by passing the Client to the messages. Also, if I had 5 Clients and each wants to do something different with each message that is 5 different operations in each message one for each of the Clients.
volker238
@volker: passing self does not violate encapsulation. Encapsulation is only violated if the Client exposes state/methods that it shouldn't.
Stephen C
@volker: the "5 Clients" use-case could be addressed by subclassing the Client class, each one overloading the messageReceived method and possibly delegating to `super.messageReceived` to deal with common message types.
Stephen C
+1  A: 

One option here is a handler chain. You have a chain of handlers, each of which can handle a message (if applicable) and then consume it, meaning it won't be passed further down the chain. First you define the Handler interface:

public interface Handler {
    void handle(IMessage msg);
}

And then the handler chain logic looks like:

List<Handler> handlers = //...
for (Handler h : handlers) {
    if (!e.isConsumed()) h.handle(e);
}

Then each handler can decide to handle / consume an event:

public class MessageAHandler implements Handler {
    public void handle(IMessage msg) {
        if (msg instanceof MessageA) {
            //process message
            //consume event 
            msg.consume();
        }
    }
}

Of course, this doesn't get rid of the instanceofs - but it does mean you don't have a huge if-elseif-else-if-instanceof block, which can be unreadable

oxbow_lakes
Readability is is a matter of opinion/taste. Creating new handler classes for each message type means you've got to look somewhere else again (apart from the Client class and the IMessage classes) to find the logic. I wouldn't call that a "big readability win".
Stephen C
Neither would I! It all depends on whether you hate big `if-else-instanceof` blocks. It sounds as if the OP does, so I suggested it. The handler chain *is* useful though, because it is more flexible than if-else. A handler doesn't have to consume an event, so it can be processed by more than one handler
oxbow_lakes
A: 

see this question i posted on handling collections of mixed POJOs, which is identical to your case

A: 

What type of message system are you using?

Many have options to add a filter to the handlers based on message header or content. If this is supported, you simply create a handler with a filter based on message type, then your code is nice and clean without the need for instanceof or checking type (since the messaging system already checked it for you).

I know you can do this in JMS or the OSGi event service.

Since you are using JMS, you can basically do the following to register your listeners. This will create a listener for each unique message type.

  String filterMsg1 = "JMSType='messageType1'";
  String filterMsg2 = "JMSType='messageType2'";

  // Create a receiver using this filter
  Receiver receiverType1 = session.createReceiver(queue, filterMsg1);
  Receiver receiverType2 = session.createReceiver(queue, filterMsg2);

  receiverType1.setMessageHandler(messageType1Handler);
  receiverType2.setMessageHandler(messageType2Handler);

Now each handler will receive the specific message type only (no instanceof or if-then), assuming of course that the sender sets the type via calls to setJMSType() on the outgoing message.

This method is built into message, but you can of course create your own header property and filter on that instead as well.

Robin
I'm using JMS and I'll have to look into your suggestion.
volker238
@volker238 - It is called a selector and you can use it to filter based on message header information.
Robin
@volker238 - I added a code snippet based on the fact that you are using JMS. Assuming you can set header properties, the selector is absolutely the way to go since it allow the JMS implementation to do all the routing for you. No need for ugly handling code you roll yourself.
Robin
A: 
//Message.java

abstract class Message{
     public abstract void doOperations();
 }

//MessageA.java

 class MessageA extends Message{
    public void doOperations(){ 
         //do concreteMessageA operations ;
    }
 }

   //MessageB.java

class MessageB extends Message {
     public void doOperations(){ 
         //do concreteMessageB operations 
     }
}

//MessageExample.java

class MessageExample{
   public static void main(String[] args) {
        doSmth(new MessageA());
    }

    public static void doSmth(Message message) {
       message.doOperations() ;     

     }
}   
Jamshid Asatillayev