views:

92

answers:

7

I find that Java's reflection API is exception verbose, and I often want to catch each specific exception, but just throw Exception. Is this poor practice, or do you really throw all of those exceptions on the method signature? Every method that calls that method must then deal with each of those specific exceptions in some way. Instead, I'm thinking of doing this:

public void someMethod()
        throws Exception {

    try {

        // do a bunch of reflection...

    } catch(ClassCastException classCastException) {

        throw new Exception("Some specific message.", classCastException);

    } catch(InvocationTargetException invocationTargetException) {

        throw new Exception("Some specific message.", invocationTargetException);

    }
}

Is that poor practice?

+9  A: 

Rethrowing different kinds of exceptions that a method throws as a single unified exception can sometimes be a reasonable approach.

Using java.lang.Exception specifically for this is a bad idea, because it's too unspecific.

You might rethrow each different exception as some custom MyException or something like that. That's sometimes a reasonable approach when you're not really interested in why someMethod failed.

Joachim Sauer
A: 

I would in some cases.

The normal way I would decide is look at what methods calling someMethod() would expect or reasonably be expected to know. So if someMethod() was doing something IO related I'd throw an IOException, if it was related to a library I was writing I might throw a custom exception..

I'd only throw the individual exceptions if the callers of someMethod() would reasonably be expected to know it used reflection for its implementation.

I guess the key question is "If someMethod() didn't use reflection, and something went wrong, what would it do?" That will normally give you the answer.

Nick Fortescue
A: 

Thats still an acceptable policy if you are collecting all exceptions and throwing a custom exception of yours.

public void someMethod()
    throws MyCustomException {

try {

    // do a bunch of reflection...

} catch(ClassCastException classCastException) {
    MyCustomException ex1 = new MyCustomException("Some specific message.", classCastException);
// Logic to add more details to exception AND/OR handling current exception.
    throw ex1;

} catch(InvocationTargetException invocationTargetException) {

            MyCustomException ex1 = new MyCustomException("Some specific message.", classCastException);
// Logic to add more details to exception AND/OR handling current exception.
    throw ex1;

}
}

Generally catching exception should be for a reason of handling them in some way. Simply re-throwing an exception of larger scope makes no sense anywhere to me.

Tushar Tarkas
+2  A: 

I agree with your approach, except I think throwing Exception is not good, eventually every method declaration in your application will end with "throws Exception". For cases like this, where the exception will only occur if you made an error, it's better to make the wrapping exception an unchecked exception.

If the exception thrown by the method (contained in the InvocationTargetException) have the potential to be interesting to you, you could make it the wrapper a different subclass of unchecked exception or create a specific checked exception subclass for it, but wrap things like the ClassCastException in unchecked exceptions.

Nathan Hughes
Thanks! I was thinking of something similar, much like what jwachter suggested. I would basically throw a custom exception, or throw a runtime exception, depending on the exception type that was caught.
hal10001
+1  A: 

To use throws Exception is always a bad idea. In your case you have to consider the following things.

  1. Can I recover from the error at the next imediate level? If so use all the different exceptions for clarity.
  2. Can I recover from the error some levels up? If so use a wrapper exception because it would pollute all methods on the way up with technical details to use the explicit ones.
  3. I can't recover from the error or it happens in a completely different place/many levels above. If wrap each exception into RuntimeException or a custom ReflectionFailedRuntimeException to avoid technical exceptions leaking throughout your application.

Alos, normally you should code it in a way that you can expect the reflection to work in most cases without Exceptions, so it would be also considerable to make it RuntimeException wrapping anyway.

