My Event-handling system

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}
 * The method needs to return {@link Void} and expect exactly one parameter of a type that implements {@link IEvent}.
 * 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;
}

EventEx

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 clazz) {
        Collection 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....

StackExchange Code Review Q#36153, answer score: 8