5
\$\begingroup\$

I have written Validation and Form classes in PHP.

  • Validation class allow you to define the rules of each field and a error message.
  • Form class allow you to get a error message and also allow you to get a value of POST via $_SESSION.

I stored in $_SESSION because if validation did not pass it will need to redirect back to main Form page and repopulate all the fields using value() method.

What do you think of Validation and Form classes - what can be improved?

Validation Class

class Validation
{
    protected  $fields = array();
    private $errors = array();

    public function setRules($field, $errorMessage, $rules = array())
    {
        if (count($_POST) == 0) {
            throw new Exception("The array of post parameters is empty");
        }

        if ($field == '') {
            throw new Exception("field parameter is empty");
        }

        if (!is_array($rules) || count($rules) == 0) {
            throw new Exception("The array of rules parameter is empty");
        }

        $errorMessage = ($errorMessage == '') ? $field : $errorMessage;

        $this->fields[] = array(
            'name' => $field,
            'errorMessage' => $errorMessage,
            'rules' => $rules,
        );
    }

    public function validate()
    {
        if (count($_POST) == 0) {
            throw new Exception("The array of post parameters is empty");
        }

        if (count($this->fields) == 0) {
            throw new Exception("Validation rules is not set");
        }

        foreach ($this->fields as $field) {

            $fieldName = $field['name'];
            if (isset($_POST[$fieldName])) {
                foreach ($field['rules'] as $rule) {

                    $param = false;

                    if ($arr = explode('=', $rule)) {
                        if (isset($arr[0]) && isset($arr[1])) {
                            $param = $arr[1];
                            $rule = $arr[0];
                        }
                    }

                    $output = $this->$rule($_POST[$fieldName], $param);

                    if ((in_array('required', $field['rules']) && $output == FALSE) || (!in_array('required', $field['rules']) && $output == FALSE && $_POST[$fieldName] != '')) {
                        if (!isset($this->errors[$fieldName])) {
                            $this->errors[$fieldName] = array('errorMessage' => $field['errorMessage']);
                        }
                    }
                }
            }
        }
        return (count($this->errors) == 0) ? true : false;
    }

    public function getError($field)
    {
        if (isset($this->errors[$field]) && $this->errors[$field] != '') {
            return $this->errors[$field]['errorMessage'];
        }
        return false;
    }

    private function validEmail($value)
    {
        return filter_var($value, FILTER_VALIDATE_EMAIL);
    }

    private function required($value)
    {
        return ($value != '') ? true : false;
    }

    private function numeric($value)
    {
        return is_numeric($value) ? true : false;
    }

    private function exactLength($value, $param)
    {
        return (strlen($value) == $param) ? true : false;
    }
}

Form Class

class Form
{
    private $validation;

    function __construct($validation) {
        $this->validation = $validation;
        $_SESSION['posts'] = $_POST;
    }

    public function error($field)
    {
       return $this->validation->getError($field);
    }

    public function value($field) {
        if (isset($_SESSION['posts'][$field]))
            return $_SESSION['posts'][$field];
    }
}

Usage Testing:

// FAKE POST TEST
$_POST['name'] = 'Pau';
$_POST['email'] = 'hello@google';

$val = new Validation();
$form = new Form($val);

$val->setRules('name', 'Please enter your full name', array('exactLength=6','required'));
$val->setRules('email', 'Please enter your email', array('validEmail'));

if ($val->validate()) {
    echo "Good! - Validation Passed";
} else {
    echo "Bad! - Validation Not Passed";
    echo $form->error("name");
}
\$\endgroup\$
2
\$\begingroup\$

Well, first off, I think using client side cookies, instead of session cookies, might be a little better here. It puts less strain on your server and keeps unnecessary transfers from occurring. But the difference might be trivial, depending on the type of traffic you are expecting. Corbin touched on a few good points, +1 to him, below are some that I found.

