39

I have two options (which technically are the same, as I understand) to declare a custom exception class thrown only from a particular class com.XXX.Foo:

  • as a public class in the package: com.XXX.CustomException
  • as a public static inner class: com.XXX.Foo.CustomException

Which option is better?

Visruth
  • 3,430
  • 35
  • 48
yegor256
  • 102,010
  • 123
  • 446
  • 597
  • 1
    The same question and options could be applied to any class. Is there a characteristic of exceptions which makes them particularly suited to nesting, or is the question simply asking [when to use nested classes?](https://docs.oracle.com/javase/tutorial/java/javaOO/whentouse.html) – jaco0646 Aug 09 '16 at 14:50

5 Answers5

19

In case the exception is very specific to Foo class, I don't mind keeping it as a public Nested class. And whenever it seems like a time to extract that out, simply extract that out.

In general practice, I have never seen any nested class defined for Exception, though. I also don't know if one exist in Java API.

Adeel Ansari
  • 39,541
  • 12
  • 93
  • 133
12

In my 10+ years experience with Java, I cannot recall coming across an API where a public exception class was defined as a static inner class. I cannot give you a specific reason why it would be a bad idea to do this, but it would certainly make your code unusual.

Why do you feel that it is necessary to do this, given that (apparently) nobody else does? I hope you are not just doing it to be "innovative".

(BTW, I know that some well-known Java APIs use public static inner classes and interfaces for other things. I'm specifically talking about the case of exception classes here.)

Stephen C
  • 698,415
  • 94
  • 811
  • 1,216
  • 2
    Take a look at this class from Apache Lucene 3.5: [org.apache.lucene.search.BooleanQuery.TooManyClauses](http://lucene.apache.org/core/old_versioned_docs/versions/3_5_0/api/all/org/apache/lucene/search/BooleanQuery.TooManyClauses.html) – yegor256 May 01 '12 at 09:18
  • 1
    OK. One example in 10+ years. (But why did they do that?) – Stephen C May 01 '12 at 14:05
  • 2
    Well, I think that it makes sense when exception is thrown _only from one class_. In that case it should be defined as a static inner class. When you remove the class - the exception goes off too. In other words, they will be "tightly coupled". – yegor256 May 01 '12 at 15:11
  • @yegor256 - and that breaks if you decide to throw the exception in a different class. Or to put it another way, nesting the exception makes it less reusable. I definitely disagree with "should". – Stephen C May 01 '12 at 15:22
  • 3
    yes, that's the purpose of such exceptions (you just defined it) - make tham non-reusable by design. When we want to keep the exception closely related to the class (without any ability to be reused) - make it static inner. Makes sense? – yegor256 May 01 '12 at 15:50
  • @yegor256 - Yes and No. What does not make sense is deliberately making exceptions (or any other class) non-reusable by design. That seems wrong-headed. – Stephen C May 01 '12 at 23:02
  • 5
    @Stephen C: No, it's really a good idea. Let's make a little detour: In java.util.HashMap, you have an inner class called Entry. Why do you think it's an inner class? Because it's closely related to HashMap, useless outside of the context of a HashMap and probably misleading if used anyway. Embed it inside of HashMap also creates a complete packaged data structure in one single "unit". The same can apply for an exception. – Alexis Dufrenoy Nov 20 '14 at 10:48
  • Yes. But why is it a good idea to do this? What are the tangible benefits? In the case of `Map.Entry`, the idea is to flag that `Entry` interface should not be reused for unrelated purposes. Why would you want to do that for an exception? – Stephen C Sep 26 '16 at 13:30
9

I can definitely think of situations where I'd prefer the exception to be a static inner class than merely a class in the same package.

The reasons not to do so seem to be:

  • Coupling the exception to the class that throws it makes it inappropriate to reuse in other contexts
  • Nobody else does it

I do not find either of those arguments at all convincing.

For the first point, why should this hypothetical future opportunity for re-use arise in the same package? This argument leads to the conclusion that we should put all exception classes as high as possible in the package hierarchy so that when we discover a future opportunity to reuse the same exception we don't have to introduce a dependency on where it was originally defined.

But even without the "taken to extremes" point, consider an exception intended to convey that class Foo was given wrong input. If I call it Foo.InvalidInput, the name is short and the association with Foo is impossible to miss. If I put it outside the Foo class and call it FooInvalidCriteria, then I can't reuse it from class Bar anyway, without changing its name (equivalent to changing its location).

But worst is if I leave it outside Foo and keep its name general like InvalidInput. Then when I later realise that Bar might have invalid input too and make it start throwing this exception. Everything compiles and runs fine, only now all the places that were catching InvalidInput and assuming they were handling errors from Foo could now also be handling errors from Bar if Foo happens to use Bar internally in a way that could cause this exception to be thrown. This could easily cause code breakage.

The reality is that taking an exception that was previously conceived as specifically indicating a situation that arises in one class and re-using it as a general error class is an interface change, not just an internal implementation change. To do so correctly in general you must revisit all the sites where the exception is caught and make sure they're still correct, so having the compiler tell you about all the use sites (because you have to change the name and/or import path) is a good thing. Any exception that you might make a static inner class is inappropriate for reuse in other contexts no matter whether you actually make it an inner class or not.

And as for the second dot point... "nobody else does it" never bears on anything. Either it really is the wrong thing to do, so there will be other reasons not to do it, so the "nobody else does it" argument is unnecessary. Or it isn't. And it's not like this particular example would even be terribly complicated and hard to understand, so not even the "it's unexpected so people will have trouble following it even if it's a good idea in theory" argument is very strong.

Ben
  • 68,572
  • 20
  • 126
  • 174
3

I'd prefer the (not necessarily public) class within the same package, as a package is a logical group of classes depicting a business model, which the exception belongs to as a technical part.

A user will see immediatelly that there's an exception when he looks at the package and does not need to read the file of class foo, which is better for maintenance and clarity/readability/comprehensional reasons. It's very good to define custom exceptions and to tell the API-user about it!

I'd only use an inner class when it's clearly a private thing of the class in question.

Nevertheless, we're talking here about a mainly conventional issue!

Falcon
  • 3,150
  • 2
  • 24
  • 35
  • How about a class extending `javax.swing.RowFilter.Entry`? – Adeel Ansari Nov 15 '10 at 10:58
  • 2
    Packages are used to group logical models, so in my team we'd agree on the convention that each class gets its own file. The business model in the package is most likely coupled pretty tight, so I can see no benefit in using a nested class. The nested class is pretty much expressing: I inseparably belong to this other class. For some APIs it may be good to design it like that, for example if the whole model inside the package is coupled kinda loose. – Falcon Nov 15 '10 at 11:04
  • 2
    You CAN extend static inner classes. – Jim Tough Nov 15 '10 at 11:05
  • 1
    @Falcon: `The static inner class also isn't very object oriented, since you can't inherit from static classes, so from an architectural design perspective I'd leave out at least the static....` Completely wrong. – Adeel Ansari Nov 15 '10 at 11:06
  • Well, but static isn't very inheritance-friendly, that's what I wanted to express. I'm sorry if you got that wrong. Even if you could extend that, it'd be a mistake by design! – Falcon Nov 15 '10 at 11:09
  • @Falcon: I know where you are coming from. But you must understand that, `static` on a class works in totally different way. See this thread for clarification, http://stackoverflow.com/questions/70324/java-inner-class-and-static-nested-class . – Adeel Ansari Nov 15 '10 at 11:11
  • Oh, I see now, thanks for the link! This has a total different meaning in Java than in other languages. By this definition, I'd totally go for the static inner class if I needed one and prohibit non-static inner classes, as they seem to make things more complex. – Falcon Nov 15 '10 at 11:18
  • @Falcon: I don't find any design problem with this, `javax.swing.RowFilter.Entry`. Well, not at least its nestedness. – Adeel Ansari Nov 15 '10 at 11:19
-3

Exceptions as inner classes is a bad idea in general because by nature they may be raised through various levels, even in simple architectures. The exception class being raised must be referenced from its containing class, and if that class is unknown to the classpath something bad would probably happen like a ClassCastException.