My Event-handling system

Question

I have written about 180 lines of code that implements an Event system.

I would like a general review about the code and I'd love comments about the usability of it. (Is the code useful? Would you like to use it or a similar system like it?)

Summary of the parts involved

• IEvent is a marker interface for event classes
• Event is an annotation that must be added for all methods that are listening to events
• EventListener is a marker interface for classes that wants to listen to events
• EventHandler is a class that is responsible for executing an event on a single EventListener
• EventExecutor is the main class that binds everything together. It is where listeners must go to get registered and it is where IEvents must go to get executed.

Reason for implementing

I don't want to use a whole bunch of different interfaces (one for each type of IEvent), along with addThisEventHandler, addThatEventHandler, dispatchThisEvent, dispatchThatEvent and a whole bunch of other repeatedly added stuff.

I want the system to be flexible and useful mainly. Speed is not a primary concern, but if you see something that is having in your opinion a too heavy performance cost then I'd love to hear about it.

The Code (Approximately 180 code lines, not including whitespace and comments)

CustomFacade.getLog... is my own logging system which I don't think is relevant to the review, therefore the source is not included for this part. Besides this, the code is stand-alone (only depending on the regular Java classes, of course).

Event.java

/**
 * Indication that a method should handle an {@link IEvent}<br>
 * The method needs to return {@link Void} and expect exactly one parameter of a type that implements {@link IEvent}.<br>
 * Only methods in classes that implements {@link EventListener} should use this annotation.
 */
@Retention(value = RetentionPolicy.RUNTIME)
@Target(ElementType.METHOD)
public @interface Event {
    int priority() default 50;
}

EventExecutor.java

public class EventExecutor {
    public static final int PRE = -1;
    public static final int ALL = 0;
    public static final int POST = 1;

    private final Map<Class<? extends IEvent>, Collection<EventHandler>> bindings;
    private final Set<EventListener> registeredListeners;

    private boolean debug = false;
    public void setDebug(boolean debug) {
        this.debug = debug;
    }

    public EventExecutor() {
        this.bindings = new HashMap<Class<? extends IEvent>, Collection<EventHandler>>();
        this.registeredListeners = new HashSet<EventListener>();
    }

    public List<EventHandler> getListenersFor(Class<? extends IEvent> clazz) {
        if (!this.bindings.containsKey(clazz)) 
            return new ArrayList<EventHandler>(); // No handlers so we return an empty list
        return new ArrayList<EventHandler>(this.bindings.get(clazz));
    }

    public <T extends IEvent> T executeEvent(T event, int i) {
        Collection<EventHandler> handlers = this.bindings.get(event.getClass());
        if (handlers == null) {
            if (this.debug)
                CustomFacade.getLog().d("Event " + event.getClass().getSimpleName() + " has no handlers.");
            return event;
        }
        if (this.debug)
            CustomFacade.getLog().d("Event " + event.getClass().getSimpleName() + " has " + handlers.size() + " handlers.");
        for (EventHandler handler : handlers) {
            // Basic support for multi-stage events. More can be added later by specifying exactly which priority to be executed - executeEventPre(event, lessThanPriority) for example
            if (i == PRE && handler.getPriority() >= 0)
                continue;
            if (i == POST && handler.getPriority() < 0)
                continue;
            handler.execute(event);
        }
        return event;
    }
    public <T extends IEvent> T executeEvent(T event) {
        return this.executeEvent(event, ALL);
    }

    public void registerListener(final EventListener listener) {
        CustomFacade.getLog().v("Register event listener: " + listener);

        if (registeredListeners.contains(listener)) {
            CustomFacade.getLog().w("Listener already registred: " + listener);
            return;
        }

        Method[] methods = listener.getClass().getDeclaredMethods();
        this.registeredListeners.add(listener);
        for (final Method method : methods) {
            Event annotation = method.getAnnotation(Event.class);
            if (annotation == null)
                continue;

            Class<?>[] parameters = method.getParameterTypes();
            if (parameters.length != 1) // all listener methods should only have one parameter
                continue;

            Class<?> param = parameters[0];

            if (!method.getReturnType().equals(void.class)) {
                CustomFacade.getLog().w("Ignoring method due to non-void return: " + method.getName());
                continue;
            }

            if (IEvent.class.isAssignableFrom(param)) {
                @SuppressWarnings("unchecked") // Java just doesn't understand that this actually is a safe cast because of the above if-statement
                Class<? extends IEvent> realParam = (Class<? extends IEvent>) param;

                if (!this.bindings.containsKey(realParam)) {
                    this.bindings.put(realParam, new TreeSet<EventHandler>());
                }
                Collection<EventHandler> eventHandlersForEvent = this.bindings.get(realParam);
                CustomFacade.getLog().v("Add listener method: " + method.getName() + " for event " + realParam.getSimpleName());
                eventHandlersForEvent.add(createEventHandler(listener, method, annotation));
            }
        }
    }

    private EventHandler createEventHandler(final EventListener listener, final Method method, final Event annotation) {
        return new EventHandler(listener, method, annotation);
    }

    public void clearListeners() {
        this.bindings.clear();
        this.registeredListeners.clear();
    }

    public void removeListener(EventListener listener) {
        for (Entry<Class<? extends IEvent>, Collection<EventHandler>> ee : bindings.entrySet()) {
            Iterator<EventHandler> it = ee.getValue().iterator();
            while (it.hasNext()) {
                EventHandler curr = it.next();
                if (curr.getListener() == listener) 
                    it.remove();
            }
        }
        this.registeredListeners.remove(listener);
    }
    public Map<Class<? extends IEvent>, Collection<EventHandler>> getBindings() {
        return new HashMap<Class<? extends IEvent>, Collection<EventHandler>>(bindings);
    }
    public Set<EventListener> getRegisteredListeners() {
        return new HashSet<EventListener>(registeredListeners);
    }
}

