Fast Event Manager

Question

I've recently coded an event manager with performance as the main factor. I'd like to know what you think of it and what could be made better.

public class EventManager {

    public static EventManager mouseRightClick = new EventManager(EventType.mouseRightClick), mouseLeftClick = new EventManager(EventType.mouseLeftClick);

    public final EventType type;

    public List<Listener> listeners = new ArrayList<Listener>();

    private EventManager(EventType type){
        this.type = type;
    }

    public void registerListener(Listener l){
        if (!listeners.contains(l)){
            listeners.add(l);
        }

    }

    public void unregisterListener(Listener l){
        listeners.remove(l);

    }

    public void dispatchEvent(Event e){
        for (int i = listeners.size() - 1; i >= 0; i--){
            listeners.get(i).onEvent(e, type);
        }
    }

}

That's basically all the code, and I've tried to keep it as simple as possible.

Answer 1

One statement per line

Avoid declaring multiple variables on the same line, especially when the line is long. At first I didn't see that there are two variables declared on this line:

public static EventManager mouseRightClick = new EventManager(EventType.mouseRightClick), mouseLeftClick = new EventManager(EventType.mouseLeftClick);

This is much better:

public static EventManager mouseRightClick = new EventManager(EventType.mouseRightClick)
public static EventManager mouseLeftClick = new EventManager(EventType.mouseLeftClick);

General purpose and reusability

At first glance it looks like the class has a nice general purpose, to manage listeners and dispatch events to them. The mouseRightClick and mouseLeftClick members violate the general purpose and the reusability of this class. It would be better to move those out to a different class, perhaps an EventManagerRegistry.

Mutable public members

The listeners member shouldn't be public. A malicious user could change its value and break your class. It should be private. And it can be final, as it's never reassigned during the lifetime of the class.

Performance

Let's see the time complexity of the main operations:

• Register listener: \$O(n)\$
• Unregister listener: \$O(n)\$

This is because you store listeners in a list. You could do better by storing them in a Set. On the other hand, if dispatching in reverse order is important, then using a Set would be tricky. I don't know your reasons for iterating in reverse order. If you wouldn't mind iterating in insertion order, then you could use a LinkedHashSet.

Thread safety

Keep in mind that the implementation is not thread-safe. If the class might be used by multiple threads, you can get runtime exceptions and incorrect behavior.