How can I make this more readable and cleaner?

Question

I am wondering what I can do to make this more readable and clean. By readable, I mean easier to read for other developers.

I don't really want to have the same code twice. I am thinking that I could make some method or methods to make it shorter, but I'm not exactly sure...

@Override
public void dispatchEvent(Event event) {
    checkNotNull(event);

    CancellableEvent cancellableEvent = null;
    boolean cancellable;
    if (cancellable = event instanceof CancellableEvent) {
        cancellableEvent = (CancellableEvent) event;
        checkArgument(cancellableEvent.isCancelled());
    }

    // Ignore-cancellation event handlers will run
    for (EventPriority priority : EventPriority.values()) {
        Map<Method, EventListener> internalMapping = getRegistry().getMethodMap(event.getClass(), priority, true);
        if (internalMapping != null) {
            for (Entry<Method, EventListener> entry : internalMapping.entrySet()) {
                try {
                    entry.getKey().invoke(entry.getValue(), event);
                } catch (IllegalAccessException e) {
                    e.printStackTrace();
                } catch (IllegalArgumentException e) {
                    e.printStackTrace();
                } catch (InvocationTargetException e) {
                    /*
                     * Delegate any exceptions that occur from
                     * the method to a runtime exception.
                     */
                    throw new RuntimeException(e);
                }
            }
        }
    }

    // Event handlers that consider cancellation will run
    for (EventPriority priority : EventPriority.values()) {
        Map<Method, EventListener> internalMapping = getRegistry().getMethodMap(event.getClass(), priority, false);
        if (internalMapping != null) {
            for (Entry<Method, EventListener> entry : internalMapping.entrySet()) {
                try {
                    entry.getKey().invoke(entry.getValue(), event);
                } catch (IllegalAccessException e) {
                    e.printStackTrace();
                } catch (IllegalArgumentException e) {
                    e.printStackTrace();
                } catch (InvocationTargetException e) {
                    /*
                     * Delegate any exceptions that occur from
                     * the method to a runtime exception.
                     */
                    throw new RuntimeException(e);
                }
                // Immediately return in the case of the event being cancelled.
                if (cancellable && cancellableEvent.isCancelled()) {
                    return;
                }
            }
        }
    }
}

Answer 1

I'm assuming that what you really want to do is eliminate those two loops. I would just brute force it and extract a method containing all the necessary arguments for example:

  @Override
  public void dispatchEvent(Event event) {
      checkNotNull(event);

      CancellableEvent cancellableEvent = null;
      boolean cancellable;
      if (cancellable = event instanceof CancellableEvent) {
          cancellableEvent = (CancellableEvent) event;
          checkArgument(cancellableEvent.isCancelled());
      }

     fireEvents(false, event, cancellableEvent, cancellable);
     fireEvents(true, event, cancellableEvent, cancellable);

  }

  private void fireEvents(boolean considerCancellation, Event event, CancellableEvent cancellableEvent, boolean cancellable)
  {
     // Event handlers that consider cancellation will run
     for (EventPriority priority : EventPriority.values()) {
         Map<Method, EventListener> internalMapping = getRegistry().getMethodMap(event.getClass(), priority, ! considerCancellation);
         if (internalMapping != null) {
             for (Map.Entry<Method, EventListener> entry : internalMapping.entrySet()) {
                 try {
                     entry.getKey().invoke(entry.getValue(), event);
                 } catch (IllegalAccessException e) {
                     e.printStackTrace();
                 } catch (IllegalArgumentException e) {
                     e.printStackTrace();
                 } catch (InvocationTargetException e) {
                     /*
                      * Delegate any exceptions that occur from
                      * the method to a runtime exception.
                      */
                     throw new RuntimeException(e);
                 }
                 // Immediately return in the case of the event being cancelled.
                 if ( considerCancellation && cancellable && cancellableEvent.isCancelled()) {
                     return;
                 }
             }
         }
     }
  }

Then you can refactor the new fireEvents method and clean it up.

Answer 2

If you are talking about exceptions then in java 7 you can club exceptions.

Here is the article about Working with Java7 Exception

} catch (ParseException | IOException exception) {
// handle I/O problems.
}

About Iterations you can have separate method for invoke functionality.

Answer 3

What to recommend about making some code more readable? One of the metric of nice and clean code is known for long time: class should be as small as possible, methods should be as small as possible.

Assuming this you could do some "extract method" refactoring and extract for example:

processIgnoreCancellationEventHandlers(); processEventHandlersWithPossibleCancellation();

I would go even further and make one method with different input params if possible, something like:

processEventHandlers(noCancellationEventHandlers); processEventHandlers(CancellationAwareEventHandlers);

