5
\$\begingroup\$

Introduction

I'm working with Java 1.8. There is a lot of repetition in the MessageHandler class that I would like to remove but I'm having trouble implementing a fix.

External Code

I'm working with a message library that I should avoid changing. This library consists of these files:

BaseMessage.java

package external;

public abstract class BaseMessage {
    public void accept(MessageVisitor visitor) {
        visitor.visit(this);
    }
}

AMessage.java

package external;

public class AMessage extends BaseMessage {
}

BMessage.java

package external;

public class BMessage extends BaseMessage {
}

MessageVisitor.java

package external;

public interface MessageVisitor extends SendVisitor {
    default void visit(BaseMessage msg) {
        this.defaultAction(msg);
    }
}

SendVisitor.java

package external;

public interface SendVisitor {
    void defaultAction(BaseMessage var1);

    default void visit(AMessage msg) {
        this.defaultAction(msg);
    }

    default void visit(BMessage msg) {
        this.defaultAction(msg);
    }
}

Internal Code

Entrypoint.java

package example;

import external.AMessage;
import external.BMessage;
import external.BaseMessage;

class Entrypoint {
    MessageHandler handler;

    Entrypoint(MessageHandler handler) {
       this.handler = handler;
    }

    void handleMessage(BaseMessage message) {
        if(handler != null && handler.supports(message.getClass())) {
            if (handler.hasSeenBefore(message)) {
                // Log that we are skipping handling
                return;
            } else {
                handler.receive(message);
            }
        }

        // and do more unrelated things...
    }

    public static void main(String[] args) {
        Entrypoint entrypoint = new Entrypoint(new MessageHandler());

        entrypoint.handleMessage(new AMessage());
        entrypoint.handleMessage(new BMessage());
    }
}

MessageHandler.java

package example;

import external.AMessage;
import external.BMessage;
import external.BaseMessage;

import java.util.Arrays;
import java.util.HashSet;
import java.util.Set;

public class MessageHandler {
    private static Set<Class<? extends BaseMessage>> SUPPORTED_MESSAGE_TYPES = new HashSet<>(
            Arrays.asList(AMessage.class, BMessage.class));

    public boolean supports(Class<? extends BaseMessage> messageType) {
        // this sucks
        return SUPPORTED_MESSAGE_TYPES.contains(messageType);
    }

    private boolean hasSeenBefore(AMessage message) {
        System.out.println("Checking if I've seen an AMessage");
        // more complicated in the actual code
        return true;
    }

    private boolean hasSeenBefore(BMessage message) {
        System.out.println("Checking if I've seen a BMessage");
        // more complicated in the actual code
        return false;
    }

    public boolean hasSeenBefore(BaseMessage message) {
        // TODO: make better
        if (message instanceof AMessage) {
            return hasSeenBefore((AMessage) message);
        } else if (message instanceof BMessage) {
            return hasSeenBefore((BMessage) message);
        } else {
            throw new UnsupportedOperationException("Tried to handle a message that is currently not supported: "
                    + message.toString());
        }
    }

    private void receive(AMessage message) {
        // do AMessage things
        System.out.println("Received an AMessage");
    }

    private void receive(BMessage message) {
        // do BMessage things
        System.out.println("Received a BMessage");
    }

    public void receive(BaseMessage message) {
        // TODO: make better
        if (message instanceof AMessage) {
            receive((AMessage) message);
        } else if (message instanceof BMessage) {
            receive((BMessage) message);
        } else {
            throw new UnsupportedOperationException("Tried to handle a message that is currently not supported: "
                    + message.toString());
        }
    }
}

Question

As you can see, it's a pain to maintain the MessageHandler; specifically, the hasSeenBefore method has an instanceof that must be added to when another message is added, and a similar thing happens in the receive method. It is also really bad that the supports function keeps a separate list of those message types that must be updated when a new message type is added.

My hunch says that the Visitor pattern should be applied here more, but I'm having trouble implementing that.

  • How can I remove all this duplicated logic so that I could add new message types to the MessageHandler cleanly?
  • And how can supports check whether the message type is handled without maintaining a separate list?
  • Is there anything else in the internal code which is not following best practices?
\$\endgroup\$
7
  • \$\begingroup\$ Can you post the complete code of MessageHandler and messages? It's not clear why you are casting. \$\endgroup\$
    – Marc
    Nov 12, 2020 at 0:43
  • \$\begingroup\$ Messages are casted into their subtypes so that they can be sent off to the subtype-specific handler methods. The messages contain some different fields which are checked in those subtype-specific handler methods. I think that posting the message contents would bloat this question up with details unrelated to my question, they are extremely big actually. \$\endgroup\$
    – karobar
    Nov 12, 2020 at 1:25
  • \$\begingroup\$ I should also mention that I am not authorized to release the full source code. \$\endgroup\$
    – karobar
    Nov 12, 2020 at 1:33
  • 2
    \$\begingroup\$ @karobar Code Review requires concrete code from a project, with enough code and / or context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site. \$\endgroup\$
    – user228914
    Nov 12, 2020 at 4:39
  • 3
    \$\begingroup\$ This is a bit borderline. I understand how this code is used and what the problem in question is and know a suitable answer. I don't think removing unrelated bits turns it into stub, obfuscated or hypothetical code. Making the requirements excessively limiting devalues this site as it will then become only a source for reviewing homework and interview question as practically no production code can be posted. \$\endgroup\$ Nov 12, 2020 at 9:48

1 Answer 1

4
\$\begingroup\$

The main problem I see is that MessageHandler is solely responsible for knowing all the different message types and how they should be handled. If I was implemeting this I would create a separate MessageHandler subtype for each different message type and each subtype would only know about the existence of the specific message it handles. This reduces the dependencies each message handler has and thus makes testing and maintenance much easier. The main guide line here is the single responsibility principle

To handle the selection of a correct MessageHandler in the EntryPoint class I would implement a MessageHandlerRegistry from which the EntryPoint can retrieve the correct handler for a message type by class. Essentially it is a simple facade for Map<Class<? extends BaseMessage>, MessageHandler>. To handle the case of unknown message type I would make the registry return a default handler which simply logs the error and throws an exception. That way the registry never has to return null values.

Populate the MessageHandlerRegistry in your bootstrap code and pass the reference to the EntryPoint class. The EntryPoint contains unnecessary null-checks. It seems obvious that the class can not function properly if it does not have a MessageHandler (or MessageHandlerRegistry in my version) so you should put the null check in the constructor and simply prevent the initialization of the EntryPoint if it's state is invalid. Make the field final so you know it will not be changed to null accidentally.

\$\endgroup\$
2
  • \$\begingroup\$ Hey thanks, this is very helpful! After retrieving a correct MessageHandler, would you send a BaseMessage to it and then cast inside of that MessageHandler, or is there some way to get a correctly-subtyped message to that handler. In other words, registry.get(message.getClass()).receive(???) \$\endgroup\$
    – karobar
    Nov 12, 2020 at 15:26
  • 1
    \$\begingroup\$ For some of the trickier bits I was able to get an implementation which satisfied me with the aid of Joshua Bloch's example of typesafe heterogeneous containers from Effective Java, modified with assistance from this answer: stackoverflow.com/questions/6541656/… \$\endgroup\$
    – karobar
    Nov 12, 2020 at 20:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.