error handling logic in php [closed]

Question

I'm wondering if you see anything terribly wrong with this code, or if you see any areas for improvement. My objective is to make sure its simple enough for someone else taking over my job to be able to pick up fairly quickly.

My basic question has to do with the fact that in my model, I'm trying to distinguish between system errors that I want to show the end user, and ones that I don't want them to see. Here's some code from my model:

 public function macaddresses($ip, $name)
 {
   try
     {
      //some code....
  if ( $obj->connect() )
  {
           //do something else
       return $macaddies
      }
  else
  {
      throw new Exception('Connection');
  } //end if                                   
   }
   catch (Exception $e) 
   {
   $this->error_handler($e->getMessage());
   return false;
    }//end catch
  }//end function

private function error_handler($emess)
{
    switch ($emess)
    {
        case 'Connection':
             $userfriendlyemess = "Unable to connect to device. Please contact administrator!";
             break;

       //handling unknown errors    
       default:
            $userfriendlyemess = "Oops!  Something big just happened.";
            log_message('error', $emess); //write to ci logs
            break;
    }// end switch
    throw new Exception($userfriendlyemess);

} // end function

Finally, the logic in my controller is where I make the calls to show_error. I know in CI i can use templates or something... but since I don't think there are going to be too many cases in my switch statement, i thought i'd try this approach. any suggestions would be appreciated.

Answer 1

I don't really like the method of exception pseudo-typing you're doing, I'd use different objects to represent the type of exception rather than the content, like so:

function macaddresses($ip, $name)
{
    try
    {
        //some code....
        if($obj->connect())
        {
            //do something else
            return $macaddies;
        }
        else
        {
            throw new UserVisibleException("Unable to connect to device. Please contact administrator!");
        }                                
    }
    catch(Exception $e)
    {
        $this->processException($e);
    }
}

private function processException($exception)
{
    if($exception instanceof UserVisibleException)
    {
        throw $exception;
    }
    else
    {
        log_message('error', $exception->getTraceAsString());
        throw new UserVisibleException(null, $exception);
    }
}

class UserVisibleException extends Exception
{
    public function __construct($message = null, $cause = null, $code = 1)
    {
        parent::__construct
        (
            $message === null ? "Oops!  Something big just happened." : $message,
            $code, $cause
        );
    }
}

I put processException() as a method as it isn't clear whether you're using this design pattern elsewhere, but I assume so as you're connecting to something and there's usually other exceptions that can happen later. If you aren't, you can just stick it back into the catch block of macaddresses().

Additionally, if it isn't clear, the ternary is so you don't have to manually type in the message to attach a cause to an unknown exception being wrapped. And I took the liberty of replacing the generic message "connection" with the stack trace of the exception that caused it being passed to your logger, as this more detailed information will be useful should you wish to debug the cause of it.

Answer 2

There's no need to have functions dedicated to processing exceptions. This is exactly what the try-catch construct is designed to do.

Best practice is to use inheritance to catch (and process) different exceptions different ways. For example, you could create a base UserException class (and inherit from it for more specific exceptions), which your try-catch blocks would show to the user:

<?php

// User-facing exception classes
class UserException extends Exception {}

class BadInputException extends UserException
{
    public function __construct($message = 'Bad Input', $code = 400)
    {
        parent::__construct($message, $code);
    }
}

// All other exceptions are assumed to be hidden from the user
class DatabaseException extends Exception
{
    public function __construct($message = 'Internal Server Error', $code = 500)
    {
        parent::__construct($message, $code);
    }
}

Then, you catch the exceptions based on how you want them handled:

<?php

try {
    // some action
}
catch (UserException $e)
{
    // show message to user
}
catch (Exception $e)
{
    // show generic message, log error, etc.
}