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;
                }
            }
        }
    }
}

For a start you can replace the multiple catch statements with one Multi-catch, like: `catch (IllegalArgumentException | IllegalAccessException e)`. Note the or uses the bitwise inclusive rather than logical or. — MrLore, Sep 26 '12 at 18:48
You could refactor the try/catch to another method, `invokeEntry(Entry entry)`, and call it from both for loops. — jalynn2, Sep 26 '12 at 18:55
you shouldn't really call `Exception.printStackTrace()` inside real code — matt b, Sep 26 '12 at 18:56
^ I know. This is for academic purposes. BTW jalynn2 that helped a bit and I wrote an answer taking your advice. — Thomas Nappo, Sep 26 '12 at 18:57

Amit Deshpande · Answer 1 · 2012-09-26T18:59:44.137

3

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.

edited Sep 26 '12 at 18:59

answered Sep 26 '12 at 18:48

Amit Deshpande

19,001
4
46
72

I know this, but I want to target Java 6. The real issue is that I'm writing the iteration twice and stuff... – Thomas Nappo Sep 26 '12 at 18:55

score 2 · Answer 2 · answered Sep 26 '12 at 18:58

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.

score 2 · Answer 3 · answered Sep 26 '12 at 18:58

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.

Guido Simone · Accepted Answer · 2012-09-26T19:13:39.200

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.

score 2 · Answer 5 · answered Sep 26 '12 at 19:08

2

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

answered Sep 26 '12 at 19:08

Fabian Barney

14,219
5
40
60

I agree about the assignments in if-conditions. `cancellable` would need to be set in your second `if` since it's used later. – user1201210 Sep 26 '12 at 19:09
... but I would refactor this use of cancellable, too. Because the second part of that if-condition is a NPE candidate when you have to think about 2 corners that it cannot be null here if cancellable is true. It should simply be `cancellableEvent != null && ...` – Fabian Barney Sep 26 '12 at 19:56

score 1 · Answer 6 · answered Sep 26 '12 at 18:59

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;
                }
            }
        }
    }
}

The invocation is good to break out. The loops and `internalMapping` read could be broken out as well. — user1201210, Sep 26 '12 at 19:19

score 1 · Answer 7 · answered Sep 26 '12 at 19:05

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;
                }
            }
        }
    }
}

How can I make this more readable and cleaner?

7 Answers7