This way you will have two achievements:

• more simple, short and readable code,
• no duplication.

Answer 4

Hard to know without more context but here are some thoughts.

• Your for (Entry<Method, EventListener> entry : internalMapping.entrySet()) { loop seems to be the same for both loops. I would put that into it's own method. It would take in a Map and it would do the entire loop. Then you two for-loops would be much smaller.

  private void runMap(Map<Method, EventListener> methodMap) {
      for (Entry<Method, EventListener> entry : methodMap.entrySet()) {
         ...
      }
  }
  

  Your could then do one loop:

  for (EventPriority priority : EventPriority.values()) {
     runMap(getRegistry().getMethodMap(event.getClass(), priority, true));
     runMap(getRegistry().getMethodMap(event.getClass(), priority, false));
  }
  

• When you are doing something in a loop where if (internalMapping != null) { which encompasses the whole loop then I tend to use if (internalMapper == null) continue;. That reduces the indent levels.

• The exception handling has been mentioned. You can also handle the InvocationTargetException first, and then catch (Exception e) below it for all of the rest to print out.

Answer 5

Never do assignments in if-conditions. This is error-prone:

if (cancellable = event instanceof CancellableEvent) {
    ...
}

Just do this:

boolean cancellable = event instanceof CancellableEvent;
if (cancellable) {
    ...
}

Answer 6

You can refactor the invocation of an entry into another method.

private final void invokeEntry(Entry<Method, EventListener> entry, Event event) {
    try {
        entry.getKey().invoke(entry.getValue(), event);
    } catch (IllegalAccessException e) {
        e.printStackTrace();
    } catch (IllegalArgumentException e) {
        e.printStackTrace();
    } catch (InvocationTargetException e) {
        /*
         * Delegate any exceptions that occur from
         * the method to a runtime exception.
         */
        throw new RuntimeException(e);
    }
}

Then you can replace your dispatchEvent method with this:

@Override
public void dispatchEvent(Event event) {
    checkNotNull(event);

    CancellableEvent cancellableEvent = null;
    boolean cancellable;
    if (cancellable = event instanceof CancellableEvent) {
        cancellableEvent = (CancellableEvent) event;
        checkArgument(cancellableEvent.isCancelled());
    }

    // Ignore-cancellation event handlers will run
    for (EventPriority priority : EventPriority.values()) {
        Map<Method, EventListener> internalMapping = getRegistry().getMethodMap(event.getClass(), priority, true);
        if (internalMapping != null) {
            for (Entry<Method, EventListener> entry : internalMapping.entrySet()) {
                invokeEntry(entry, event);
            }
        }
    }

    // Event handlers that consider cancellation will run
    for (EventPriority priority : EventPriority.values()) {
        Map<Method, EventListener> internalMapping = getRegistry().getMethodMap(event.getClass(), priority, false);
        if (internalMapping != null) {
            for (Entry<Method, EventListener> entry : internalMapping.entrySet()) {
                invokeEntry(entry, event);
                // Immediately return in the case of the event being cancelled.
                if (cancellable && cancellableEvent.isCancelled()) {
                    return;
                }
            }
        }
    }
}

Answer 7

As the loops are identical except the one boolean, I'd start by splitting them like this, then break them down further if required.

@Override
public void dispatchEvent(Event event) {
    checkNotNull(event);
    CancellableEvent cancellableEvent = null;
    boolean cancellable;
    if (cancellable = event instanceof CancellableEvent) {
        cancellableEvent = (CancellableEvent) event;
        checkArgument(cancellableEvent.isCancelled());
    }
    handleEvents(event, true);
    handleEvents(event, false, cancellableEvent);
}

public void handleEvents(Event event, boolean cancellable)
{
    handleEvents(event, cancellable, null);
}

public void handleEvents(Event event, boolean cancellable, CancellableEvent cancellableEvent)
{
    for (EventPriority priority : EventPriority.values()) {
        Map<Method, EventListener> internalMapping = getRegistry().getMethodMap(event.getClass(), priority, cancellable);
        if (internalMapping != null) {
            for (Entry<Method, EventListener> entry : internalMapping.entrySet()) {
                try {
                    entry.getKey().invoke(entry.getValue(), event);
                } catch (IllegalAccessException | IllegalArgumentException e) {
                    e.printStackTrace();
                } catch (InvocationTargetException e) {
                    /*
                    * Delegate any exceptions that occur from
                    * the method to a runtime exception.
                    */
                    throw new RuntimeException(e);
                }
                // Immediately return in the case of the event being cancelled.
                if (cancellableEvent != null && cancellable && cancellableEvent.isCancelled()) {
                    return;
                }
            }
        }
    }
}