Sometimes I would want to throw an exception with more information to the user, so they can easily see why the method failed.

My method would look like this:

public myPublicMethod(...) throws Exception1, Exception2 {
  try {
    doSomething(); // this throws OtherException
  }
  catch (OtherException e) {
    Exception cause = e.getCause()
    if (cause instanceof Exception1) {
      throw (Exception1) cause;
    }
    if (cause instanceof Exception2) {
      throw (Exception2) cause;
    }
    throw new IllegalStateException("some other type of exception occurred", cause);
  }
}

Is this design acceptable?

CodePudding user response：

Your thinking is good, however in my humble opinion the way you implemented it is kind of missing the point, I'll try to explain.

You started with the claim that you want to give the user more information about the exception. What you did - striping the cause from its "parent exception" (e.getCause() and then throwing the cause) - actually gives the user less information, since the information in OtherException, that you discarded, might contain useful details about the flow caused the program to crash.

1. It is a good practice to catch the most specific exception and throw it to the caller, but make sure that you're not omitting details. Throwing the cause without the parent exception is actually omitting details.
2. It makes sense sometimes to wrap an exception that you caught and provide more details about it (like you intended to do in your last line), but again - make sure that you're only adding details, and not omitting details.

You can read more about exceptions handling best practices here.

CodePudding user response：

I would want to throw an exception with more information

There's nothing wrong with the practice of catching an exception in order to provide additional information by wrapping it with a custom or built-in exception and re-throwing.

I think it's if you want to do it only for a limited number of types of exceptions by wrapping with the IllegalStateException and adding a custom message and preserving the cause of exception. But there's a caveat: a type of OtherException shouldn't be too broad, like general Exception, so that it will encompass only a limited number of exceptions types.

But re-throwing the only a cause like you've done for Exception1 or Exception2 looks suspicious. For some reason, you don't consider them to be the root-cases and going to blind on the information they could carry. It sounds like a design flaw. Also keep in mind that cause could be null (in this case, exception doesn't wrap anything) and this code will fail with NullPointerException. Therefore, in the in example below, the same exception will get re-thrown instead of re-throwing the cause.

From the clean-coding perspective, it's better to use as fewer type-checks as possible. And I think in this case you don't need to resort to instanceof check.

Instead, you can utilize the features that exception handling mechanism provides.

Since you need to perform the same action (re-throw the exception that was caught) if Exception1 or Exception2 occurs, if types Exception1 or Exception2 are siblings (i.e. they don't extend each other), these exceptions can be handled with a multi-catch clause to avoid code duplication:

catch (Exception1 | Exception2 e) {
    throw e;
}

Otherwise, if let's say Exception2 is a subtype of Exception1 then you should handle them in the separate catch blocks starting from more specific type, i.e. subtype should be handled before supertype:

catch (Exception2 e) {
    throw e;
}
catch(Exception1 e) {
    throw e;
}

And after that add a catch clause with the least specific type (more wide than all exception types defined previously), to handle all other exception types that might take place by throwing a new instance of IllegalStateException.

Example:

public void myPublicMethod(Path source, Path destination) 
            throws FileAlreadyExistsException, DirectoryNotEmptyException {
    try {
        Files.copy(source, destination);
        // do some other actions
    }
    catch (FileAlreadyExistsException | DirectoryNotEmptyException e) {
        throw e;
    }
    catch (IOException e) {
        throw new IllegalStateException("some other type of exception occurred", e);
    }
}