Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I have coded a very simple error class that will allow me to display errors if they should happen. For example, if a file is missing or a database connection couldn't be established I would display a nice error, instead of revealing an ugly PHP error to them.

<?php
defined('LATRINA') or exit('You cannot view this file.');

class Error
{
    private $errorTitles = array();
    private $errorExplanations = array();
    private $errorDesign;

    public function load()
    {
        $this->errorTitles['myerror1'] = 'My Error 1';
        $this->errorExplanations['myerror1'] = 'This is my first error, just a small description.';

        $this->errorTitles['myerror2'] = 'Oh my, my second error!';
        $this->errorExplanations['myerror2'] = 'I really need to sort this error out, its a bad one!!';

        $this->errorTitles['myerror3'] = 'Well, well, well!';
        $this->errorExplanations['myerror3'] = 'Oh wow, this could potentially break a lot of shit..';
    }

    public function triggerError($error)
    {
        if (!isset($errorTitles[$error]) || !isset($errorExplanations[$error]))
        {
            exit('An error happened, unfortinately we couldn\'t handle it...');
        }

        echo '
        <!DOCTYPE html>
        <html lang="en-GB">
        <head>
            <title>' . Latrina::getCodeName() . ': Error</title>
            <style type="text/css">
            body {
                background-color: whitesmoke;
                font-family: sans-serif;
            }
            </style>
        </head>
        <body style="margin-top:4%;margin-left:4%;">
            <h1>'. $this->errorTitles[$error] . '</h1>
            <p>'. $this->errorExplanations[$error] . '</p>
        </body>
        </html>';

        exit();
    }
}

Usage:

Latrina::getLibrary('latrina.error.error')->triggerError('myerror2');
share|improve this question
3  
While expletives in private code are tolerable, I would avoid them in all code that potentially could be seen by many people. Such as code posted on the internet. It looks very unprofessional. – Graipher Dec 5 at 16:25

My primary feedback is that this is very hard-coded and inflexible.

I don't think you are going to want to have to change class code every time you want to make a change the end user messaging or HTML messaging format.

You have several concerns you are trying to address all within this single class:

  • error classification
  • determining end user message based on
  • defining display template (in this case and HTML template) for response tpo end user

These could all be broken out into classes that handle this responsibility. Consider something like:

  • ErrorDisplayTemplate - a class defining template to use for error display. Perhaps this takes form of an abstract base class that can be inherited for different template types (html, json, xml, etc.)

  • ErrorDisplayMapper - a class that maps system errors to end user messages

  • ErrorDisplayController - a class that uses classes above to render the display

  • UserFriendlyError - class defining fields being exposed for display (i.e. title, text, etc.)

This would allow you to decouple these pieces of functionality, giving more flexible usage such as:

// to render html
$user_error = ErrorDisplayMapper::getUserFriendlyError($some_system_error_info);
$template = ErrorDisplayTemplate::getTemplate('html');
ErrorDisplayController::render($user_error, $template);
exit();

// to render json
$user_error = ErrorDisplayMapper::getUserFriendlyError($some_system_error_info);
$template = ErrorDisplayTemplate::getTemplate('json');
ErrorDisplayController::render($user_error, $template);
exit();

Note here that you could easily define multiple templates for totally different types of responses without having to make changes to the other classes in the application.

So the primary takeaway for you is that this could certainly use some refactoring to separate these different concerns.

Some other thoughts:

  • Be meaningful and specific when naming objects (classes, functions, variables, etc.) in your code. Some poor naming examples include:
    • Error as class name. This is not an application-level error or error handling class, so please don't attach such a meaningful (in multiple contexts) term to this class. Something like UserFriendlyError is much more specific to what role this class is supposed to take in your application.
    • triggerError method does not "trigger" anything. It renders output. So a naming like renderError or displayError would be much more appropriate.
    • load method does not "load" anything. This way this class is hard-coded now, you could easily eliminate this method (and perhaps even provide more clarity in code) by having these definitions as constants on the class.
  • I don't see a case for why this should be a concrete class. Why would one need to instantiate an object of this class in order to access the behavior of this class. Consider whether this functionality should be use statically as in my usage example shown above.
  • What is $errorDesign property used for? If not used, remove it.
  • When your get into exceptional conditions in your code, it is usually bad practice to die/exit and directly message end user. For example, your triggerError method has guarding clause here where it is unclear whether this is truly an exceptional condition (the app should NEVER try to render an undefined error type) or whether you are just applying some fallback/default messaging. If truly an exceptional/unexpected case, then throw an Exception here.

To expand on my suggested usage above, you might leave it up to ErrorDisplayMapper class to determine what to do about an unknown error type - does it throw exception, or does it apply default messaging? If it applies default message, then you could use my example above as is, but if it throws an exception, perhaps you need to modify to be like this:

try {
    $user_error =
        ErrorDisplayMapper::getUserFriendlyError($some_system_error_info);
} catch (Exception $e) {
    // perhaps log exceptional case here
    // perhaps do something like this to recover
    $user_error = ErrorDisplayMapper::getUserFriendlyError(
        ErrorDisplayMapper::UNKNOWN_ERROR
    );       
}
$template = ErrorDisplayTemplate::getTemplate('html');
ErrorDisplayController::render($user_error, $template);
exit();
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.