Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm looking for advice and ways on making it better.

Code

<?php

Class Validate {

    private $_errors = array();

    public function __construct()
    {
        require_once 'errors.php';
        $this->errorText = $errorText;
    }

    public function rule($value, $name, $inputRules)
    {

        $inputRules = explode('|', $inputRules);
        foreach ($inputRules as $inputRule) {

            if (!strlen($value) && $inputRule != 'required') {
                break;
            }

            if (preg_match('/\[(.*?)\]/', $inputRule, $match)) {

                $rule = explode('[', $inputRule);
                $rule = $rule[0];
                $param = $match[1];
                if (!method_exists($this, $rule)) {
                    throw new Exception("Method {$rule} does not exist");
                    exit;
                }

                $call = array($value, $param);
            }
            else {
                if (!method_exists($this, $inputRule)) {
                    throw new Exception("Method {$inputRule} does not exist");
                    exit;
                }
                $rule = $inputRule;
                $call = array($value);
            }

            $response = call_user_func_array(array($this, $rule), $call);

            if ($response) {
                $error = $this->errorText[$rule];
                $replace = array(
                    ':name' => $name,
                    ':param' => (isset($param)) ? $param : NULL
                );
                $response = str_replace(array_keys($replace), array_values($replace), $error);

                $this->_errors[] = $response;
            }

        }

    }

    public function exec()
    {
        return (empty($this->_errors)) ? false : $this->_errors;
    }

    /*
    * Rule functions
    */

    public function min_length($value, $param)
    {
        return (strlen($value) < $param);
    }

    public function max_length($value, $param) 
    {
        return (strlen($value) > $param);
    }

    public function email($value) 
    {
        return !filter_var($value, FILTER_VALIDATE_EMAIL);
    }

    public function required($value) 
    {
        return (strlen($value) !== 0);
    }

    public function ip($value) 
    {
        return !filter_var($value, FILTER_VALIDATE_IP);
    }

    public function match($value, $param)
    {
        return ($value != $param);
    }

    public function match_exact($value, $param) 
    {
        return ($value !== $param);
    }

    public function match_password($value, $param) 
    {
        return ($value !== $param);
    }

    public function alphanum($value)
    {
        return !ctype_alnum($value);
    }

    public function url($value)
    {
        return !filter_var($value, FILTER_VALIDATE_URL);
    }

    public function numeric($value)
    {
        return !(is_numeric($value));
    }

    public function min($value) 
    {
        return ($value < $param);
    }

    public function max($value) {
        return ($value > $param);
    }

}
share|improve this question
    
Post rolled back as answers have been invalidated. –  Jamal Jun 24 at 4:03

3 Answers 3

Technical Issues

public function __construct()
{
    require_once 'errors.php';
    $this->errorText = $errorText;
}

Will not do what you think it will do.

$val1 = new Validate();
$val2 = new Validate();

The second instance will not include 'errors.php', and thus $errorText will not be defined.

You should probably consider generalizing your error message handling since you might need to have different messages or internationalization.

In other words, your class should probably either return error codes (class constants), or your class should have some kind of facility for requesting a different error message at run time.

If you do stick with your current approach though, consider structuring it something like:

public function __construct()
{
    $this->errorText = $this->getErrorText();
}

private function getErrorText()
{
    include 'errors.php';
}

Your errors.php would then look like:

<?php
return array(
    'something' => 'Some message'
    'somethingElse' => 'some other message',
    ...
);

This would of course mean that the file is included on every object instantiation though. A different approach could be to store the messages in a static variable the first time they're needed.

Also, while I'm talking about this, errors.php could be a problematic path. You should probably use something like dirname(__FILE__) . '/errors.php'; so the path is more resistant to outside factors.


if (!method_exists($this, $rule)) {
    throw new Exception("Method {$rule} does not exist");
    exit;
}

There's two issues here:

  • exit will never be reached
  • exit shouldn't here

When you throw an exception, you bail out of the current scope up until you hit a catch block. No code under the throw will be executed.

function f() {
    throw new Exception();
    //No code here is ever excecuted
}

function f() {
    if (...) {
        throw new Exception();
        //Code here is never excecuted
    }
    //Code here will be executed only if the throw above is not reached
}

