Cleanly handling events

Question

I have code similar to this in all my observer classes that handle events fired by an event bus class. As you can see there are a lot of instanceof checks to choose the path of action needed to appropriately handle events, and I was wondering if this could be done more cleanly, eliminating the instanceof tests?

@Override
public void handleEvent(Event event) {
    if (event instanceof DownloadStartedEvent) {
        DownloadStartedEvent dsEvent = (DownloadStartedEvent)event;
        dsEvent.getDownloadCandidateItem().setState(new BusyDownloadingState());
    } else if (event instanceof DownloadCompletedEvent) {
        DownloadCompletedEvent dcEvent = (DownloadCompletedEvent)event;
        dcEvent.getDownloadCandidateItem().setState(new FinishedDownloadingState());
        DownloadCandidate downloadCandidate = dcEvent.getDownloadCandidateItem(). getDownloadCandidate();
        if (downloadCandidate.isComplete()) {
            // start extracting
        }
    } else if (event instanceof DownloadFailedEvent) {
        DownloadFailedEvent dfEvent = (DownloadFailedEvent)event;
        dfEvent.getDownloadCandidateItem().setState(new FailedDownloadingState());
    }
}

Answer 1

Yes, assuming you control the Event class, and/or its subclasses. You could add an abstract handle method to your Event class, and move the specific code for each subclass into that method.

It would look like this:

abstract class Event {
  //...
  public abstract void handle();
}

class DownloadStartedEvent extends Event {
  //...
  @Override
  public void handle() {
    getDownloadCandidateItem().setState(new BusyDownloadingState());
  }
}

// The same for the other classes

In your calling code, you simply write:

@Override
public void handleEvent(Event event) {
  event.handle();
}

Answer 2

You could eliminate the events by adding listeners for each event in turn. Consider for instance

public void handleStartedEvent(DownloadStartedEvent ev) { ... }
public void handleCompletedEvent(DownloadCompletedEvent ev) { ... }
public void handleFailedEvent(DownloadFailedEvent ev) { ... }

You may also consider combining Completed/Failed into a single event since it looks like you already have an isCompleted method. You could handle CompleteEvent and check for success. If successful you could start extracting otherwise you could set your failure state.

My other thought would be why are you setting a new object as a state indicator. Could that perhaps be better served using constants/enum values?

Answer 3

You can come up with a cleaner solution using annotations. You'll need to define @EventHandler and @HandlesEvent, then use it like:

@EventHandler
public class MyEventHandler {

   @HandlesEvent(DownloadStartedEvent.class)
   public void handleDownloadStartedEvent(DownloadStartedEvent dse) {
      dsEvent.getDownloadCandidateItem().setState(new BusyDownloadingState());
   }

   @HandlesEvent(DownloadCompletedEvent.class)
   public void handleDownloadCompletedEvent(DownloadComletedEvent dse) {
      dcEvent.getDownloadCandidateItem().setState(new FinishedDownloadingState());
      DownloadCandidate downloadCandidate = dcEvent.getDownloadCandidateItem(). getDownloadCandidate();
      if (downloadCandidate.isComplete()) {
            // start extracting
      }
   }


   // etc.

}

Of course you'll need additional code to register this class and it will need to use reflection to check which method handles which event. Your observables then interact with this registrar which in turn interacts with the above @EventHandler annotated class.

Answer 4

if what you need to do is just to instantiate "states" like BusyDownloadedState or FinishedDownloadedState you could give your Event class an attribute that states which is the state that must be set according to the kind of event.

If you need to have both simple events and complex events you can have a mix of two things:

class Event
{
   State state;
   boolean isSimple;
}

public void handleEvent(Event event)
{
  if (event.isSimple)
    setState(event.state)
  else
  {
     if (event instanceof WhateverEvent)
        // special handling
  }
}

Answer 5

Improving on Ryan's answer, instead of having a separate method for each event, you can use generics. Then you could write:

public void registerListeners() {

eventSource.addListener(DownloadStartedEvent.class, new EventHandler<DownloadStartedEvent>() {
    public void handleEvent(DownloadStartedEvent event) {
        dsEvent.getDownloadCandidateItem().setState(new BusyDownloadingState());
    }
});

eventSource.addListener(DownloadCompletedEvent.class, new EventHandler<DownloadCompletedEvent>() {
    public void handleEvent(DownloadCompletedEvent event) {
        dcEvent.getDownloadCandidateItem().setState(new FinishedDownloadingState());
        DownloadCandidate downloadCandidate = dcEvent.getDownloadCandidateItem(). getDownloadCandidate();
        if (downloadCandidate.isComplete()) {
            // start extracting
        }
    }
});

eventSource.addListener(DownloadFailedEvent.class, new EventHandler<DownloadFailedEvent>() {
    public void handleEvent(DownloadFailedEvent event) {
        dfEvent.getDownloadCandidateItem().setState(new FailedDownloadingState());
    }
});
}

I leave the implementation of addListener() as an exercise for the reader.

Answer 6

How about:

public void handleEvent(Event e) {
   // Not interested
}


public void handleEvent(DownloadStartedEvent dsEvent) {
    dsEvent.getDownloadCandidateItem().setState(new BusyDownloadingState());
}


public void handleEvent(DownloadCompletedEvent dcEvent) {
    dcEvent.getDownloadCandidateItem().setState(new FinishedDownloadingState());
    DownloadCandidate downloadCandidate = dcEvent.getDownloadCandidateItem().getDownloadCandidate();
    if (downloadCandidate.isComplete()) {
        // start extracting
    }
}

public void handleEvent(DownloadFailedEvent dfEvent) {
    dfEvent.getDownloadCandidateItem().setState(new FailedDownloadingState());
}