Exceptions - am I using them correctly?

[Pain]

Hello. Been programming OOP for the past year. I am very ashamed to admit that I have not learned about exceptions yet. Well actually Im not sure...

 

Am I using them correctly?

 

public function indexAction(Request $request)
{
        try {
            if(!is_object($request))
                throw new \Exception('Param msut be an object');
        } catch(Exception $e) {
            $this->logger = $this->get('logger');
            $this->logger->info("You have the following error: { $error }", array("error" => $e->getMessage()));
        }
}

I throw an exception in order for the error to appear on the screen. Then I catch the error and log it. Is this the correct usage? Or is there more? 

Thanks!

 

[requinix]

That code? You're throwing an exception only to catch it on the next line. That's pointless. You might as well just

public function indexAction(Request $request)
{
        if(!is_object($request)) {
            $this->logger = $this->get('logger');
            $this->logger->info("You have the following error: { $error }", array("error" => "Param must be an object"));
        }
}

By the way, inconsistent indentation drives me crazy.

 

I'm trying not to go into a treatise about how exceptions are supposed to work (too much to say that has been said elsewhere on the internet), with mixed success, but basically you use them if there is something unexpected that happens and your code is not able or supposed to handle it. Look around to see what people say about how to use exceptions - even if it applies to other languages (notably Java and C#).

 

First of all, in your situation $request is going to be an object because you used the typehint. Not that it will prevent someone from calling the method with a non-Request or non-object, but if they did then there will be an error about it and your logger will see that (assuming your logger is set up for PHP errors, which it should be).

 

If you still want exceptions here then I'd use something more like

public function indexAction(Request $request)
{
	if (!($request instanceof Request))
	{
		throw new InvalidArgumentException('$request must be a Request object');
	}

The main point here is that indexAction cannot recover from this problem. There is nothing it can do. The Exception "forces" someone in the call stack to handle an InvalidArgumentException - or worst case nobody does and PHP fatals.

 

Also note the InvalidArgumentException instead of just Exception. Pretty much every sort of exception you'd want to throw really, really needs to be a subclass. Throwing Exception everywhere severely limits what you can do when catching them:

(a) You would have to resort to crazy hacks to figure out what kind of problem it represents, but more critically

(b) You'll catch and handle types of circumstances your code isn't prepared for. If you set up a try/catch because you know that (eg) the function you're calling can error because a file doesn't exist, you'll get that, but you'll get every other exception too. And if you discard the exception because the missing file isn't a big problem, you're now discarding every exception.

 

The InvalidArgumentException class specifically indicates that there was a problem with an argument to a function. I don't ever catch this class[1] because it represents a fundamental problem with the code itself - not merely some sort of unexpected situation or unusual circumstance.

 

Continuing that train of thought, only catch the exceptions you want to handle. If something can throw an exception but your code doesn't know what to do in that case, don't catch it there. Instead, go backwards up the call stack until you find a place where it makes sense. Common example: set up a try/catch just before you try to "execute" the code for a particular page on your site, then in case of an exception you can (log it and) show an error message to the user.

 

So

public function indexAction(Request $request)
{
	// do whatever

public static function /* Controller:: */ invokeAction($controllerClass, $action)
{
	$controller = new $controllerClass();
	$method = $action . "Action";
	$request = Request::current();

	// no try/catch because there isn't any type of exception this method is prepared to handle
	$controller->$method($request);
}

try
{
	Controller::invokeAction($controllerClass, $action);
}

// one type of exception that you want to use special handling for
catch (SomeParticularTypeOfException1 $e1)
{
	// ...
}

// another type of exception with special handling
catch (SomeOtherTypeOfException2 $e2)
{
	// ...
}

// catch every other exception so we can present an error message to the user
catch (Exception $e)
{
	ErrorController::showErrorPage(500, $e); // public static function showErrorPage($code, Exception $error = null)
}

[1] Unless I'm working with a system that must not fail[2], I do this. Which is to say I have a logger set up for uncaught Exceptions, log their presence, then let PHP die.

[2] Well, that or there's a chokepoint in the code where I want to wrap any exceptions caught inside another exception. In which case I use the "$previous" parameter to Exception, like

try {
	callSomeFunction();
} catch (Exception $e) {
	throw new BadMethodCallException("callSomeFunction failed: " . $e->getMessage(), 0, $e);
}

Side note: make sure your logger can handle nested exceptions.

[Jacques1]

No, you're not using exceptions correctly.

 

It would be insane to write a separate try statement for every single action that could possibly go wrong. You'd have to clutter your code with thousands of statements all doing the exact same thing. Actually, the whole point of exceptions is that you do not have to manually check for errors all the time: Since exceptions “bubble up” the call stack, you can just leave them alone. Either catch them on a higher level or let the PHP error handler do its job (which is what you usually want).

 

Secondly, exceptions are just a small subset of all possible errors. As you're probably aware, PHP wasn't designed as an object-oriented language. It started out as a purely procedural language, and a lot of core functions rely on notices, warnings etc. rather than exceptions. So your logger needs to handle all kinds of errors, not just exceptions.

 

Since this is clearly a global error strategy which you want to apply to your entire application, the right approach is to set up an error handler. This allows you to specify how PHP should handle errors. Besides the logging, you'll also need to make sure that the user gets an appropriate HTTP code and sees an error page (but do not print any internal PHP errors on the screen; it's not the job of your users to debug your code).

 

Besides that, your try statement makes no sense. Always catch and throw specific exceptions (e. g. InvalidArgumentException), and always terminate the script if there's no way to recover from the error. If you keep running after an error (like you currently do), you're likely to cause havoc. As a more extreme example:

<?php

// DO NOT USE THIS

try
{
    do_something_critical();
}
catch (Exception $error)
{
    // your error logging stuff
}

// Script execution continues

Since Exception is the superclass of all exceptions, the above try statement will catch every exceptions that can possibly exist. It could be anything from “There's a tiny problem with an argument.” to “Russian hackers have pwned your server, need to shut down immediately!”. Do you really want to take responsibility for every possible error (most of which you don't even know), just log it and then keep going? That would be insane.

 

Don't use Pokémon exception handling. Unless you have a specific error handling strategy for a specific exception, just leave it alone.

Edited November 4, 2015 by Jacques1