$fields should only be protected if it is being shared, either by a parent or child class. Since the Validation class isn't extending anything I think its safe to say that no parent classes are using it. Its a little harder to tell if it has any children that might use it. I don't see any in the code you posted, but that doesn't mean its not being done somewhere else. Just know that protected means a property is to be shared but not available outside of a parent/child relationship, otherwise it should be public or private, depending on how you are planning on using it. Though typically public properties are avoided in favor of setters/getters.

As Corbin has said, you should pass in the POST array instead of explicitly calling for it in your class. For instance, say you parsed a JSON file and wanted to verify it before using it, this class would be the perfect candidate, but because you have limited it to using only POST values you will either have to create a new class just for JSON, or you will have to modify this one to use both POST and JSON, and the cycle will continue the more things you add. Better to just generalize it now. This will also mean that verifying that the POST array was set will become unnecessary, at least here.

Also, an easier way to determine if an array is empty is to use the empty() function. This holds true for anywhere you are comparing the size of an array to zero.

if( ! is_array( $rules ) || empty( $rules ) ) {

Since it appears obvious that you only want an array to be passed as your ruleset, perhaps a better way of passing the $rules parameter would be to use type hinting in addition to a default value. This would throw an error automatically if some other form of variable was used, meaning you don't have to explicitly check it anymore, except for empty of course, you can just assume its an array and work from there.

public function setRules( $field, $errorMessage, Array $rules = array() ) {

If you are like me and are unlucky enough not to be able to use short ternary yet, then the following ternary statement is fine, but, if your PHP version is >= 5.3, then know that you can change it. Also, an empty string translates to a FALSE state, so explicitly checking it should be unnecessary; and those parenthesis are also unnecessary.

$errorMessage = ($errorMessage == '') ? $field : $errorMessage;
//The same thing in 5.3
$errorMessage = $errorMessage ?: $field;

Perhaps a better way to parse your rules would be to use array functions rather than explicitly declaring each array element. This will make your nested if statements completely unnecessary. Additionally, try not to define variables in your statements. It can make debugging hard, besides, the only way this function would return false is if you passed it an empty string. If you use array_filter() in your setRules() method before setting this value to the fields array then all such elements will automatically be removed from your array.

$rules = array_filter( $rules );
if( empty( $array ) { /* throw error */ }
//complete setRules() and start validate()

$ruleParams = explode( '=', $rule );
$rule = array_pop( $ruleParams );
$param = array_shift( $ruleParams );

Be careful of variable-functions, or in this case variable-methods. Typically most people will glare at you and call you dirty names for doing this. They are very hard to debug for. I understand what you are trying to do, but I'm almost tempted to tell you to do it manually.

The following if statement is working too hard. First, it is just too long. You can abstract some of those statements to make this a bit easier to read.

if ((in_array('required', $field['rules']) && $output == FALSE) || (!in_array('required', $field['rules']) && $output == FALSE && $_POST[$fieldName] != '')) {
//compared to
$noOutput = $output == FALSE;
$required = in_array( 'required', $field[ 'rules' ] );
if( ( $required && $noOutput ) || ( ! $required && $noOutput && $_POST[ $fieldName ] ) ) {

Additionally, the above statement is redundant. At least in as far as you are checking if a field exists in your array on every iteration. Do it once outside of the loop and use that instead. But what is this even doing? You are saying that if one parameter is required, they all are? Did you instead mean to check if $rule == 'required'? This just seems a bit odd.

$required = in_array( 'required', $field[ 'rules' ] );
foreach( $this->fields AS $field ) {
    //etc...
    if( ( $required && $noOutput ) || ( ! $required && $noOutput && $_POST[ $fieldName ] ) ) {

Alright the next statement I sort of get, you don't want to overwrite any existing messages, but why not instead just log them all using a multidimensional array?

if (!isset($this->errors[$fieldName])) {
    $this->errors[$fieldName] = array('errorMessage' => $field['errorMessage']);
}
//compared to
$this->errors[ $fieldsName ] [] = array( 'errorMessage' => $field[ 'errorMessage' ] );

No need to explicitly define a TRUE/FALSE state for the return value. Just return the count, or empty() check, either one would result in the same thing, though I would use the latter as it does not require the added negation.

return ! count($this->errors);
//or
return empty( $this->errors );

The following statement will always be true, at least partially. Each element of the $errors property is an array, so it will never be equal to an empty string. Maybe you meant to use an empty() check here, but even that should be impossible. The $errors property is private and I don't see any method unsetting values from this array, therefore, if the $field index is set, the value should be as well. Just remove the second check.

if (isset($this->errors[$field]) && $this->errors[$field] != '') {
//compared to
if( isset( $this->errors[ $field ] ) ) {

Then the above could be rewritten so that you only have one return statement. Though this ternary statement is getting a bit long and complicated, so this might be unwise.

return isset( $this->errors[ $field ] ) ? $this->errors[ $field ] [ 'errorMessage' ] : FALSE;

Please, always use braces on your statements. PHP inherently requires them, otherwise you wouldn't have to add them if your statements extend more than one line. This would be different if that wasn't the case, but it is and it can cause issues. Besides, this is inconsistent with the rest of your code.

if (isset($_SESSION['posts'][$field]))
    return $_SESSION['posts'][$field];
//use the following instead
if( isset( $_SESSION[ 'posts' ] [ $field ] ) ) {
    return $_SESSION[ 'posts' ] [ $field ];
}

Correct me if I'm wrong, but I'm not sure your usage tests works. You pass your validation class to the form class, but because nothing has been done to it yet it will not reflect in the form class. This means that calling for the errors from the form class is pointless. You should just wait to inject the validation class until you add everything you need to first.

\$\endgroup\$
5
\$\begingroup\$

Short on time, but here's a few quick things:

  • Consider making individual validators rather than putting all of the different validation rules in one class.
    • One option would be to make a Field class and have Form contain a collection of Fields (with each field having a collection of Validators)
  • Pass in $_POST instead of accessing it in your class (what if you want to validate $_GET or something?)
  • Form shouldn't directly depend on $_SESSION
  • $_SESSION['posts'] = $_POST; The form shouldn't be responsible for storing state. If you want to do that, do it outside of the Form class.
  • How do you know which validation(s) failed?
  • The two exceptions thrown in validate should be reconsidered. A missing field should probably be considered empty (and thus a required validation failed), and a Form with no rules, should probably always validate as true. (Also, they should probably be specialized exception classes, not just the generic Exception)
  • return ($value != '') ? true : false; Is redundant: return ($val != '');
    • In particular, any expression of the form (expr) ? true : false can be written as (expr)
  • Be careful with implicit typecasts (for example, some of your string comparisons might should be ===)
  • $this->errors[$fieldName] = array('errorMessage' => $field['errorMessage']); You're clobbering any old errors
  • There should probably be some way to tell which elements have errors. How did you know name has an error if validate fails? How do you know it's not email?

(I will likely come back to this. Also, I haven't forgotten about your post from 2 days ago that I said the same thing on... Just been very busy :D)

\$\endgroup\$
  • \$\begingroup\$ Thanks for answer. Looking forward for more details :P Oh please clarity what do you mean by "Consider making individual validators rather than putting all of the different validation rules in one class." ... If would be great if you can show example of code and improve my code. Thanks! \$\endgroup\$ – I'll-Be-Back Sep 30 '12 at 22:41
  • 1
    \$\begingroup\$ @I'll-Be-Back: instead of having one Validator class, you may have Validator_email, Validator_price...etc This way you can unittest each class individually, and you may also create new validators by extending from old ones \$\endgroup\$ – Quamis Oct 1 '12 at 14:09
  • \$\begingroup\$ @I'll-Be-Back, @Quamis nailed it. Each validator should be responsible for one validation (fairly related to separation of concerns). I would have an interface that has one method: public function validate();. That would then return an array of any errors. (Or an empty array if there are no errors. Or perhaps true, or, you get the point.) All of your validators would then implement that interface. If you want, I can still type out a full example with a few interfaces. The Zend_Validate stuff is a fairly good real-world example though. \$\endgroup\$ – Corbin Oct 5 '12 at 18:46

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.