5
\$\begingroup\$

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);
    }
}
\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ This will be my personal code, I won't share it with anyone. That's the reason why I haven't commented as well. I only comment where I feel it would be necessary. As far as my dump method, it's actually just something quick I put there for developing/debugging purposes to see what's going on, I plan on removing that later. Thank you for your review! \$\endgroup\$ – Kid Diamond Jun 21 '14 at 23:26
  • 1
    \$\begingroup\$ @KidDiamond Even if it's personal code, I don't see why that should stop you from documenting it. It could make using it in a year a lot easier! I had a feeling the dump() was temporary, it's fine then. \$\endgroup\$ – Alex L Jun 21 '14 at 23:31
  • \$\begingroup\$ Yeah, makes sense. I'm gonna look into PHPDoc. Also, I changed the if .. elseif .. else in my setRule() method with a switch() which slightly improved the readability. \$\endgroup\$ – Kid Diamond Jun 22 '14 at 0:34
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Nice one on the exceptions part, I will change that. As far as the Required rule class, I had one called NotEmpty but I got rid of it. Because if I left it out (for the field to be optional), the validator was still checking against the rest of the declared rules instead of just returning true. That's why I fused the required rule logic with the validator otherwise all my rule classes would need an extra argument to specify if the input is optional or not. I posted a question on SO specifically for that. \$\endgroup\$ – Kid Diamond Jun 22 '14 at 10:03
  • 1
    \$\begingroup\$ That makes sense. Note that documenting this type of design decision might help you to understand why you did it like this in the future. \$\endgroup\$ – ebo Jun 22 '14 at 10:41

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.