4

I have inherited a codebase which contains code such as the following (note: example code is PHP):

try {
    // Do something which doesn't intentionally throw exceptions.
} catch (\Exception $e) {
    $this->log->log($e->getMessage());
    $this->product->setError($e->getMessage());
    return false;
}

So essentially, the code is catching the exception. Logging it, and the failing silently (apart from the log message).

This behaviour seems to make sense in production, but makes development much more difficult (as stack traces have to be looked up in log files, rather than being printed to the console). So I have come up with the following function:

private function tryCatch ($func) {

    // Bind closure, so that $this allows it to access class properties
    if (is_object($func) && ($func instanceof Closure)) {
        \Closure::bind($func, $this, "static");
    }

    if (\App::environment('test')) {
        return $func();
    } else {
        try {
            return $func();
        } catch (\Exception $e) {
            $this->log->log($e->getMessage());
            $this->product->setError($e->getMessage());

            return false;
        }
    }

}

which can then be used like so:

$this->tryCatch(function () {
    // Do something
});

This code special-cases the 'test' environment which it calls the passed in function without exception handling (so any exceptions remain unhandled). In every other environment (e.g. production), it wraps the passed in closure in a try-catch block in production behaves as the code originally behaved.

This solutions solves my problems, but it seems a bit hacky, and leaves me with aI have nagging feeling that it isn't a good idea.

My question: Is there any reason why I shouldn't do this? Or is there a better way of handling this situation?

Nico Burns
  • 16,639
  • 10
  • 40
  • 54

1 Answers1

13

Don't try to reinvent the wheel with regards to exceptions. There is one and only one scenario in which you should catch an exception:

Catch an exception if you have an alternative plan what to do with it.

An exception means that your code encountered an exceptional condition in which it cannot continue its work and has no choice but to throw in the towel. That's a perfectly good way to abandon a function/module/execution context and signal to the caller higher up what happened. That's exactly what exceptions do.

During development you want to see the exception in all its ugly glory to be able to debug. In production you do not want your users to see exceptions, but instead present them with a nice looking error screen and/or have all sorts of bells and whistles go off which notify the admin/developer/CTO/whoever.

That means, in production you only want one global error handler which responds appropriately if an unexpected, uncaught exception occurs. The exception should be thrown and (not) caught exactly as in development, you do not want two completely separate code paths. This global error handler could be set conditionally by some bootstrapping script simply with set_exception_handler; or probably even better, you configure your web server appropriately to serve useful error pages. Configuring the web server is the best possible way, since this is a system specific setting (production only) without any need to change anything about your code.

The only time you should actually write a try..catch is if there's a legitimate reason a subsystem might fail and you have a backup plan. E.g.:

try {
    $file = download_file_from_url($url);
    echo "Cool, got your file.";
} catch (HttpNotFound $e) {
    echo "Hey user, that file doesn't exist.";
} catch (HttpEmptyResponse $e) {
    echo "Hey user, that file seems empty.";
}
..

In this scenario a failed HTTP download is an expected outcome and can be handled well with exceptions, so this is a good use case. But don't reflexively try to catch them all even if they don't represent expected outcomes.

deceze
  • 510,633
  • 85
  • 743
  • 889
  • Thank you, that more or less confirms what I thought. I think the `try..catch` is there in order to log the error to a subsystem-specific log file. Does that seem like a reasonable usage (in production)? – Nico Burns Jul 22 '15 at 13:42
  • 2
    "Just logging" should happen by a global error handler. If that's all you're doing, you shouldn't be doing it manually for this specific case, but have something in place that *always logs all errors*. Only if an exception is an *expected outcome* and you have something else that you can do in that case should you catch the exception. If `$this->product->setError` does something useful to "recover" from the exception is this a valid use case. – deceze Jul 22 '15 at 13:45
  • Note Python is different: we use exceptions to control program flow. Indeed, in the internals of Python it actually uses a `StopIteration` exception to know it is time to stop a for loop. Other languages have different conventions for exceptions. In Python they are part of program flow. Discussion here (but also just Google): https://stackoverflow.com/a/3087143/1886357 – eric Dec 12 '17 at 13:53
  • @neuro True, Python is using exceptions much more liberally. The `StopIteration` makes sense for things like `next(foo for foo in bar if foo == baz)` – it can be considered exceptional if nothing was found. Other than that somewhat unintuitive use of exceptions, Python's exceptions are all very well used IMO. The *ask-for-forgiveness* idiom is simply very pythonic. – deceze Dec 12 '17 at 14:01
  • 1
    @neuro Note that "exceptional" doesn't mean it has to be *catastrophic*. If a function simply cannot return the expected return value, that is an *exception* in its design and it should raise an exception. The more narrow you define a function, the more reasons it will have to raise exceptions; and that's often the case in Python. – deceze Dec 12 '17 at 14:05
  • @deceze yes, I was trying to bring attention to Python's focus on 'ask for forgiveness, not permission' versus the alternate 'look before you leap' styles. In Python, there is an attitude of: "Just try it, and if it doesn't work (i.e., an exception is thrown), do something else." This is actually encouraged in Python, but would evince horror in C developers. (This is all codified in the `try/except/else` pattern). – eric Dec 12 '17 at 14:12