3
\$\begingroup\$

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':
                        self::validate($method, 'validatePostcode', $input, null);
                        break;
                    case 'message':
                        self::validate($method, 'validateMessage', $input, null);
                        break;
                    case 'compareString':
                        self::validate($method, 'compareStrings', $input, $rule[1]);
                        break;
                    default:                            
                        break;
                }
            }
        }

        if(count($this->errors) > 0): return false; endif;
        return true;
    }

    private function validate($method, $func, $input, $options) {
        $tmp = self::$func($input, $options);
        if(!$tmp): self::setErrors($method); endif;
    }

    private function setErrors($method) {
        $this->errors[] = $this->defaultErrors[$method];
    }

    public function getErrors() {
        return $this->errors;
    }

    private function validateNumber($data) {
        if(is_int($data)):return true; endif;
        return false;
    }

    private function numberRange($number, $range) {
        if(($number > $range[0]) && ($number < $range[1])): return true; endif;
        return false;
    }

    private function validateChars($data) {
        if (ctype_alpha(str_replace(' ', '', $data)) === true): return true; endif;
        return false;
    }

    private function validateLength($data, $limit) {
        $data = strlen($data);
        if(($data > $limit[0]) && ($data < $limit[1])): return true; endif;
        return false;
    }

    private function validateEmail($data) {
        if(preg_match("~^[a-z0-9!#$%&'*+/=?^_`{|}-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$~", $data)):return true; endif;
        return false;
    }

    private function validatePostcode($data) {
        if(preg_match('/^([0-9]{4}[a-zA-Z]{2})$/', $data)): return true; endif;
        return false;
    }

    private function validateMessage($data) {
        if($data == strip_tags($data)): return true; endif;
        return false;
    }

    private function compareStrings($string1, $string2) {
        if($string1 === $string2): return true; endif;
        return false;
    }
}

Anyway, Any suggestions on how to improve this class? All suggestions are greatly appreciated :)

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! We're here to help improve code, not to judge. =) I've edited your question accordingly. \$\endgroup\$ – 200_success Apr 23 '15 at 9:49
  • \$\begingroup\$ ty <3 did not realize that. \$\endgroup\$ – paintception Apr 23 '15 at 10:26
2
\$\begingroup\$

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];
    }
}
\$\endgroup\$
  • \$\begingroup\$ thank you for your feedback. I'll definitely take up your advice :) really appreciate it <3 \$\endgroup\$ – paintception Apr 23 '15 at 11:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.