4
\$\begingroup\$

This code was already reviewed, but the reviewer missed many points of my code. Most of the following question is from the previous review.

I have written a system for handling the dispatch of events between objects. I need this functionality for a larger project I am working on. Objects should be able to dispatch events without knowing anything about the objects that will receive them, or vice versa. (bolded because this is important, and the last person missed it.)

If this indicates that there is something seriously wrong with my larger design, please tell me.

I currently have two classes: EventHandler and EventListener. (There's a third class in the package, ResolvableEventObject, but it just adds a resolve() method to the basic EventObject that is called once the event is finished with dispatch.)

The code is as follows. (imports were skipped)

(If you need more explanation, please ask.)

EventManager.class

public final class EventManager //The manager holds all the EventListeners, and dispatches events among them.
{

    private static EventManager INSTANCE = new EventManager();  //Singleton pattern, as multiple managers is nonsensical.
    private HashMap<Class<? extends EventObject>, LinkedList<EventListener>> listeners;
    private int lockCount = 0;
    private LinkedList<ListenerStub> delayedListeners;

    public static EventManager getInstance()
    {
        return INSTANCE;
    }

    private EventManager()
    {
        listeners = new HashMap();
        delayedListeners = new LinkedList();
    }

    /*Adds a listener if there's nothing using the set of listeners, or delays it
     * until the usage is finished if something is.
     * Package protected because nothing other than EventListener's registerListener() 
     * should call it. Eqivalent statements apply for removeListener() below.
     */
    void addListener(Class<? extends EventObject> clazz, EventListener listener)
    {
        if (lockCount == 0) {
            realAddListener(clazz, listener);
        } else {
            delayListenerOperation(clazz, listener, true);
        }
    }

    private void realAddListener(Class<? extends EventObject> clazz, EventListener listener)
    {
        if (!listeners.containsKey(clazz)) {
            listeners.put(clazz, new LinkedList<EventListener>());
        }
        LinkedList<EventListener> list = listeners.get(clazz);
        list.add(listener);
        System.out.println("added");
    }

    void removeListener(Class<? extends EventObject> clazz, EventListener listener)
    {
        if (lockCount == 0) {
            realRemoveListener(clazz, listener);
        } else {
            delayListenerOperation(clazz, listener, false);
        }
    }

    private void realRemoveListener(Class<? extends EventObject> clazz, EventListener listener)
    {
        LinkedList<EventListener> list = listeners.get(clazz);
        list.remove(listener);
        System.out.println("removed");
    }

    private void delayListenerOperation(Class<? extends EventObject> clazz, EventListener listener, boolean add)
    {
        delayedListeners.add(new ListenerStub(clazz, listener, add));
        System.out.println("delayed");
    }

    public void dispatchEvent(EventObject event) 
    {
        ListenerLock l = new ListenerLock();
        //This odd construction seems to force garbage collection. 
        //Security measure to clean up old objects before dispatching events.
        System.gc();
        try {
            Thread.sleep(1);
        } catch (Exception ex) {
        }
        //end of odd construction
        dispatchEvent(event, event.getClass()); //Hiding recursive call from user
        l.release();
        if (event instanceof ResolvableEventObject) {
            ((ResolvableEventObject) event).resolve();
            //ResolvableEventObject is my class, it's just an EventObject with a
            //resolve() method.
        }
    }

    private void dispatchEvent(EventObject event, Class clazz)
    {
        LinkedList<EventListener> list = listeners.get(clazz);
        if (list != null) {
            for (EventListener listener : list) {
                listener.handleEvent(event);
            }
        }
        if (EventObject.class.isAssignableFrom(clazz.getSuperclass())) {
            dispatchEvent(event, clazz.getSuperclass());
        }
    }

    private void resolveDelayedListeners()
    {
        for (ListenerStub listenerStub : delayedListeners) {
            if (listenerStub.isAdd()) {
                realAddListener(listenerStub.getListenerClass(), listenerStub.getListener());
            } else {
                realRemoveListener(listenerStub.getListenerClass(), listenerStub.getListener());
            }
        }
    }

    private class ListenerStub /*
     * This class is used to hold the data of the listeners to be removed
     * until the lock on the listener set is released.
     */

    {

        private Class<? extends EventObject> clazz;
        private EventListener listener;
        private boolean add;

        ListenerStub(Class<? extends EventObject> clazz, EventListener listener, boolean add)
        {
            this.clazz = clazz;
            this.listener = listener;
            this.add = add;
        }

        public Class<? extends EventObject> getListenerClass()
        {
            return clazz;
        }

        public EventListener getListener()
        {
            return listener;
        }

        public boolean isAdd()
        {
            return add;
        }


    }

    public class ListenerLock
    /*
     * This is a basic lock that is used to prevent modifications to the list of
     * listeners. It releases when told, or when finalized if forgotten.
     */
    {

        boolean released = false;

        ListenerLock()
        {
            lockCount++;
        }

        public void release()
        {
            if (!released) {
                lockCount = Math.min(0, lockCount - 1);
                released = true;
                if (lockCount == 0) {
                    resolveDelayedListeners();
                }
            }
        }

        protected void finalize() throws Throwable
        {
            super.finalize();
            release();
        }
    }
}

EventListener.class

public final class EventListener
{
/*
 * An EventListener pairs a class (EventObject or a subtype) with a method on an object.
 * It can hold either a strong or a weak reference to the object, and defaults weak.
 * It uses reflection to call the specified method.
 */
    Object source = null;
    Method handler = null;
    Class clazz;
    WeakReference<Object> sourceReference;

    public EventListener(Class<? extends EventObject> clazz, Object source,
                         String handlingMethod, boolean weakReference)
    {

        if (weakReference) {
            this.source = null;
            sourceReference = new WeakReference(source);
        } else {
            this.source = source;
            sourceReference = null;
        }
        /*
         * This structure sets up the call to the specified method on the specified
         * object. If no method is found for the specified class to listen to,
         * superclasses are tried until either a valid method is found or EventObject
         * is reached with no match.
         */
        Class C = clazz;
        while (EventObject.class.isAssignableFrom(C) && handler == null) {
            try {
                this.handler = source.getClass().getMethod(handlingMethod, C);
            } catch (NoSuchMethodException e) {
                C = C.getSuperclass();
            }
        }
        if (handler == null) {
            throw new IllegalArgumentException("No method with the signature: "
                                               + handlingMethod + "("
                                               + clazz.getSimpleName() + ") found.");
        }
        this.clazz = clazz;
    }

    public EventListener(Class<? extends EventObject> clazz, Object source,
                         String handlingMethod)
    {
        this(clazz, source, handlingMethod, true);
    }

    void handleEvent(EventObject event) //package protected because it should only be
                                        //called by EventManager's dispatchEvent().
    {
        if (source == null) { //source == null iff using weak references.
            if (sourceReference.get() == null) { //referenced object garbage collected, delete listener.
                deregisterListener();
                return;
            }
            try {
                handler.invoke(sourceReference.get(), event);
            } catch (IllegalAccessException | IllegalArgumentException |
                     InvocationTargetException e) {
                throw new IllegalArgumentException(e.getMessage(), e.getCause());
            }
        } else {
            try {
                handler.invoke(source, event);
            } catch (IllegalAccessException | IllegalArgumentException |
                     InvocationTargetException e) {
                throw new IllegalArgumentException(e.getMessage(), e.getCause());
            }
        }
    }

    public void registerListener() //registers a listener with the manager. Reference to listener
                                   //is required to deregister later.
    {
        EventManager.getInstance().addListener(clazz, this);
    }

    public void deregisterListener()
    {
        EventManager.getInstance().removeListener(clazz, this);
    }
}

Could you please tell me whether this is proper practice, or if there is a better way to do it? I am knowledgeable about Java, but this is the first time I have done anything like this.

Also, is there anything you notice that is hurting performance? I haven't had a chance to test it with regards to speed, but my functionality tests completed almost instantly after compile.

If you were wondering, this is intended to be used as part of a library for a larger project, as well as other subsequent projects with complex message passing between unspecified objects that don't reference each other.

\$\endgroup\$
  • \$\begingroup\$ One performance issue I see is that your EventManager uses a single lockCount variable, meaning that every list is locked if I'm adding or removing. This would serialize writes to different lists that would not have a race condition, which is unnecessary. A lock per list would be better here. \$\endgroup\$ – cbojar Mar 13 '14 at 17:27
  • \$\begingroup\$ @cbojar Good catch. How would you suggest I implement that? \$\endgroup\$ – user3033745 Mar 13 '14 at 17:37
3
\$\begingroup\$
  1. If I'm right you're planning to use these classes from multiple threads. If that's the case you should do some synchronization or use thread-safe classes to make it thread-safe.

  2. There are existing event handling libraries. Don't reinvent the wheel unless you have a really good reason for that.

    See also: Effective Java, 2nd edition, Item 47: Know and use the libraries (The author mentions only the JDK's built-in libraries but I think the reasoning could be true for other libraries too.)

  3. private HashMap<Class<? extends EventObject>, LinkedList<EventListener>> listeners;
    

    could be

    private Map<Class<? extends EventObject>, List<EventListener>> listeners;
    

    See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

    Anyway, a Mulitmap would be a better choice here. It was designed exactly to supersede this structure.

  4. The same is true for the delayedListeners:

    private LinkedList<ListenerStub> delayedListeners;
    

    The assigment also could be in the same line as the field declaration.

    private final Map<Class<? extends EventObject>, List<EventListener>> listeners = new HashMap<>();
    private final List<ListenerStub> delayedListeners = new LinkedList<>(); 
    
  5.  // Singleton pattern, as multiple managers is nonsensical.
    

    Singleton is rather an antipattern nowadays. You might need multiple instances in unit and integration tests and later you might have separated modules which will require separate event managers. Consider making it non-singleton. Singletons ussually make unit-testing really hard.

  6. Calling System.gc() is a code smell. Are you sure that you need that? See also: Why is it a bad practice to call System.gc?

  7. /*
     * This is a basic lock that is used to prevent modifications to the list of listeners. It
     * releases when told, or when finalized if forgotten.
     */
    

    I'd put a warning log to the finalize to get a notification if the client is broken.

\$\endgroup\$
  • 2
    \$\begingroup\$ After looking into the provided link, it seems like "EventBus" from "guava-libraries" does the same thing my code does, but is better developed and tested. (It's an existing library) I'll see if I can simply substitute it in place of this ad-hoc solution. If I can't, the other comments were helpful. Also, thank you for not suggesting I use Lock in place of ListenerLock, the behavior of Lock (ie. blocking) is not wanted here. Thanks for the assistance! \$\endgroup\$ – user3033745 Mar 14 '14 at 18:33
  • \$\begingroup\$ @user3033745: I'm glad that it helped. Guava is a high-quality lib, strongly recommended. Thanks for the feedback! \$\endgroup\$ – palacsint Mar 14 '14 at 22:28

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.