EventHandler.java

public class EventHandler implements Comparable<EventHandler> {
    private final EventListener listener;
    private final Method method;
    private final Event annotation;

    public EventHandler(EventListener listener, Method method, Event annotation) {
        this.listener = listener;
        this.method = method;
        this.annotation = annotation;
    }

    public Event getAnnotation() {
        return annotation;
    }

    public Method getMethod() {
        return method;
    }
    public EventListener getListener() {
        return listener;
    }

    public void execute(IEvent event) {
        try {
            method.invoke(listener, event);
        } catch (IllegalAccessException e1) {
            CustomFacade.getLog().e("Exception when performing EventHandler " + this.listener + " for event " + event.toString(), e1);
        } catch (IllegalArgumentException e1) {
            CustomFacade.getLog().e("Exception when performing EventHandler " + this.listener + " for event " + event.toString(), e1);
        } catch (InvocationTargetException e1) {
            CustomFacade.getLog().e("Exception when performing EventHandler " + this.listener + " for event " + event.toString(), e1);
        } 
    }
    @Override
    public String toString() {
        return "(EventHandler " + this.listener + ": " + method.getName() + ")";
    }

    public int getPriority() {
        return annotation.priority();
    }

    @Override
    public int compareTo(EventHandler other) {
        // Because we are using a TreeSet to store EventHandlers in, compareTo should never return "equal".
        int annotation = this.annotation.priority() - other.annotation.priority();
        if (annotation == 0) 
            annotation = this.listener.hashCode() - other.listener.hashCode();
        return annotation == 0 ? this.hashCode() - other.hashCode() : annotation;
    }
}

EventListener.java

/**
 * Marker interface for classes that can be scanned for @Event annotations
 * @see Event
 */
public interface EventListener {}

IEvent.java

/**
 * Marker interface for events
 */
public interface IEvent {}

A simple JUnit test

Normally the class(es) that implements EventListener and the class that contains the EventExecutor are in entirely different packages.

public class EventTest implements EventListener {
    private EventExecutor events;
    private int messages;

    @Before
    public void before() {
        this.events = new EventExecutor();
        this.events.registerListener(this);
    }

    @Test
    public void test() {
        assertEquals(0, messages);
        this.events.executeEvent(new SimpleEvent("Message"));
        assertEquals(1, messages);
    }

    @Event
    public void onSimpleEvent(SimpleEvent event) {
        messages++;
    }
}

Answer 1

Being particularly pedantic, but I have the following observations:

Annotation @Event

You go to great lengths to document @Event but not to document the priority() setting .... just saying... The priority is the sort of thing that needs documentation... is priority == 80 more or less important than priority == 0?

EventExecutor

• There is nothing in here that suggests it is thread-safe. This may well be by design, but for a general-purpose system it will need to be, especially if you are advertising it as being a 'library' implementation.
• It is 'traditional' for getListeners() type methods to return an array\[\] rather than a List. I prefer the array[] because I like primitives, but, additionally, it is 'clearer' that you cannot add and remove items from the array.... Your code returns a list, but it is a modifyable List (even when empty). I believe I would prefer your code as (note how there's now only one lookup in the bindings map now as well):

...

    private static final EventHandler[] EMPTYHANDLERS = {};

    public EventHandler[] getListenersFor(Class<? extends IEvent> clazz) {
        Collection<EventHandler> handlers = this.bindings.get(clazz);
        if (handlers == null || handlers.isEmpty()) 
            return EMPTYHANDLERS; // No handlers so we return an empty list
        return handlers.toArray(new EventHandler[handlers.size()]);
    }

• In registerListener() I think you have inconsistent logging (why log when a single-argument method is non-void, bot not log when you have a multi-argument method?) I think the code should all be condensed to: if (parameters.length == 1 && IEvent.class.isAssignableFrom(parameters[0]) && method.getReturnType().equals(void.class)) { ...
• should getBindings() and getRegisteredListeners() really be public?

EventHandler

I thin kit is naive to have the execute(IEvent) method have no error handling (it swallows all exceptions). I think you should at least document that there is an unchecked exception thrown from the method.... (these are significant problems) like:

/**
 * ......
 * throws IllegalStateException if it is not possible to invoke the event on a listener.
 */
public void execute(IEvent event) throws IllegalStateException {
    try {
        method.invoke(listener, event);
    } catch (IllegalAccessException e1) {
        CustomFacade.getLog().e("Exception when performing EventHandler " + this.listener + " for event " + event.toString(), e1);
        throw new IllegalStateException("Unable to call " + method, e1);
    } catch (IllegalArgumentException e1) {
        CustomFacade.getLog().e("Exception when performing EventHandler " + this.listener + " for event " + event.toString(), e1);
        throw new IllegalStateException("Unable to call " + method, e1);
    } catch (InvocationTargetException e1) {
        CustomFacade.getLog().e("Exception when performing EventHandler " + this.listener + " for event " + event.toString(), e1);
        throw new IllegalStateException("Unable to call " + method, e1);
    } 
}    

Also, what if the invoked method throws an exception... do you propagate that?

Why do you feel it is necessary to have getMethod(). The reflection you are doing should not be exposed....