5
\$\begingroup\$

I have created a simple event manager and I want it to work properly in a multithreaded environment:

public enum EventType {
    EVENT_TYPE_A,
    EVENT_TYPE_B,
    EVENT_TYPE_C;
}


public interface Observer {
    void onEvent(EventType eventType);
}


public class ObserverManager {
    private Set<Observer> m_weakReferencedObservers;

    public ObserverManager() {
        m_weakReferencedObservers = Collections.newSetFromMap(new WeakHashMap<Observer,Boolean>()); // allows a set of weak references
    }

    public void add(Observer observer){
        if (observer != null) {
            m_weakReferencedObservers.add(observer);
        }
    }

    public void remove(Observer observer){
        m_weakReferencedObservers.remove(observer);
    }

    public void callEvent(EventType eventType){
        m_weakReferencedObservers.stream()
                .filter(Objects::nonNull)
                .forEach(observer -> observer.onEvent(eventType));

        m_weakReferencedObservers.remove(null);
    }
}

public class EventManager {
    private Map<EventType,ObserverManager> m_observerManagers = new HashMap<>();

    public void registerObserver(EventType eventType,Observer observer){
        synchronized (m_observerManagers){
            if (!m_observerManagers.containsKey(eventType)){
                m_observerManagers.put(eventType,new ObserverManager());
            }

            ObserverManager observerManager = m_observerManagers.get(eventType);
            observerManager.add(observer);
        }
    }

    public void unregisterObserver(EventType eventType,Observer observer){
        synchronized (m_observerManagers) {
            if (!m_observerManagers.containsKey(eventType)) {
                return;
            }

            ObserverManager observerManager = m_observerManagers.get(eventType);
            observerManager.remove(observer);
        }
    }

    public void callEvent(EventType eventType){
        if (!m_observerManagers.containsKey(eventType)){
            return;
        }

        ObserverManager observerManager = m_observerManagers.get(eventType);
        observerManager.callEvent(eventType);
    }
}

A few points:

  1. Is my use of weak references appropriate here?
  2. Is my use of the synchronized keyword appropriate here?
  3. In general, is there a better way to implement this?
  4. If I want my event manager to be able to send many more different onEvent methods (with different prototypes), what's the best way to do it? So, instead of only having onEvent(EventType eventType), I could also easily use onEvent(EventType eventType, MyClass myClass). I mean, in general, how can I let the user of this class create his own onEvent() prototype?
\$\endgroup\$
1
  • \$\begingroup\$ I have rolled back the last edit. Please see What to do when someone answers. Feel free to post a follow-up question instead. Don't feel rushed, there could still be answers incoming. \$\endgroup\$
    – Mast
    Aug 30, 2016 at 0:07

1 Answer 1

3
\$\begingroup\$

Answers to Your Questions

  1. Yes, this usage is acceptable if your intention was to have a Set of Observer objects that can be removed by the garbage collector if they are not referenced from elsewhere.

  2. Instead of using the old-school synchronized keyword, I'd recommend to add a ReentrantReadWriteLock field in EventManager and use it in the three methods.

  3. I think that your implementation is already quite simple, readable and usable. Making it better would depend on the goal you are looking for. But there are still a few remarks after these answers.

  4. If you add another method like onEvent(EventType, Class), it will have drawbacks such as pollution of the APIs of the objects that do not need the second method, ambiguities in the choice of the method to use for the caller, etc. If you want to grant your user a possibility to define custom events, the simplest way that I see here is to replace the EventType argument with String: the API will not be coupled with the rigid enum (which needs to be modified in order to introduce a new event type) and the user will be able to define his own event types easily.

Other Remarks

  1. Style: the m_ prefix for the private fields violates standard Java naming conventions.

  2. There is no need to call observerManagers.containsKey() to check if the item is already in the map. For Maps, there is the putIfAbsent method that does exactly the same in one line.

  3. I'm not sure that .filter(Objects::nonNull) and weakReferencedObservers.remove(null) are necessary in ObserverManager.callEvent. How do you expect null values to be present in the collection, if add method checks the object to be not null?

\$\endgroup\$
2
  • \$\begingroup\$ 2. What would be the advantage of using ReentrantReadWriteLock? Using it would mean to add try catch blocks everywhere, decreasing the readability. 4. Makes sense, thanks. 2. Thanks a lot, I didn't know putIfAbsent, it will simplify my life. 3. I agree but I'm not entirely sure how the set is reacting when a weak reference is not used anymore outside. In a WeakHashMap, the key would be remove, but in the set, I'm not so sure. So I assumed that it might be set to null. \$\endgroup\$
    – dyesdyes
    Aug 29, 2016 at 22:51
  • \$\begingroup\$ For 2. ReentrantReadWriteLock, it's actually interesting after reading the documentation. Thanks! For your last point, about null values, according to this question: stackoverflow.com/questions/19367338/…, this is actually useful to check. \$\endgroup\$
    – dyesdyes
    Aug 30, 2016 at 0:02

This site is temporarily in read-only mode and not accepting new answers.

Not the answer you're looking for? Browse other questions tagged .