Several catch blocks or one with dynamic_cast?

Question

We have a hierarchy of exception classes - there's a GenericException class and a number of classes that derive from it. GenericException is polymorphic - it has a virtual destructor.

One of derived classes is FileException thrown to indicate errors when manipulating with filesystem objects. FileException has GetErrorCode() method that returns the code indicating what actually went wrong.

The hierarchy is already there, I can't change it, so it's out of the question.

Exceptions are thrown from code we don't control and they are always thrown with new keyword so we can only catch them by pointer - this is out of the question either.

Now I have a situation when a set of statements can generate an exception which is guaranteed to be derived from a GenericException. If it is a FileException and the type of error situation is "file not found" I want to do nothing, otherwise I want to show an error message and then I consider the exception handled anyway.

I have two options - either:

} catch( GenericException* e ) {
    FileException* fileException = dynamic_cast<FileException*>( e );
    if( fileException == 0 || fileException->GetErrorCode() != FileNotFound ) {
        ShowMessage( e );
    }
    delete e;
}

or this one:

} catch( FileException* e ) {
    if( fileException->GetErrorCode() != FileNotFound ) {
        ShowMessage( e );
    }
    delete e;
} catch( GenericException* e ) {
    ShowMessage( e );
    delete e;
}

I prefer the first one because it reduces code duplication. But I've heard an opinion that since C++ allows for selective catch blocks (like in the second option) I should not use dynamic_cast - selective catch blocks make no need for it.

Which option is better? What might be the reason to not the first option here?

Answer 1

You should never,ever, throw pointers. Always throw values and catch by const references. It is almosr never possible to clear up a dynamically allocated exception object correctly.

To answer your question, I personally don't believe in having complex exception heirarchies. If you need to perform conditional processing on the exception, you need to handle the exception much nearer to the throw site, where you have more contextual information, than you currently appear to be doing.

Answer 2

Second option is the "best" if you rewrite it with const & ;-)
I find the second much cleaner than the first one, it clearly indicates what you want.

Answer 3

I would prefer the second one.

If you're concerned by the duplication, are you aware something called an "exception dispatcher"? If you find many places where you need to perform the same catch handling, you can stick it all in one function and call this function from a catch(...) block.

e.g.

void fn()
{
    try
    {
        MightThrow();
    }
    catch (...)
    {
        ExceptionHandler();
    }
}

void ExceptionHandler()
{
    try
    {
        throw;
    }
    catch( FileException* e ) {
        if( fileException->GetErrorCode() != FileNotFound ) {
            ShowMessage( e );
        }
        delete e;
    } catch( GenericException* e ) {
        ShowMessage( e );
        delete e;
    }
}

It won't stop duplication within the handlers but it will stop duplication where you need to catch the same things in different places.

Edit: You must ensure the ExceptionHandler is not called when not handling an exception otherwise much badness will occur: related question

Answer 4

You need virtual function to show message from your exception. This function must be member of your root hierarchy class.