function f() {
    try {
        if () {
            throw new Exception();
            //Code here is never exceuted
        }
        //Code here is executed if the throw is not reached
    } catch (Exception $e) { }
    //Code here is always executed
}

It gets a bit more complicated than that, but those examples should illustrate the basic behavior.

exit shouldn't be in there anyway though. Would you actually want script execution to completely stop? Typically exit should only be used if there is no way to continue the program execution without compromising security or stability.

Imagine if instead of throwing exceptions, you had only the exits there. This means that no error message would be displayed. Nothing would be logged. The code would just bail.

Imagine if someone was using your class based only on the API. They would have no idea why the code exited.

Typically low level flow control like exiting or redirecting to a different page, so on, should be handled in dedicated "controller" code and not in classes that have responsibilities unrelated to that.

Throwing an exception is the right thing to do here. It allows the code that called rule to decide how to handle it. rule should only responsible for saying "Hey, ummm, something wen't horribly wrong," not deciding how to handle it. (In fact, an uncaught exception is basically an exit with debugging information included :p. You do have to be careful with uncaught exceptions though as you need to make sure they don't get displayed to end clients. Some exceptions, like PDOException, may contain sensitive data like database credentials.)

Design Issues

Your class does way too much. This is just using a class as a collection of functions. Each validation can be seen as a "responsibility" and thus your class completely violates the single responsibility principle.

Imagine that in the future you want to add a new validation. How can you do this? Well, you have two options, both of which are unpleasant:

  1. Edit the source code. What if many people are using your class though, or you're using it across 10 applications? Suddenly you have a lot of version management going on. Since you control the class, you could just commit it all to git which would alleviate the synchronization, but what about end-consumers of your code? This isn't a viable option for them.

  2. Extend the class. This is also a bad option. You would have to change all of the object types in your code to be a MyValidation instead of Validation. Depending on how widespread this was, this could be a huge chore.

Or, you have a third option. Scrap your current design and abstract each validation into its own class. You could have a very simple interface:

interface Validator
{

    /**
     * Validates the given $data against whatever rule(s) the implementation of this
     * interface enforces.  Returns an array of error messages, or an empty array
     * if $data is valid.
     *
     * @param mixed $data
     * @return array An array of error messages
     */
    public validate($data);

}

And then you could build on this:

class RangeValidator implements Validator
{

    private $_from;
    private $_to;

    const ERROR_INVALID_TYPE = 'type';
    const ERROR_OUT_OF_RANGE = 'range';

    //You could pull this into an abstract base class to handle code -> message mapping automatically.
    //That would also allow for easy internationalization later.  
    private $_messages = array(

    );

    public function __construct($from, $to)
    {
        //You could make this a lot looser and perhaps put it in methods setFrom and setTo.
        //Just an example.
        if (!is_int($from)) {
            throw new InvalidArgumentException("\$from must be an int.  Given value: '{$from}'");
            }
            if (!is_int($to)) {
                throw new InvalidArgumentException("\$to must be an int.  Given value: '{$from}'");
        }
        $this->_from = $from;
        $this->_to = $to;
    }

    public function validate($data)
    {
        if (is_int($data) || (is_string($data) && ctype_digit($data))) {
            if ($data >= $this->_from && $data <= $this->_to) {
                return array();
            } else {
                return array(self::ERROR_INVALID_TYPE => $this->_messages[self::ERROR_INVALID_TYPE]);
            }
        } else {
            return array(self::ERROR_INVALID_TYPE => $this->_messages[self::ERROR_INVALID_TYPE]);
        }
    }

}

This looks very laborious at first, but it gives you a certain degree of flexibility you didn't have before. Consider how you add a new validator now. You just create a new class completely unrelated to any of the other ones. (Technically it would implement Validator, and perhaps extend an abstract base class. What I mean though is that it doesn't change any of the other classes.)

In this system, consumption of your code is much simpler too. How does someone that doesn't have write access to your git repository create their own validator? They just implement validator in their own namespace rather than in yours. (Oh, side note, you should probably be using namespaces.)

