patternphpMinor
Validator Readability and Efficiency
Viewed 0 times
andreadabilityvalidatorefficiency
Problem
This is my first validator I have created myself and I was wondering if there is anything about it that I could improve as far as:
Note: The reason why I put an underscore before each validation method name is so that non-validation methods cannot be called. E.g.
Any other suggestions are also welcome!
Validator class:
```
class Validator
{
private $rules = [];
private $errors = [];
public function setRule($id, Array $methods)
{
if (!array_key_exists($id, $this->rules)) {
$this->rules[$id] = $methods;
} else {
throw new Exception('Rule "' . $id . '" already exists.');
}
}
public function setRules(Array $rules) {
foreach ($rules as $key => $value) {
$this->setRule($key, $value);
}
}
public function validate($rule, $input)
{
foreach ($this->rules[$rule] as $key => $value) {
if (is_int($key)) { //$key => rule
$value = '_' . $value;
$method = $value;
$arguments = [$input];
} else { //rule => [args]
$key = '_' . $key;
$method = $key;
$arguments = array_merge([$input], is_array($value) ? $value : [$value]);
}
return call_user_func_array([$this, $method], $arguments);
}
}
public function setError($error)
{
return $this->errors[] = $error;
}
public function getErrors()
{
return $this->errors;
}
private function _required($input)
{
return empty($input) ?: $this->errors[] = substr(__FUNCTION__, 1);
}
private function _min($input, $min)
{
return mb_strlen($input, 'utf8') >= $min ?: $this->errors[] = substr(__FUNCTION__, 1);
}
- Code readability
- Code efficiency
Note: The reason why I put an underscore before each validation method name is so that non-validation methods cannot be called. E.g.
setRule() can't be called if it's specified in the rule unless there is an actual validation method called "_setRule".Any other suggestions are also welcome!
Validator class:
```
class Validator
{
private $rules = [];
private $errors = [];
public function setRule($id, Array $methods)
{
if (!array_key_exists($id, $this->rules)) {
$this->rules[$id] = $methods;
} else {
throw new Exception('Rule "' . $id . '" already exists.');
}
}
public function setRules(Array $rules) {
foreach ($rules as $key => $value) {
$this->setRule($key, $value);
}
}
public function validate($rule, $input)
{
foreach ($this->rules[$rule] as $key => $value) {
if (is_int($key)) { //$key => rule
$value = '_' . $value;
$method = $value;
$arguments = [$input];
} else { //rule => [args]
$key = '_' . $key;
$method = $key;
$arguments = array_merge([$input], is_array($value) ? $value : [$value]);
}
return call_user_func_array([$this, $method], $arguments);
}
}
public function setError($error)
{
return $this->errors[] = $error;
}
public function getErrors()
{
return $this->errors;
}
private function _required($input)
{
return empty($input) ?: $this->errors[] = substr(__FUNCTION__, 1);
}
private function _min($input, $min)
{
return mb_strlen($input, 'utf8') >= $min ?: $this->errors[] = substr(__FUNCTION__, 1);
}
Solution
Although you used a
And the usage:
As you can't type hint the content of an array, you might also want to replace
Validator class, this is not really object orientated. Actually I'm missing a Rule class. Your rules are applied by comparing strings right now, which is error prone and really ugly if you need to refactor it. Extending your code with new rules also requires changing the Validator itself.class Rule {
/**
* @return bool
*/
abstract function check($value);
/**
* @return string
*/
abstract function getErrorMessage($value)
}
class BetweenRule extends Rule{ //Maybe there is also an IntegerRule to collect some operations.
private $min;
private $max;
public function __construct ($min,$max) {...}
public check($value)
{
return this->minmax>=$value;
}
public function getErrorMessage($value) { return $valus." is not between ..." ;}
}
...And the usage:
$v = new Validator();
$r=new RequiredRule();
$v->setRule('email', [$r, new BetweenRule(5,255), new EmailRule()]);
$v->setRule('password', [$r]);
if (!$v->getErrors()) { //either as a list of string or list of map fields and rules
...
} else {
...
}As you can't type hint the content of an array, you might also want to replace
setRule by addRule and append new rules to the existing ones, instead setting the array of rules.$v = new Validator();
$v->addRule('email', new RequiredRule());
$v->addRule('email', new BetweenRule(5,255));
$v->addRule('email', new EmailRule());Code Snippets
class Rule {
/**
* @return bool
*/
abstract function check($value);
/**
* @return string
*/
abstract function getErrorMessage($value)
}
class BetweenRule extends Rule{ //Maybe there is also an IntegerRule to collect some operations.
private $min;
private $max;
public function __construct ($min,$max) {...}
public check($value)
{
return this->min<=$value && $this->max>=$value;
}
public function getErrorMessage($value) { return $valus." is not between ..." ;}
}
...$v = new Validator();
$r=new RequiredRule();
$v->setRule('email', [$r, new BetweenRule(5,255), new EmailRule()]);
$v->setRule('password', [$r]);
if (!$v->getErrors()) { //either as a list of string or list of map fields and rules
...
} else {
...
}$v = new Validator();
$v->addRule('email', new RequiredRule());
$v->addRule('email', new BetweenRule(5,255));
$v->addRule('email', new EmailRule());Context
StackExchange Code Review Q#52499, answer score: 3
Revisions (0)
No revisions yet.