HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

PHP validation class using method dispatch

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
methodphpvalidationdispatchusingclass

Problem

I wrote this validation class. I'm kinda new to the whole OO(PHP) style, so I don't know if my class is acceptable. I looked around at other people's validation classes, but most are huge (1k+ lines of code) or similar to mine in some ways.

```
class Validation{

private $errors = array();
private $defaultErrors = array();

public function __construct(){
$this->defaultErrors = array(
'required' => "This field is required",
'unique' => "This value is already in use",
'number' => "Not a number",
'numberRange' => "Number is not in range",
'email' => "Not a valid email",
'characters' => "Only letters are allowed",
'length' => "This value is too long or short",
'postcode' => "This postcode is not valid",
'message' => "This message is not valid",
'compareString' => "These values are not the same"
);
}

public function validationMethod($data) {
foreach ($data as $input => $rule) {
$methods = explode('|', $rule[0]);
foreach ($methods as $method) {
switch ($method) {
case 'number':
self::validate($method, 'validateNumber', $input, null);
break;
case 'numberRange':
self::validate($method, 'numberRange', $input, array($rule[1], $rule[2]));
break;
case 'characters':
self::validate($method, 'validateChars', $input, null);
break;
case 'length':
self::validate($method, 'validateLength', $input, array($rule[1], $rule[2]));
break;
case 'email':
self::validate($method, 'validateEmail', $input, null);
break;
case 'postcode':

Solution

I find Validation to be an odd name for the class, and validationMethod($data) to be an odd name for its main method. Which would you rather read… (new Validation())->validationMethod($data), or (new Validator())->validate($data)?

It doesn't look like there is a way to override $defaultErrors, so you might as well make it static, and not redefine it in the constructor:

class Validator {
    private static $defaultErrors = array(…);
    …
}


I consider if (…): …; endif; to be poor style. The code would be easier to read if you laid it out with proper indentation.

In all your validateX() methods, you should avoid writing an if-statement altogether. For example:

private function validateNumber($data) {
    return is_int($data);
}


I would avoid clutter by eliminating the trivial setErrors() method. You can also eliminate the $tmp variable in validate(). (Variables named $tmp are generally a bad idea anyway. If you do need a variable, there is always a better name for it than $tmp.) In accordance with the naming remark above, I would rename the existing validate().

private function callValidationMethod($method, $func, $input, $options) {
    if (!self::$func($input, $options)) {
        $this->errors[] = self::$defaultErrors[$method];
    }
}

Code Snippets

class Validator {
    private static $defaultErrors = array(…);
    …
}
private function validateNumber($data) {
    return is_int($data);
}
private function callValidationMethod($method, $func, $input, $options) {
    if (!self::$func($input, $options)) {
        $this->errors[] = self::$defaultErrors[$method];
    }
}

Context

StackExchange Code Review Q#87727, answer score: 2

Revisions (0)

No revisions yet.