1

Consider the Java code snippet below:

public <T> T create(Class<T> clazz) throws InstantiationException, IllegalAccessException {
    return clazz.newInstance();
}

This in Eclipse (Neon.2, JDK 8) with SonarLint performing static code analysis. It is providing a recommendation to refactor this:

Refactor this method to throw at most one checked exception instead of: java.lang.InstantiationException, java.lang.IllegalAccessException

What best practice is the basis for this recommendation? I understand there's some controversy about checked exceptions in general, but why would it be better in this instance to catch one here and pass the other up the stack vs. passing them both up the stack for something else to handle them?

Community
  • 1
  • 1
andand
  • 17,134
  • 11
  • 53
  • 79
  • 2
    This really is opinion-based, but my opinion is that nothing at all is wrong with doing this. If I were using SonarLint, I would switch off that warning. – Dawood ibn Kareem Mar 25 '17 at 05:03

3 Answers3

2

I agree with @David it is opinion based, though in my opinion it is best practice to throw one single exception from a method based on single responsibility principle. When multiple exceptions are thrown it sounds like the method is implemented to perform multiple tasks which is not ideally a good practice though each one of us do it that way quite often. When such a need is there it is better to throw a custom exception by mentioning the problem with an appropriate error code or error message

upendra
  • 111
  • 5
2

What best practice is the basis for this recommendation?

"Best practice" implies that there is a single objectively correct answer to the question. There isn't one.

Why would it be better in this instance to catch one here and pass the other up the stack vs. passing them both up the stack for something else to handle them?

In this case, you wouldn't do that. Both IllegalAccessException and InstantiationException are subtypes of ReflectiveOperationException. If you wanted to reduce the signature to a single checked exception (as suggested by the checker) you would use that exception.

In general, the argument for unifying (by either picking an existing superclass or by redesigning the exception hierarchy) is that the caller needs to handle fewer exceptions.

But the counter-argument to that is that when you unify the throws list, you are hiding information from the programmer / compiler. For example if the API was changed to this:

public <T> T create(Class<T> clazz) throws ReflectiveOperationException {
    ...
}

programmer using that API no longer knows which kinds of reflective operation exception can be thrown. And when you "unify" by catching, wrapping and throwing as a new exception, it is even worse.

Note that I'm not saying that either approach is wrong. What I'm saying is that the context should determine which approach you take. In other words, appealing to "best practice" is missing the point.

Stephen C
  • 698,415
  • 94
  • 811
  • 1,216
-1
public class IllegalAccessException
extends ReflectiveOperationException

An IllegalAccessException is thrown when an application tries to reflectively create an instance (other than an array), set or get a field, or invoke a method, but the currently executing method does not have access to the definition of the specified class, field, method or constructor.

Gayan_Samuditha
  • 29
  • 2