Event Handling system in Java

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. 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.)

Note: this is the old code, kept for TwoThe's answer. See below for new code.

EventManager.class

```
public class EventManager //The manager holds all the EventListeners,
//and dispatches events among them.
{
//Singleton pattern, as multiple managers is nonsensical.
private static EventManager INSTANCE = null;
private HashMap, ArrayList> listeners;
private int lockCount = 0;
private ArrayList delayedListeners;

public static EventManager getInstance()
{
if (INSTANCE == null) {
INSTANCE = new EventManager();
}
return INSTANCE;
}

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

/*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 c, EventListener m)
{
if (lockCount == 0) {
realAddListener(c, m);
} else {
delayListenerOperation(c, m, true);
}
}

void realAddListener(Class c, EventListener m)

In general an event listener is something used often in Java programs, that is why there are already a lot of event managers that you can use, for example the Observable pattern. So what you are basically doing is to reinvent the square wheel. It is a good idea to always look for the work of others first before doing unnecessary work.

private static EventManager INSTANCE = null;

The proper singleton pattern to use is:

private static final EventManager INSTANCE = new EventManager();

This reduces the getInstance() Method to:

public static EventManager getInstance() {
    return INSTANCE;
}

Some people discuss whether or not you even need a getInstance() method here, since you could just modify INSTANCE to be public instead vs "the Java way".

private ArrayList delayedListeners;

You seem to use ArrayList for all purposes. There are other List-types in Java that suit certain situations better than an ArrayList. You should have a look at those and try to get a feeling when to use which list. In this particular case i.E. a LinkedList is better.

private EventManager()

This should be protected, just in case you want to eventually inherit this class at any point in the future. private should only be used to prevent inheritance on purpose.

delayListenerOperation(c, m, true);

What is the purpose of delaying listeners? I don't really see one in your event system. If you want to make sure some events are processed later, try a PriorityQueue to sort Events based on their order. Working with "global" variables for these purposes is prone to threading issues or coding mistakes.

ListenerLock l = new ListenerLock();

This is a C++-pattern in Java, it won't work as expected and shouldn't be used. You should use a class-global Lock if you really need to lock something. But since this code isn't thread-safe at all, I don't see the reason for a lock.

System.gc();

Never! do that, unless you have a really good reason to and absolutely know and have proven that you must do it. The garbage collector is very smart in the way it works, and those constructs just dumb it down and cause hick-ups or a waste of processing time.

Thread.sleep(1);

Why? What do you try to archive with that? This serves no purpose.

ListenerStub(Class c, EventListener m, boolean add)

c, m or add are not a good choice for variable names. Try to be expressive so you still know what this does should you have to review the code in 2 years.

public class ListenerLock

Reinventing the wheel once more. See Lock for the already existing (and actually working) implementation.

protected void finalize() throws Throwable

Again: never do that. This is prone to many errors as you can never tell when this function is actually called.

public class EventListener

This should be an Interface if you want other classes to be able to implement this. Otherwise every class needs to use this as a base class, and in Java you can only inherit from one class. That would as well simplify a lot of your handling code down to a simple

void handleEvent(EventObject e)

in the actual class that is supposed to handle the event.

if (weakReference) {
    this.source = null;
    sourceReference = new WeakReference(source);
} else {
    this.source = source;
    sourceReference = null;
}

If you use Reference as base class here and write your own StandardReference class you can simplify this to:

this.source = weakReference ? new WeakReference(source) : new StandardReference(source);

This would be more elegant and remove the duplicated variable.

Summary

You wrote a lot of code to duplicate existing functionality but did neither improve nor add to the existing one. I would suggest to remove those classes and work with the existing functionality to solve your problem.

That would remove all bugs and potential bugs from the code you have, and will be future-proof should there be any major changes to Java.

StackExchange Code Review Q#44235, answer score: 5