Why? "Always declare user defined exceptions as final"

Question

I analyzed the code I am working on using a Java source code analyzer. One of the warnings is "Always declare user defined exceptions as final". There are many other warnings that doesn't make much sense but this one made me a little bit confused.

I am working on a framework and I have one root generic exception (say FrameworkGenericException) and for other exceptions I am simply deriving them from the root exception. So I have a hierarchy of exceptions for the framework. I might extend the hierarchy but this warning I think tells me not to have such hierarchy but define them individually. So what which way should I go, what are your comments?

Answer 1

I've never hear of this recommendation, nor does it make any sense to me.

Which tool are you using? I tried Googling for that message and got ZERO hits apart from your SO question. Trying other searches in attempt to "exploit the wisdom of the net" gave me nothing obviously relevant either.

Perhaps you should ask the code analyzer tool authors to explain the rationale for this style rule ...

Answer 2

Strange. Google shows only one document with such words - your question :)

In my projects this is quite normal to have user-defined ancestor(s) for all exceptions. Few years of development in this style, nothing special so far linked to that.

Answer 3

The thing is, a static analysis tool basically scans your code for violations of (what the author of the tool considers to be) "best practises". In this case, it's usually considered a "best practise" to have very shallow exception hierarchies.

The reason is that there's basically two times when you'll catch an exception. The first, is you want to catch the exception because you know how to handle it. For example:

try
{
    return fetchDataFromFile();
}
catch(FileNotFoundException)
{
    return defaultData;
}

In this case, you caught the FileNotFoundException because you want to return "default data" when the file isn't found. In this situation, you typically catch a specific exception class - because that's the only type of exception you know how to handle (for example, if fetchDataFromFile throws some other exception, you don't want it to return default data.

The other time you'll catch exceptions is at the very root of your thread,

try
{
    doStuff();
catch(Exception e)
{
    logException(e);
    notifyUser(e);
}

In this case, you catch the root exception class, because you specifically want all exceptions so that you can log them, or notify the user.

There are very few situations where you need to do anything else.

Now, remember that this is a static analysis tool. It's job is to just flag "potential" problems to you. If you believe your situation is somehow different, then the idea is that you document why you think it's different and specifically disable that warning for that section of code (or that class or whatever).

Answer 4

This is probably standard practice to them: declare classes as final if they are not supposed to be inherited from, and also they probably think that all of your exceptions won't be extended from.

However, in your case I think that it is a good thing to make one generic exception and extend all others from it. Say, you have:

public void frameworkMethod() throws IllegalDataException, InvalidCommandException;

where these exceptions are sublasses of FrameworkException. Then you could handle exceptions a little bit easier.

try {
    frameworkMethod();
} catch (FrameworkException ex) {
    logger.log(Level.SEVERE, "An exception occured!", ex);

    if (ex instanceof IllegalDataException) {
        // Do something to recover
    } else if (ex instanceof InvalidCommandException) {
        //Inform the user
    } //And so on...
}

So I'd say, you're doing the right thing, the architecture of your program will be easier to work with.

Answer 5

I'll bet that message goes away if you make your generic exception abstract. You'll be following another well-respected object-oriented guru, Scott Meyer:

Make non-leaf classes abstract.

Personally, I disagree with a root exception for your whole framework. What extraordinary behavior have you embedded in it that all children will inherit over and above what comes from java.lang.Throwable?

I think having SpecialBusinessException overriding the four constructors from java.lang.RuntimeException is plenty. What's the point of the hierarchy? I'll bet it's flat and wide, with one root class. Why bother?