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?
