3
\$\begingroup\$

I have a system that receives messages. Each message has a type defined. These types are declared in an enum:

enum MessageType {
  TYPE1, TYPE2, ... , TYPE999
}

People can extend my system by implementing message processors. Each message type can only have one processor and most will have none. My current design has users writing their processors and I have one factory that they have to change:

public static Map<MessageType, MessageProcessor> getProcessors() {
  Map<MessageType, MessageProcessor> processors = Maps.newHashMap();
  processors.put(TYPE33,  new Type33Processor());
  processors.put(TYPE111, new Type111Processor());
  processors.put(TYPE222, new Type222Processor());
  processors.put(TYPE444, new Type444Processor());
  return processors;
}

My problem with this is twofold:

  1. Each new processor has to remember to add this boilerplate.
  2. This factory has to know all processor types.

I thought of maybe solving this using reflection:

public static Map<MessageType, MessageProcessor> getProcessors() {
  Map<MessageType, MessageProcessor> processors = Maps.newHashMap();
  Reflections reflections = new Reflections(getClass().getPackage().getName());
  Set<Class<? extends MessageProcessor>> allProcessors = 
      reflections.getSubTypesOf(MessageProcessor.class);
  for (Class<? extends MessageProcessor> processorClass : allProcessors) {
    MessageProcessor processor = processorClass.getConstructor().newInstance();
    processors.put(processor.getMessageType(), processor);
  }
  return processors;
}

This solution solves the two issues I mentioned above. My questions are:

  1. I understand that reflection has dangers and pitfalls. Is this a valid scenario to use it?
  2. Is there a better way to improve my first solution without having to resort to using reflection?
\$\endgroup\$
4
  • \$\begingroup\$ Since you have MessageProcessor.getMessageType(), why can't you construct the Map by accepting a Collection<MessageProcessor> for getProcessors()? \$\endgroup\$
    – h.j.k.
    Commented May 19, 2015 at 14:27
  • \$\begingroup\$ Who would create this Collection<MessageProcessor> and how? \$\endgroup\$ Commented May 19, 2015 at 14:28
  • \$\begingroup\$ The users...? They could call a setter with the Collection, which creates the Map. In other words, replace hard-coding the method body with a method call that is called with the desired MessageProcessors. \$\endgroup\$
    – h.j.k.
    Commented May 19, 2015 at 14:34
  • \$\begingroup\$ How would I call this method? I would need to create a Collection of MessageProcessors. Who would populate this collection? \$\endgroup\$ Commented May 19, 2015 at 14:47

2 Answers 2

4
\$\begingroup\$

I understand your concerns here. This is a messy situation, and Java (like most languages, I believe), makes it relatively hard to build up interdependencies during the bootstrapping of the classes, classloaders, etc.

What would be ideal is if you could have an Enum that allowed the following:

MessageProcessor processor = message.getMessageType().getProcessor();

but, that would require the MessageType to be constructed knowing what it's processor is.... but you need to map that on a case-by-case basis, based on the factory that supplies them.

What is certainly not ideal, is having public static methods that create new Maps each time they are called....

