Reading this SO question it seems that throwing exceptions for validating user input is frowned upon.
But who should validate this data? In my applications, all validations are done in the business layer, because only the class itself really knows which values are valid for each one of its properties. If I were to copy the rules for validating a property to the controller, it is possible that the validation rules change and now there are two places where the modification should be made.
Is my premise that validation should be done on the business layer wrong?
What I do
So my code usually ends up like this:
<?php
class Person
{
private $name;
private $age;
public function setName($n) {
$n = trim($n);
if (mb_strlen($n) == 0) {
throw new ValidationException("Name cannot be empty");
}
$this->name = $n;
}
public function setAge($a) {
if (!is_int($a)) {
if (!ctype_digit(trim($a))) {
throw new ValidationException("Age $a is not valid");
}
$a = (int)$a;
}
if ($a < 0 || $a > 150) {
throw new ValidationException("Age $a is out of bounds");
}
$this->age = $a;
}
// other getters, setters and methods
}
In the controller, I just pass the input data to the model, and catch thrown exceptions to show the error(s) to the user:
<?php
$person = new Person();
$errors = array();
// global try for all exceptions other than ValidationException
try {
// validation and process (if everything ok)
try {
$person->setAge($_POST['age']);
} catch (ValidationException $e) {
$errors['age'] = $e->getMessage();
}
try {
$person->setName($_POST['name']);
} catch (ValidationException $e) {
$errors['name'] = $e->getMessage();
}
...
} catch (Exception $e) {
// log the error, send 500 internal server error to the client
// and finish the request
}
if (count($errors) == 0) {
// process
} else {
showErrorsToUser($errors);
}
Is this a bad methodology?
Alternate method
Should maybe I create methods for isValidAge($a) that return true/false and then call them from the controller?
<?php
class Person
{
private $name;
private $age;
public function setName($n) {
$n = trim($n);
if ($this->isValidName($n)) {
$this->name = $n;
} else {
throw new Exception("Invalid name");
}
}
public function setAge($a) {
if ($this->isValidAge($a)) {
$this->age = $a;
} else {
throw new Exception("Invalid age");
}
}
public function isValidName($n) {
$n = trim($n);
if (mb_strlen($n) == 0) {
return false;
}
return true;
}
public function isValidAge($a) {
if (!is_int($a)) {
if (!ctype_digit(trim($a))) {
return false;
}
$a = (int)$a;
}
if ($a < 0 || $a > 150) {
return false;
}
return true;
}
// other getters, setters and methods
}
And the controller will be basically the same, just instead of try/catch there are now if/else:
<?php
$person = new Person();
$errors = array();
if ($person->isValidAge($age)) {
$person->setAge($age);
} catch (Exception $e) {
$errors['age'] = "Invalid age";
}
if ($person->isValidName($name)) {
$person->setName($name);
} catch (Exception $e) {
$errors['name'] = "Invalid name";
}
...
if (count($errors) == 0) {
// process
} else {
showErrorsToUser($errors);
}
So, what should I do?
I’m pretty happy with my original method, and my colleagues to whom I have showed it in general have liked it. Despite this, should I change to the alternate method? Or am I doing this terribly wrong and I should look for another way?
4
The approach I’ve used in the past is to put all validation logic dedicated Validation classes.
You can then inject these Validation classes into your Presentation Layer for early input validation. And, nothing prevents your Model classes from using the very same classes to enforce Data Integrity.
Following this approach you can then treat Validation Errors differently depending on which layer they happen in:
- If Data Integrity Validation fails in the Model, then throw an Exception.
- If User Input Validation fails in the Presentation Layer, then display a helpful tip and delay pushing the value to your Model.
5
I’m pretty happy with my original method, and my colleagues to whom I have showed it in general have liked it. Despite this, should I change to the alternate method? Or am I doing this terribly wrong and I should look for another way?
If you and your colleagues are happy with it, I see no pressing need to change.
The only thing that is questionable from a pragmatic perspective is that you are throwing Exception rather than something more specific. The problem is that if you catch Exception, you may end up catching exceptions that have nothing to do with validation of user input.
Now there are many people who say things like “exceptions should only be used for exceptional things, and X Y Z is not exceptional”. (For example, @dann1111’s Answer … where he labels user errors as “perfectly normal”.)
My response to that is that there is no objective criterion for deciding whether something (“X Y Z”) is exceptional or not. It is a subjective measure. (The fact that any program needs to check for errors in user input does not make the occurrence errors “normal”. In fact, “normal” is largely meaningless from an objective standpoint.)
There is a grain of truth in that mantra. In some languages (or more accurately, some language implementations) exception creation, throwing and/or catching is significantly more expensive than simple conditionals. But if you look at it from that perspective, you need to compare the cost of create/throw/catch against the cost of the extra tests that you might need to perform if you avoided using exceptions. And the “equation” has to take into account the probability that the exception needs to be thrown.
The other argument against exceptions is that they can make code harder to understand. But the flip-side is that when they are used appropriately, they can make code easier to understand.
In short – the decision to use or not use exceptions should be made after weighing up the merits … and NOT on the basis of some simplistic dogma.
5
In my opinion, it is useful to distinguish between application errors and user errors, and only use exceptions for the former.
-
Exceptions are intended to cover things that prevent your program from executing properly.
They are unexpected occurrences that prevent you from continuing, and their design reflects this: they break normal execution and jump to a place that allows error handling.
-
User errors like invalid input are perfectly normal (from your program’s perspective) and should not be treated as unexpected by your application.
If the user enters the wrong value and you display an error message, did your program “fail” or have an error in any way? No. Your application was successful–given a certain kind of input, it produced the correct output in that situation.
Handling user errors, because it is part of normal execution, should be part of your normal program flow, rather than handled by jumping out with an exception.
Of course it is possible to use exceptions for other than their intended purpose, but doing so confuses the paradigm and risks incorrect behavior when those errors occur.
Your original code is problematic:
- The caller of the
setAge()method must know way too much about the method’s internal error handling: the caller needs to know that an exception is thrown when the age is invalid, and that no other exceptions could be thrown within the method. This assumption could be broken later if you added additional functionality withinsetAge(). - If the caller does not catch exceptions, the invalid age exception would later be handled in some other, most likely opaque, way. Or even cause an unhandled exception crash. Not good behavior for invalid data being entered.
The alternative code also has problems:
- An extra, possibly unnecessary method
isValidAge()has been introduced. - Now the
setAge()method has to assume that the caller already checkedisValidAge()(a terrible assumption) or validate the age again. If it validates the age again,setAge()still has to provide some sort of error handling, and you are back to square one again.
Suggested design
-
Make
setAge()return true on success and false on failure. -
Check the return value of
setAge()and if it failed, inform the user that the age was invalid, not with an exception, but with a normal function that displays an error to the user.
9
From my point of view (I’m a Java guy) it is totally valid how you implemented it the first way.
It is valid that an object throws an Exception when some preconditions are not met (e.g. empty-string).
In Java the concept of checked exceptions is inteded to such a purpose – exceptions that must be declared in the signature to be propably thrown, and the caller explicitly needs to catch those. In contrast, unchecked exceptions (aka RuntimeExceptions), may happen any time without the need to define a catch-clause in code. While the first are used for recoverable cases (e.g. wrong user input, filename does not exist), the latter are used for cases the user/programmer can’t do anything about (e.g. Out-Of-Memory).
You should though, as already mentioned by @Stephen C, define your own exceptions and catch specifically those to not catch others unintentionally.
Another way, however, would be to use Data Transfer Objects which are simply data-containers without any logic. You then handover such a DTO to a validator or the Model-Object itself for validation, and only if successfull, make the updates in the Model-Object.
This approach is often used when presentation logic and application logic are seperated tiers (presentation is a webpage, appplication a webservice). This way they are physically separated, but if you have both on one tier (as in your example), you must ensure that there will be no workaround to set a value without validation.
With my Haskell hat on, both approaches are wrong.
What happens conceptually is that you first have a bunch of bytes, and after parsing and validating, you can then construct a Person.
The Person has certain invariants, such as the precense of a name and an age.
Being able to represent a Person that only has a name, but no age is something you want to avoid at all costs, because that is what creates compleity. Strict invariants means you do not need to check for the presence of an age later on for example.
So in my world, Person is created atomically by using a single constructor or function. That constructor or function can again check the validity of the parameters, but no half-Person should be constructed.
Unfortunately, Java, PHP and other OO languages make the correct option pretty verbose. In proper Java APIs, builder objects are often used. In such an API, creating a person would look something like this:
Person p = new Person.Builder().setName(name).setAge(age).build();
or the more verbose:
Person.Builder builder = new Person.Builder();
builder.setName(name);
builder.setAge(age);
Person p = builder.build();
// Person object must have name and age here
In these cases, no matter where exceptions are thrown or where validation happens, it is impossible to receive a Person instance that is invalid.
4
In layman’s words:
The first approach is the correct one.
The second approach assumes that those business classes will only be called by those controllers, and that they will never be called from other context.
Business classes must throw an Exception everytime a business rule is violated.
Controller or presentation layer must decide whether it throws them or it does it’s own validations to prevent the exceptions from happening.
Remember: your classes will potentially be used in different contexts and by different integrators. So they must be smart enough to throw exceptions to bad inputs.