For simplicity, you could make a "composite" Validator that just ties a bunch of validators together:

class EvenValidator implements Validator
{
    //Imagine that this validates whether or not a number is even
    //(Yes, really weird example :p)
}

class CompositeValidator implements Validator
{

    $this->_validators = array();

    public function __construct(array $validators = array())
    {
        foreach ($validators as $validator) {
            $this->addValidator($validator);
        }
    }

    public function addValidator(Validator $validator)
    {
        $this->_validators[] = $validator;
    }

    public funtion validate($data)
    {
        $errors = array();
        foreach ($this->_validators as $validator) {
            $errors += $validator->validate($data);
        }
        return $errors;
    }

}

You could then use it like:

$range = new RangeValidator(1, 100);
$even = new EvenValidator();
$comp = new CompositeValidator(array($range, $even));

$comp->validate(5); //Returns an array of one message: 5 isn't odd
$comp->validate(2); //Returns an empty array
$comp->validate(222); //Returns an array of one message: 222 out of range
$comp->validate(223); //Two messages: 223 out of range and odd

This is some pretty clumsy usage here. With more than 5 seconds of thought though, an interface could be designed that allowed for very robust functionality while leaving the code highly maintainable and extensible.

You could even take it a step farther than a composite validator and make a "Form" class that has "Element"s which each have zero or more "Validator"s. (The approach take by Zend Framework, and I'm sure others).


If you're familiar with any PHP frameworks, you might note that all of that above looks eerily familiar. In particular, that's a modified version of the Zend_Validate interface from the Zend Framework.

I would suggest you pick a framework, go through the code, figure out how they designed things, and then ask yourself why they designed them that way: what the alternatives could have been and why they picked the path they chose. If you do look at frameworks for design, make sure you look at more than one. Though most of the major PHP frameworks/libraries are very well written overall, all of them have at least one area where they far from shine.

Also, if you haven't already, read everything you can find about SOLID.

share|improve this answer

Judging your interface: Nice!

You could have provided some static method for simpler checking:

if( Validate::check($email, 'Email', 'required|email') ){ ... }

or and check-all-at-once method

$errors = Validate::checkall( array(
    array($email, 'Email', 'required|email'),
    array($name, 'Name', 'required'),
) );
if( sizeof( $errors ) ){
    # print errors
}

It looks really nice so far!

Edit: I looked at the two classes: It's really nice! It's undocumented and it's still understandable!

share|improve this answer
    
Thanks man! Yeah, I wanted something that you could instansiate for each form you're validating, which is why I didn't use static methods. Good suggestion though! Appreciate the compliments :) –  Prash Nov 19 '12 at 22:18

Please don't use static methods as was suggested to you by max.haredoom. Static methods cause tight coupling in the code that calls them. See the following answers for reasons to avoid static: Why, oh why and what to do?

Properties

  • Remove the underscore from $_errors. See this answer for the reason that underscore should not be used for class properties.

  • errorText should be defined as a property of the class and its accessibility should be defined.

Constructor

  • require_once in the constructor is a bad idea. This class cannot stand alone as a unit and it is completely broken with more than one object. Pass in the required information as arguments to the constructor.

rule method

  • The name of this method does not explain what it is doing.
  • Pass $inputRules as an array (and typehint for it). I can't see the reason for mashing together rules as a string when they are a list of rules. The array type represents lists very well, so I would use it and avoid having to explode the string on |.

    public function rule($value, $name, Array $inputRules)

  • Your code for throwing an exception is repeated. Calculate the variables in the if / else and then check only once.

  • The variables should be renamed to something more meaningful. $rule should be $validationMethod and $call should be $validationArguments.
  • Use is_callable rather than method_exists it ensures that you will be able to call it (which is what you really want).

exec method

  • This does not exec anything (thankfully).
  • Perhaps it should be named validate, but I would have expected it to return true when there weren't any errors?

Design

I agree with Corbin. A validation interface is a great way to do this sort of thing. I think the key part of a validation class should be returning whether the data is valid or not. Your class does not make this a focus. Personally I like a simple interface that checks the validity and returns a boolean of whether the data is valid or not. The other thing that is needed is a method to return the errors from the last validation.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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