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();