Cleaning up validator class

Question

I updated my class with more functionality, but I kind if feel like it's a mess or that it can at least be improved by a bit readability, efficiency and/or DRY wise.

Anybody who could see if that can be done?

Any other suggestions are also welcome.

class Validator
{
    private $errors = [];
    private $rules  = [];

    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 order as specified
    public function setRule($name, Array $rules, $required = true)
    {
        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'] = $rules;

            if ($required === true) {
                $this->rules[$name]['required'] = 'Must not be null or empty.';
            } elseif ($required === false) {
                $this->rules[$name]['required'] = false;
            } else {
                $this->rules[$name]['required'] = (String) $required;
            }
        } else {
            throw new Exception(sprintf('Rule %s already exists.', $name));
        }
    }

    public function validate($rulename, $input)
    {
        if ($this->rules[$rulename]['required']) {
            if (empty($input)) {
                $this->errors[$rulename][] = $this->rules[$rulename]['required'];
            }

            foreach ($this->rules[$rulename]['rules'] as $rule) {
                try {
                    $rule->check($input);
                } catch (Exception $e) {
                    $this->errors[$rulename][] = $e->getMessage();
                }
            }

            if (isset($this->errors[$rulename])) {
                return false;
            }
        } elseif (!empty($input)) {
            foreach ($this->rules[$rulename]['rules'] as $rule) {
                try {
                    $rule->check($input);
                } catch (Exception $e) {
                    $this->errors[$rulename][] = $e->getMessage();
                }
            }

            if (isset($this->errors[$rulename])) {
                return false;
            }
        }

        return true;
    }

    public function dump()
    {
        print_r($this->rules);
    }
}

Answer 1

Good work formatting and approaching styles in a fashionable method, it makes reading your code very easy! a couple things to note:

• It's common to see private variables prefixed with an underscore to improve readability. It's up to you if you want to implement this, and if you use a framework, check their docs to see if it's standard.

• Regarding your last method, dump(): the name dump could mean many things. Without seeing the source, what's it dumping? It'd be fine to leave it as is, but you may consider renaming to dumpRules() or something of that sort.

• I'm not sure if you left it out, or you just haven't, but it's also a good practice to document your code. That means add comments. Googling PHPDoc will give you a good start, and if your familiar with JavaDocs, that's what I'm talking about.

---

Be careful making new array's with just = [].

As of PHP 5.4 you can also use the short array syntax, which replaces array() with [].

That's from the docs. So if anyone using below 5.4 wants your code, they could experience some turbulence setting it up.

---

The one comment you do have, is started with #. I believe this style of comments are deprecated and the C style comments (/*...*/, //) are favorable. I'll try and find a reference to this claim.

---

I noticed you have

foreach ($this->rules[$rulename]['rules'] as $rule) {
    try {
        $rule->check($input);
    } catch (Exception $e) {
        $this->errors[$rulename][] = $e->getMessage();
    }
}

a couple times. You could put this in a function (with appropriate parameters) and simply call the function to reduce multiplicities.

---

Other than that, all looks well. Just in case you wanted to look at some other validation classes (if you haven't already), here's one to get you started.

Answer 2

Two remarks considering the design of the current approach.

While checking the rules an exception is thrown to indicate that the rule is violated. Thus, exceptions are used as control-flow, which is considered an anti-pattern. Why not have the check method return a boolean and give IRule a getErrorMessage method?

Secondly, the Validator class now also contains the logic for required fields. Why not create an RequiredRule which contains this logic? This would make your validate and setrule methods considerably simpler.