1
\$\begingroup\$

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.

\$\endgroup\$
0

1 Answer 1

2
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ I've heard that this method of iteration (reverse iteration) is very effective. Thanks for your help. Also do you think merging EventType and EventManager into one enumeration with these features would be a good idea? \$\endgroup\$
    – Matt
    Jul 5, 2015 at 16:46
  • \$\begingroup\$ In what way is reverse iteration "effective"? I don't see it. I also don't see what you mean by merging EventType and EventManager into one "enumeration". \$\endgroup\$
    – janos
    Jul 5, 2015 at 16:50
  • \$\begingroup\$ I just tried to use a Set instead of a List. The results showed that a List performed nearly two times better. Reverse iteration is apparently more effective because you're calling for the list length only one time, on the first iteration. EventType+EventManager, as each event manager is in one instance in this code, instead it would be merged all into one class, where each manager for each type of Event would be an enumeration item, if it makes sense. \$\endgroup\$
    – Matt
    Jul 5, 2015 at 16:59
  • \$\begingroup\$ Here's what I mean: pastebin.com/fCi4wqJG EDIT: I've quickly made this in pastebin, pretend that the argument type of the dispatchEvent(Event e) method is named EventData. \$\endgroup\$
    – Matt
    Jul 5, 2015 at 17:03
  • \$\begingroup\$ You can of course save the list size in a local variable for the same effect. But the best is to use an enhanced for-each loop, for (Listener listener : listeners) { ... } which will also cache the list size behind the scenes. \$\endgroup\$
    – janos
    Jul 5, 2015 at 17:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.