public static Map<MessageType, MessageProcessor> getProcessors() { ...

Why does it need to be public... what happens if it is called multiple times?

That's an ugly solution.

The ideal solution would be to construct the MessageProcessor at the same time as the MessageType, but, since that can't be done (as the MessageType is a parameter to the factory), I would instead recommend that you create a synchronized, or locked, or atomic method that is part of the Enum, and handles the situation for you. If your factory has the method:

public MessageProcessor getMessageProcessorFor(MessageType mType) {
    .....
}

then your enum could have a private setup like;

enum MessageType {
    TYPE1, TYPE2, ... , TYPE999;

    private final AtomicReference<MessageProcessor> processor = new AtomicReference<>();

    public MessageProcessor getProcessor() {
        MessageProcessor proc = processor.get();
        if (proc != null) { 
            return proc;
        }
        proc = FactoryClass.getmessageProcessorFor(this);
        if (processor.compareAndSet(null, proc)) {
            return proc;
        }
        // there was a race condition, we lost, so use the winner
        return processor.get();
    }
}

This is a compromize I use quite often. It gives you an abstraction between the class-load initialization, and the runtime usage of the enum.

It has the following benefits:

  1. you have a simpler run-time execution model.
  2. your exceptions, if any, related to creating the MessageProcessor have better traces.
  3. you do not have complicated circular dependencies at construct time.
  4. you only create instances for types you use.

on the down-side, you have the following:

  1. slight performance hit each time you call the method as it checks its state.
  2. well, no 2, really.
\$\endgroup\$
6
  • \$\begingroup\$ IIUC this solution still requires the FactoryClass to know all the processors, right?! \$\endgroup\$ Commented May 19, 2015 at 14:33
  • 1
    \$\begingroup\$ This only requires the FactoryClass to know the processors that are actually going to be used. The Factory is now called at "run time", not at "load time". This makes a big difference. \$\endgroup\$
    – rolfl
    Commented May 19, 2015 at 14:34
  • \$\begingroup\$ What is the difference between that and my first suggestion? In both cases the factory knows only the processors that are used and in both cases people who implement processors need to remember to update the Factory. I'm not sure I understand the "load time" vs. "run time" distinction. In my solution the static function getProcessors is called once in the main which is during run-time. \$\endgroup\$ Commented May 19, 2015 at 14:46
  • \$\begingroup\$ I think I understand. You are saying that this way only types that are actually seen in the wild are instantiated. \$\endgroup\$ Commented May 19, 2015 at 14:54
  • \$\begingroup\$ That is one of the key benefits. For example, if only TYPE001 is ever used, then the factory only ever creates a single Processor, for TYPE001. \$\endgroup\$
    – rolfl
    Commented May 19, 2015 at 15:02
2
\$\begingroup\$

My current design has users writing their processors and I have one factory that they have to change:

   public static Map<MessageType, MessageProcessor> getProcessors() {
      Map<MessageType, MessageProcessor> processors = Maps.newHashMap();
      processors.put(TYPE33,  new Type33Processor());
      // ...
      return processors;
   }

My problem with this is twofold:

  1. Each new processor has to remember to add this boilerplate.
  2. This factory has to know all processor types.

My rationale for suggesting users to supply a Collection<MessageProcessor> stem from the presumption that if they have to modify a method body to include their implementation, they certainly can create such a Collection to do so instead. Also, I quoted your two 'problems' above because I'm not sure if I'm getting them right:

  1. Do you mean users when creating a new MessageProcessor must append another put() inside this method body, instead of the MessageProcessor implementation itself knowing how to update the Map?
  2. Are you referring to are the number of MessageProcessor implementations that the users choose to put() in the Map when you mentioned 'all processor types', or everything loaded by the classloader (which I infer from your reflection solution)?

To retrofit my suggestion into your example, I suppose that somewhere in your users' codebase contain their required MessageProcessor objects, such as the ones they are already creating now. Thereafter, they can pass them into your 'factory class' so that it can construct the mapping. For example, using a static setter:

(for-loops)

public static Map<MessageType, MessageProcessor> setProcessors(MessageProcessor... procs) {
    Map<MessageType, MessageProcessor> result = new EnumMap<>(MessageType.class);
    for (MessageProcessor current : procs) {
        result.put(current.getMessageType(), current);
    }
    return result;
}

(Java 8 Stream equivalent)

public static Map<MessageType, MessageProcessor> setProcessors(MessageProcessor... procs) {
    return Stream.of(procs).collect(Collectors.toMap(
                    MessageProcessor::getMessageType, 
                    Function.identity(), 
                    (a, b) -> b, 
                    () -> new EnumMap<>(MessageType.class));
}

The Map returned from the setter method can be used to set your 'factory class' internally first, which will be returned by getProcessors().

The other suggestion I can give is to use an EnumMap instead, as it is a more efficient implementation for enums over HashMap.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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