Exceptions: Is this a good practice?

Question

This is written in PHP but it's really language agnostic.

try
{
    try
    {
        $issue = new DM_Issue($core->db->escape_string($_GET['issue']));
    }
    catch(DM_Exception $e)
    {
        throw new Error_Page($tpl, ERR_NOT_FOUND, $e->getMessage());
    }
}
catch(Error_Page $e)
{
    die($e);
}

Is nested try, catch blocks a good practice to follow? It seems a little bulky just for an error page - however my Issue Datamanager throws an Exception if an error occurs and I consider that to be a good way of error detecting.

The Error_Page exception is simply an error page compiler.

I might just be pedantic but do you think this is a good way to report errors and if so can you suggest a better way to write this?

Thanks

Answer 1

I think you'd be better off not nesting. If you expect multiple exception types, have multiple catches.

try{
  Something();
}
catch( SpecificException se )
{blah();}
catch( AnotherException ae )
{blah();}

Answer 2

Depending on your needs this could be fine, but I am generally pretty hesitant to catch an exception, wrap the message in a new exception, and rethrow it because you loose the stack trace (and potentially other) information from the original exception in the wrapping exception. If you're sure that you don't need that information when examining the wrapping exception then it's probably alright.

Answer 3

You're using Exceptions for page logic, and I personally think that's not a good thing. Exceptions should be used to signal when bad or unexpected things happen, not to control the output of an error page. If you want to generate an error page based on Exceptions, consider using set_exception_handler. Any uncaught exceptions are run through whatever callback method you specify. Keep in mind that this doesn't stop the "fatalness" of an Exception. After an exception is passed through your callback, execution will stop like normal after any uncaught exception.

Answer 4

I'm not sure about PHP but in e.g. C# you can have more then one catch-Block so there is no need for nested try/catch-combinations.

Generally I believe that errorhandling with try/catch/finally is always common-sense, also for showing "only" a error-page. It's a clean way to handle errors and avoid strange behavior on crashing.

Answer 5

The ideal is for exceptions to be caught at the level which can handle them. Not before (waste of time), and not after (you lose context).

So, if $tpl and ERR_NOT_FOUND are information which is only "known" close to the new DM_Issue call, for example because there are other places where you create a DM_Issue and would want ERR_SOMETHING_ELSE, or because the value of $tpl varies, then you're catching the first exception at the right place.

How to get from that place to dying is another question. An alternative would be to die right there. But if you do that then there's no opportunity for intervening code to do anything (such as clearing something up in some way or modifying the error page) after the error but before exit. It's also good to have explicit control flow. So I think you're good.

I'm assuming that your example isn't a complete application - if it is then it's probably needlessly verbose, and you could just die in the DM_Exception catch clause. But for a real app I approve of the principle of not just dying in the middle of nowhere.

Answer 6

I wouldn't throw an exception on issue not found - it's a valid state of an application, and you don't need a stack trace just to display a 404.

What you need to catch is unexpected failures, like sql errors - that's when exception handling comes in handy. I would change your code to look more like this:

try {
    $issue = DM_Issue::fetch($core->db->escape_string($_GET['issue']));
}
catch (SQLException $e) {
    log_error('SQL Error: DM_Issue::fetch()', $e->get_message());
}
catch (Exception $e) {
    log_error('Exception: DM_Issue::fetch()', $e->get_message());
}

if(!$issue) {
    display_error_page($tpl, ERR_NOT_FOUND);
}
else
{
    // ... do stuff with $issue object.
}

Answer 7

Exceptions should be used only if there is a potentially site-breaking event - such as a database query not executing properly or something is misconfigured. A good example is that a cache or log directory is not writable by the Apache process.

The idea here is that exceptions are for you, the developer, to halt code that can break the entire site so you can fix them before deployment. They are also sanity checks to make sure that if the environment changes (i.e. somebody alters the permissions of the cache folder or change the database scheme) the site is halted before it can damage anything.

So, no; nested catch handlers are not a good idea. In my pages, my index.php file wraps its code in a try...cache block - and if something bad happens it checks to see if its in production or not; and either emails me and display a generic error page, or shows the error right on the screen.

Remember: PHP is not C#. C# is (with the exception (hehe, no pun intended :p) of ASP.net) for applications that contain state - whereas PHP is a stateless scripting language.