Validator refactored to be OO

Question

Based on my previous post I completely re-factored my code to make it object oriented.

Anything I could improve when it comes to:

• Object oriented code;
• Efficiency;
• Readability.

Any other suggestions are also welcome!

Validator class:

class Validator
{
 private $rules = [];
 private $errors = [];
 public function getErrors($rulename = null)
 {
 if ($rulename) {
 return isset($this->errors[$rulename]) ? $this->errors[$rulename] : null;
 }
 return $this->errors;
 }
 # rules will be checked against in same order as specified
 public function setRule($name, Array $rules)
 {
 if (!array_key_exists($name, $this->rules)) {
 foreach ($rules as $rule) {
 if (!$rule instanceof IRule) {
 throw new Exception('Array must only contain instances of IRule.');
 }
 }
 $this->rules[$name] = $rules;
 } else {
 throw new Exception(sprintf('Rule %s already exists.', $name));
 }
 }
 public function validate($rulename, $input)
 {
 foreach ($this->rules[$rulename] as $rule) {
 try {
 $rule->check($input);
 } catch (Exception $e) {
 $this->errors[$rulename][] = $e->getMessage();
 }
 }
 if (isset($this->errors[$rulename])) {
 return false;
 }
 return true;
 }
}

One of the rule classes:

class Between implements IRule
{
 private $errMsg = 'Must be between %s and %s.';
 private $inclusive;
 private $max;
 private $min;
 public function __construct($min, $max, $inclusive = true, $errMsg = null)
 {
 $this->min = $min;
 $this->max = $max;
 $this->inclusive = $inclusive;
 if ($errMsg !== null) {
 $this->errMsg = $errMsg;
 }
 }
 public function check($input)
 {
 if ($this->inclusive && $input >= $this->min && $input <= $this->max) {
 return true;
 } elseif (!$this->inclusive && $input > $this->min && $input < $this->max) {
 return true;
 } else {
 throw new Exception(sprintf($this->errMsg, $this->min, $this->max));
 }
 }
}

Example:

$validator = new Validator();
$validator->setRule('name', [new NotEmpty(), new MaxChars(20), new Alpha(true)]);
$validator->setRule('email', [new NotEmpty(), new Email()]);
$validator->setRule('age',
 array(
 # last constructor argument overrides default error message
 new NotEmpty('You did not enter your age.'),
 new Numeric('Your age can only be numerical.'),
 new Between(13, 17, true, 'Only teenagers are allowed.')
 )
);
$validator->validate('name', 'John');
$validator->validate('email', 'Doe');
$validator->validate('age', 'Hello');
# report
if (!$validator->getErrors()) {
 echo '<b>Input(s) valid:</b> Passed complete validation.';
} else {
 echo '<b>Input(s) invalid:</b> Failed complete validation.';
}

Answer 1

This is a really solid start. I only have a couple of suggestions.

1. Instead of defining the rules outside of the Validator class, it might be a good idea to sub class Validator and set the rules inside the constructor.

2. Create public methods specifically for each kind of validator.

Some pseudo code to get the main idea:

validator.php

class Validator
{
 private $rule_collection = array();
 public function __construct()
 {
 }
 public function ruleFor($property)
 {
 if (!isset($this->rule_collection[$property]))
 $this->rule_collection[$property] = new RuleBuilder($property);
 return $this->rule_collection[$property];
 }
 public function validate($model)
 {
 $valid = true;
 foreach ($this->rule_collection as $property => $rule_builder)
 {
 foreach ($rule_builder->rules as $rule)
 {
 if (!$rule->validate($model->$property))
 $valid = false;
 }
 }
 return $valid;
 }
}

rule_builder.php

interface IRule
{
 public validate($value);
 public get_message();
}
class RuleBuilder
{
 private $property;
 public $rules = array();
 public function __construct($property)
 {
 $this->property = $property;
 }
 public function isRequired()
 {
 return $this->addRule(new RequiredRule());
 }
 public function length($min, $max = -1)
 {
 return $this->addRule(new LengthRule($min, $max));
 }
 public function addRule(IRule $rule)
 {
 $this->rules[] = $rule;
 return $this;
 }
}

And your specific validator:

class UserValidator extends Validator
{
 public function __construct()
 {
 $this->ruleFor('name')
 ->isRequired()
 ->length(20);
 }
}

To use it:

$user = new User();
$user->name = "Foo";
$validator = new UserValidator();
if ($validator->validate($user))
 // save
else
 // redraw form and show errors

This way all of your rule classes are hidden. All of the validation rules are portable because they are created inside a model specific class, and it's easy to use an IDE's auto complete feature to discover which rules are available since the RuleBuilder class has strongly typed methods encapsulating each rule.

On top of that, the IRule interface just accepts a value and not a model, making each rule unit testable to ensure your validation library is functioning properly.

And for those who do some .NET/C# development, this pattern probably looks familiar if you've used the FluentValidation NuGet package for Visual Studio. Say what you want about .NET development, but there are some gems out there. I do like the pattern that FluentValidation uses.

• Kid Diamond: This is basically a domain object validator, right?
• Greg Burghardt: Essentially, yes, but could also be applied to a view-model as well.
• Kid Diamond: I took your answer and made some changes. You can check it out if you like. :)

Answer 2

Other points to consider:

Localization: instead of

throw new Exception('Must be between 13 and 17');

go for

$e = new ValidationException('Must be between %s and %s');
$e->setArgs([$this->min, $this->max]);
throw $e;

It's possible to translate Must be between %s and %s and then do the sprintf part, but if the string is formated first with arguments, you loose the ability to translate (unless you translate all combination of Must be between [number] and [number+]).

Allow for empty values. You already have NotEmpty validator, so other validators should pass if the value is empty.

You should return cleaned up values from your validators. Eg. someone inputs phone number as [phone], another one as 555-6363-6637, but you want to store [phone] in database, this is what a validator can do.

Consider also, that you will need access to more than one input, for example password and confirm_password. Your design does not take that into account.

• Kid Diamond: It'd be nice if you could explain why I should go for that localization in your answer, I'm not so knowledgeable with exceptions. Making it pass the rest of the rules when value is empty with NotEmpty, I can just check if the rule is an instance of NotEmpty and if it validates to false stop executing the rest. I can have a PhoneNumber rule that checks if the input is valid, but I don't think it's a validator's job to return sanitized values. As far as validating double inputs, I can make a setError() method and have ($a !== $b) { $v->setError(...) } outside of the validator.
• Marek: I don't think your Validator class should handle NotEmpty specially. For double (or multiple) inputs, do you really want to do validation outside of validator? Again, another special case. Sure, it's possible to do sanitanization in another class, but I'm used to do it in validators and it works for me. I'll elaborate on localization in my answer.
• Kid Diamond: I think I should get rid of the NotEmpty class, and fuse it with my Validator class instead. So I can add a last argument and do something like $v->setRule('email', [new Email()], true); If true and input is empty, don't validate, vice versa. For confirmation inputs, I can have a $v->setRule('email', [new Email(), Match($_POST['confirm_email'])]); rule for that. Pass in the confirmation input in the Match rule, and the first input in the validate method. $v->validate('email', $_POST['email'];.