Johannes Wachter
Thanks! Although I can see the benefit of using a custom exception that extends RuntimeException, I can't imagine a scenario in which I would be able to recover from a reflection operation unless it was an internal process.
hal10001
@hal10001: why? Showing a note to the user that reads "Sorry, we are unable to fulfill this request at the moment, do you want to ..." might be a reasonable solution to such a problem (depending on the application).
Joachim Sauer
@hal10001: Exactly. That's why you normally just do the wrap into a _RuntimeException_ because you cannot recover in a senseful way. You just need to make sure that the Exception provides a useful message to the user and/or logs that and how it occured.@Joachim Sauer: Showing a message in a dialog that masks the real error is not recovering :) Recovering would be for example to solve the problem and providing the user with the result _without_ showing that an error even happened.
Johannes Wachter
You should expect reflection to fail in a variety of ways, and you should handle it appropriately. Better of course, is to avoid using reflection.
Tom Hawtin - tackline
+1  A: 

Think of what you want to achieve. The caller of your method is not supposed to know you're using reflection to achieve your goal. That's none of its business. If you would just put all those exceptions in your throws clause that would break that encapsulation.

Catching these exceptions and wrapping them in your own is therefore the way to go. Then you just have to decide which exception to throw. Exception itself is a very bad idea (I'm of the opinion it should even be abstract to suggest that) The type of the exception should be related to the problem.

Throw an exception that relates to what you're trying to do and to why it failed (if it failed because a given argument is inappropriate, IllegalArgumentException would be one). Also make an informed decision on whether you would like to throw a checked or an unchecked exception (RuntimeException).

Joeri Hendrickx
+3  A: 

Short answer

  • Reflection is verbose by design
  • Exception translation is idiomatic (catching lower level exception and abstracting it to a higher level)
    • ... but don't overuse!
  • Prefer custom exceptions to java.lang.Exception; it's probably too high of an abstraction

Point 1: Avoid reflection if at all possible

Here's a quote from Effective Java 2nd Edition, Item 53: Prefer interfaces to reflection:

[The power of reflection], however, comes at a price:

  • You lose all the benefits of compile time checking, including exception checking.
  • The code required to perform reflective access is clumsy and verbose. It is tedious to write and difficult to read.
  • Performance suffers. Reflective method invocation is much slower than normal method invocation.

[...] As a rule, objects should not be accessed reflectively in normal application at runtime. There are a few sophisticated applications that require reflection. Examples include [omitted on purpose]. If you have any doubts to whether your application falls into one of these categories, it probably doesn't.


Point 2: Exception translation can be a good thing, but don't overuse

Here's a quote from Effective Java 2nd Edition, Item 61: Throw exceptions appropriate to the abstraction.

It is disconcerting when a method throws an exception that has no apparent connection to the task that it performs. This often happens when a method propagates an exception thrown by a lower-level abstraction. [...]

To avoid this problem, higher layers should catch lower-level exceptions and, in their place, throw exceptions that can be explained in terms of higher-level abstraction. This idiom is known as exception translation.

[...] While exception translation is superior to mindless propagation of exceptions from lower layers, it should not be overused. Where possible, the best way to deal with exceptions from lower layers is to avoid them, by ensuring that lower-level methods succeed. [...]

If it is impossible to prevent exceptions from lower layers, the next best thing is to have the higher layer silently work around these exceptions, insulating the caller of the higher-level method from lower-level problems. Under these circumstances, it may be appropriate to log the exception using some appropriate logging facility. This allows an administrator to investigate the problem, while insulating client code and end user from it.


Point 3: java.lang.Exception is way too general

Looking at the documentation, one can see a long list of direct known subclasses, with a wide-ranging topics from Reflection, RMI, XML parsing, I/O, GUI, cryptography, etc.

Declaring that a method throws Exception is probably a poor API design decision, because it does not immediately tell the user anything useful about what category the exceptions may be, or under what circumstances they may be thrown. Contrast this with a hypothetical ReflectionException; this is a lot more informative, and it also allows user to catch specifically ReflectionException and not, say, an IOException that somehow slipped through.


Related questions

polygenelubricants
I'm actually interested in Point 1, and a pattern substitution for reflection. Can you offer any links or information? Thanks.
hal10001
@hal10001: You can ask another question with more information about `// do a bunch of reflection...` and see if there's a better alternative. EJ2 quote: "create instances reflectively and access them normally via their interface or superclass".
polygenelubricants