Exception Wrapper #32

Merged
kske merged 3 commits from f/exception-wrapper into develop 2022-01-08 16:54:06 +01:00
4 changed files with 68 additions and 3 deletions
Showing only changes of commit e53f356c54 - Show all commits

View File

@ -112,10 +112,11 @@ public final class EventBus {
* *
* @param event the event to dispatch * @param event the event to dispatch
* @throws EventBusException if an event handler isn't accessible or has an invalid signature * @throws EventBusException if an event handler isn't accessible or has an invalid signature
* @throws ExceptionWrapper if it is thrown by an event handler
* @throws NullPointerException if the specified event is {@code null} * @throws NullPointerException if the specified event is {@code null}
* @since 0.0.1 * @since 0.0.1
*/ */
public void dispatch(Object event) throws EventBusException { public void dispatch(Object event) {
Objects.requireNonNull(event); Objects.requireNonNull(event);
logger.log(Level.INFO, "Dispatching event {0}", event); logger.log(Level.INFO, "Dispatching event {0}", event);
@ -140,6 +141,10 @@ public final class EventBus {
// Transparently pass error to the caller // Transparently pass error to the caller
throw (Error) e.getCause(); throw (Error) e.getCause();
else if (e.getCause() instanceof ExceptionWrapper)
// Transparently pass exception wrapper to the caller
throw (ExceptionWrapper) e.getCause();
else if (event instanceof DeadEvent || event instanceof ExceptionEvent) else if (event instanceof DeadEvent || event instanceof ExceptionEvent)
// Warn about system event not being handled // Warn about system event not being handled
@ -214,7 +219,7 @@ public final class EventBus {
* @since 0.0.1 * @since 0.0.1
* @see Event * @see Event
*/ */
public void registerListener(Object listener) throws EventBusException { public void registerListener(Object listener) {
delvh marked this conversation as resolved
Review

Why did you remove that declaration?

Why did you remove that declaration?
Review

Because we decided against that for our other projects and it also seems to be like this in other big Java projects like Spring. The @throws in the Javadoc will stay, though.

Because we decided against that for our other projects and it also seems to be like this in other big Java projects like Spring. The `@throws` in the Javadoc will stay, though.
Review

I think that every exception you explicitly declare via throw should also be mentioned in throws.

I know it is unnecessary for unchecked exceptions.

However, I noticed that whenever I'm not directly in a Giga-IDE such as Eclipse, my first course of action is to look for throws in the source code, and not @throws in the Javadoc, especially as it is often faster to open the source code and look there instead of the javadoc as Javadoc always needs a timeout to plop up, and knowing Eclipse, it sometimes even doesn't plop up. And even if it plops up, you still have to read through the Javadoc, which is also longer than simply looking at the source code.

That's why I prefer it that way.

And regarding Spring: Spring is not a project where you should get your styleguides from. Otherwise we also should name our interface implementations <interface>Impl or <interface>Spec. Or have classes such as the MultiPartResolverTestFactoryTest.

I think that every exception you **explicitly** declare via `throw` should also be mentioned in `throws`. I know it is unnecessary for unchecked exceptions. However, I noticed that whenever I'm not directly in a Giga-IDE such as Eclipse, my first course of action is to look for `throws` in the source code, and not `@throws` in the Javadoc, especially as it is often faster to open the source code and look there instead of the javadoc as Javadoc always needs a timeout to plop up, and knowing Eclipse, it sometimes even doesn't plop up. And even if it plops up, you still have to read through the Javadoc, which is also longer than simply looking at the source code. That's why I prefer it that way. And regarding Spring: Spring is **not** a project where you should get your styleguides from. Otherwise we also should name our interface implementations `<interface>Impl` or `<interface>Spec`. Or have classes such as the `MultiPartResolverTestFactoryTest`.
Review

In that case, however, the code base would already violate compliance, as we don't declare throws NullPointerException for example. Also, we would have to consider all unchecked exceptions potentially thrown by the methods that are called by the method we want to declare throws on, which don't follow those conventions themselved. That would be a large and unnecessary effort in my opinion.

In that case, however, the code base would already violate compliance, as we don't declare `throws NullPointerException` for example. Also, we would have to consider all unchecked exceptions potentially thrown by the methods that are called by the method we want to declare `throws` on, which don't follow those conventions themselved. That would be a large and unnecessary effort in my opinion.
Review

I think that every exception you explicitly declare via throw should also be mentioned in throws.

> I think that every exception you **explicitly** declare via `throw` should also be mentioned in `throws`.
Review

Why? This is completely arbitrary. So the NullPointerException should be removed frmo the Javadoc in that case?

Why? This is completely arbitrary. So the `NullPointerException` should be removed frmo the Javadoc in that case?
Review

I'd say it depends: When you explicitly call Objects.requireNonNull() or something similar, then no.

I've oversimplified my view point in the previous messages.
I am in favor of explicitly declaring unchecked exceptions, when:

  1. you explicitly use throw yourself
  2. you call functions whose main purpose is to check a state (i.e. Objects.requireNonNull)
  3. when there are parameters that you yourself have no restrictions on, but other functions have, and your only option would be to copy the check in the other method to ensure that they don't throw an unchecked exception
    (i.e. you have two string parameters, of which one is is the complete name of an enum class and the other is the name of one of its constants. Then I would declare the exceptions thrown both from Class.forName() and from <enum class>.valueOf() inside throws)
  4. I have to cast where I can only assume that it is the correct type.

I am only in favor of declaring something JVM-internal such as a NullPointerException when you're actually doing a null check, and never else.

I'd say it depends: When you explicitly call `Objects.requireNonNull()` or something similar, then no. I've oversimplified my view point in the previous messages. I am in favor of explicitly declaring unchecked exceptions, when: 1. you explicitly use `throw` yourself 2. you call functions whose main purpose is to check a state (i.e. `Objects.requireNonNull`) 3. when there are parameters that you yourself have no restrictions on, but other functions have, and your only option would be to copy the check in the other method to ensure that they don't throw an unchecked exception (i.e. you have two string parameters, of which one is is the complete name of an enum class and the other is the name of one of its constants. Then I would declare the exceptions thrown both from `Class.forName()` and from `<enum class>.valueOf()` inside `throws`) 4. I have to cast where I can only assume that it is the correct type. I am only in favor of declaring something JVM-internal such as a `NullPointerException` when you're actually doing a null check, and never else.
Review

If I were to implement this, multiple other methods in the code base have to be adjusted, as well as their Javadoc. Are you in favor of this?

If I were to implement this, multiple other methods in the code base have to be adjusted, as well as their Javadoc. Are you in favor of this?
Review

But first I'm interested in whether @kske overrules me.

But first I'm interested in whether @kske overrules me.
Objects.requireNonNull(listener); Objects.requireNonNull(listener);
if (registeredListeners.contains(listener)) if (registeredListeners.contains(listener))
throw new EventBusException(listener + " already registered!"); throw new EventBusException(listener + " already registered!");

View File

@ -14,13 +14,14 @@ package dev.kske.eventbus.core;
*/ */
public final class EventBusException extends RuntimeException { public final class EventBusException extends RuntimeException {
private static final long serialVersionUID = 1L; private static final long serialVersionUID = 7254445250300604449L;
/** /**
* Creates a new event bus exception. * Creates a new event bus exception.
* *
* @param message the message to display * @param message the message to display
* @param cause the cause of this exception * @param cause the cause of this exception
* @since 0.0.1
*/ */
public EventBusException(String message, Throwable cause) { public EventBusException(String message, Throwable cause) {
super(message, cause); super(message, cause);
@ -30,6 +31,7 @@ public final class EventBusException extends RuntimeException {
* Creates a new event bus exception. * Creates a new event bus exception.
* *
* @param message the message to display * @param message the message to display
* @since 0.0.1
*/ */
public EventBusException(String message) { public EventBusException(String message) {
super(message); super(message);

View File

@ -0,0 +1,25 @@
package dev.kske.eventbus.core;
mpk marked this conversation as resolved
Review

One blank line too much

One blank line too much
/**
* This unchecked exception acts as a wrapper for an arbitrary exception to prevent an
* {@link ExceptionEvent} from being dispatched. Instead, the wrapped exception is rethrown by
* {@link EventBus#dispatch(Object)}.
*
* @author Kai S. K. Engelbart
* @since 1.2.1
*/
public final class ExceptionWrapper extends RuntimeException {
private static final long serialVersionUID = -2016681140617308788L;
/**
* Creates a new exception wrapper.
*
* @param cause the exception to wrap
* @since 1.2.1
*/
public ExceptionWrapper(Exception cause) {
delvh marked this conversation as resolved
Review

What I just noticed: Do we want to allow null causes?

What I just noticed: Do we want to allow `null` causes?
Review

Well, Event Bus doesn't have a problem with it, even though it makes little sense. I would leave it.

Well, Event Bus doesn't have a problem with it, even though it makes little sense. I would leave it.
super(cause);
}
}

View File

@ -0,0 +1,33 @@
package dev.kske.eventbus.core;
import static org.junit.jupiter.api.Assertions.assertThrows;
import org.junit.jupiter.api.Test;
/**
* Tests the behavior of the event bus when an {@link ExceptionWrapper} is thrown.
*
* @author Kai S. K. Engelbart
* @since 1.2.1
*/
public class ExceptionWrapperTest {
EventBus bus = new EventBus();
String event = "This event will cause an exception";
/**
* Tests transparent rethrowing of an exception wrapper by {@link EventBus#dispatch(Object)}.
*
* @since 1.2.1
*/
@Test
public void testExceptionWrapper() {
bus.registerListener(this);
assertThrows(ExceptionWrapper.class, () -> bus.dispatch(event));
}
@Event(String.class)
void onString() {
throw new ExceptionWrapper(new RuntimeException("I failed!"));
}
}