2
\$\begingroup\$

I've started to write a form data handler class in PHP. Can you please review it and point out any mistakes or logic errors, or perhaps if a method can be written in a better way?

define('VALIDATION_ERR', ' couldn`t be validated.');
define('REQUIRED_ERR', ' is required.');
define('PASSWORD_ERR', 'Password and Confirm Password do not match.');

class DataValidator {

protected $errorMsg;
protected $errorFlag;

public function __construct(){
    $this->errorMsg = array();
    $this->errorFlag = 0;
} // constructor

public function checkRequiredData( $dataArray , $requiredFields = NULL ){
    if( count($dataArray) > 0 ){
        if( $requiredFields != NULL ){
            foreach( $dataArray as $key => $val ){
                if( array_key_exists( $key , $requiredFields ) ){
                    $format = $requiredFields[$key];
                    if( !empty($val) && $format != 'none' ){
                        if( !($this->dataFormatCheck($val, $format)) ){
                            $msg = $key . VALIDATION_ERR ;
                            $this->setErrorMsg($msg);
                            $this->errorFlag = 1;
                        }
                    }
                    else{
                        $msg = $key . REQUIRED_ERR;
                        $this->setErrorMsg($msg);
                        $this->errorFlag = 1;
                    }
                }
            }
        } // requiredFields array IF
        else{
            foreach( $dataArray as $key => $val ){
                if( empty($val) ){
                    $msg = $key . REQUIRED_ERR;
                    $this->setErrorMsg($msg);
                    $this->errorFlag = 1;
                }
            }
        }
    } // requiredFields count IF
    else{
        $msg = "data array is empty";
        $this->setErrorMsg($msg);
        $this->errorFlag = 1;
    }

    if( $this->errorFlag == 0 ){
        return true;
    }
    else{
        return false;
    }
} // checkRequiredData

public function getErrorMsg( $last = '' ){
    if( $last == 'last' ){
        return end($this->errorMsg);
    }
    else{
        return $this->errorMsg;
    }
} // getErrorMsg

public function setErrorMsg( $msg ){
    array_push( $this->errorMsg, $msg );
} // setErrorMsg

public function dataFormatCheck( $data , $format ){
    // use switch or if else?
    switch( $format ){
        case 'text' :
            $return = true; // will be implemented later
            break;

        case 'email' :
            $return = $this->checkEmail($data);
            break;

        case 'numeric' :
            $return = $this->checkNumeric($data);
            break;

        case 'alphanumeric' : // will be implemented later
            $return = true;
            break;

        case 'password' :
            $return = $this->checkPassConfirm($data);
            break;

        case 'image'    : // will be implemented later
            $return = true;
            break;

        case 'none' : // will be implemented later
            $return = true;
            break;
    }
    return $return;
} // dataFormatCheck

public function checkAlpha( $data ){
     // will be implemented later
} // check alphabets

public function checkEmail( $data ){
    return filter_var($data, FILTER_VALIDATE_EMAIL);
} // check email

public function checkNumeric( $data ){
    return is_numeric( $data );
} // check Numbers

public function checkAlphaNumeric( $data ){
     // will be implemented later
} // check Alpha Numerics

public function checkPassConfirm( $pass , $cPass ){
    if( $pass != $cPass )
        return false;
    else
        return true;
}

}
\$\endgroup\$
  • \$\begingroup\$ Does the code work? \$\endgroup\$ – Kid Diamond Aug 27 '14 at 11:56
  • \$\begingroup\$ Of course it does. Why do you ask? \$\endgroup\$ – Syntax Error Aug 27 '14 at 11:58
  • \$\begingroup\$ "... point out any mistakes or logic errors ..." Questions regarding pointing out mistakes and logic errors don't belong on CodeReview. \$\endgroup\$ – Kid Diamond Aug 27 '14 at 11:59
2
\$\begingroup\$

Not yet implemented

It's unsafe to just have a comment like this:

// will be implemented later

At least add a TODO (so that an IDE can collect all of these):

// TODO will be implemented later

Or better yet, throw an exception:

throw new Exception('Not implemented');

If your code gets bigger and bigger, it's easy to overlook one or two of your original comments, and then your data will not be filtered correctly.

if-else return true-false

Again, whenever you have an if/else statement that only returns true/false, just return the statement inside the if. So for example

if( $pass != $cPass )
    return false;
else
    return true;

would become

return $pass === $cPass;

Notice also the === instead of ==.

The same goes for this:

if( $this->errorFlag == 0 ){
    return true;
}
else{
    return false;
}

write it like this instead:

return $this->errorFlag == 0;

End-of-Function comments

I think that these only clutter up the code, I would get rid of them.

Inside a function, // endif type comments are a sign of bad code. If you have so many if-statements that you cannot see where each ends, you probably have too many. Try to get rid of some or extract some functionality into a separate method.

Switch or if-else

Since you asked: I think that the switch looks better than an if-else.

Error Flag

You never set this to 0, is this intentional?

Also, you set this flag always directly after calling setErrorMsg. Just move $this->errorFlag = 1; inside setErrorMsg, so you don't have to manage it.

If you would change it to a boolean value, it would also be easier to handle.

Unnecessary local variables

You do this very often:

$msg = $key . VALIDATION_ERR ;
$this->setErrorMsg($msg);

As you don't need msg afterwards, just call setErrorMsg directly:

$this->setErrorMsg($key . VALIDATION_ERR);

Deeply nested checkRequiredData function

You have too many if-else in your checkRequiredData function which makes it really hard to read.

One way to solve this is to pull the checks to the top of the method like this:

function checkRequiredData($dataArray, $requiredFields = NULL) {
    if (count($dataArray) <= 0) {
        $msg = "data array is empty";
        $this->setErrorMsg($msg);
        $this->errorFlag = 1;
        return false;
    }

    if ($requiredFields == NULL) {
        foreach ($dataArray as $key => $val) {
            if (empty($val)) {
                $msg = $key . REQUIRED_ERR;
                $this->setErrorMsg($msg);
                $this->errorFlag = 1;
            }
        }
        return $this->errorFlag == 0;
    }

    foreach ($dataArray as $key => $val) {
        if (array_key_exists($key, $requiredFields)) {
            $format = $requiredFields[$key];
            if (!empty($val) && $format != 'none') {
                if (!($this->dataFormatCheck($val, $format))) {
                    $msg = $key . VALIDATION_ERR;
                    $this->setErrorMsg($msg);
                    $this->errorFlag = 1;
                }
            } else {
                $msg = $key . REQUIRED_ERR;
                $this->setErrorMsg($msg);
                $this->errorFlag = 1;
            }
        }
    }
    return $this->errorFlag == 0;
}

Confusing nested ifs

This code is a bit confusing (it's not that easy to see when there will be no error):

        if (!empty($val) && $format != 'none') {
            if (!($this->dataFormatCheck($val, $format))) {
                $msg = $key . VALIDATION_ERR;
                $this->setErrorMsg($msg);
                $this->errorFlag = 1;
            }
        } else {
            $msg = $key . REQUIRED_ERR;
            $this->setErrorMsg($msg);
            $this->errorFlag = 1;
        }

This might be better:

        if (empty($val) || $format == 'none') {
            $msg = $key . REQUIRED_ERR;
            $this->setErrorMsg($msg);
            $this->errorFlag = 1;
        } else if (!$this->dataFormatCheck($val, $format)) {
            $msg = $key . VALIDATION_ERR;
            $this->setErrorMsg($msg);
            $this->errorFlag = 1;
        } // else: everything is fine

Final Code

If you follow all these suggestions, your checkRequiredData function would look like this:

function checkRequiredData($dataArray, $requiredFields = NULL) {
    if (count($dataArray) <= 0) {
        $this->setErrorMsg("data array is empty");
        return false;
    }

    if ($requiredFields == NULL) {
        foreach ($dataArray as $key => $val) {
            if (empty($val)) {
                $this->setErrorMsg($key . VALIDATION_ERR);
            }
        }
        return $this->hasError;
    }

    foreach ($dataArray as $key => $val) {
        if (array_key_exists($key, $requiredFields)) {
            $format = $requiredFields[$key];
            if (empty($val) || $format == 'none') {
                $this->setErrorMsg($key . REQUIRED_ERR);
            } else if (!$this->dataFormatCheck($val, $format)) {
                $this->setErrorMsg($key . VALIDATION_ERR);
            } // else: everything is fine
        }
    }
    return $this->hasError;
}

public function setErrorMsg( $msg ){
    $this->hasError = true; // set error flag here, use boolean instead of int for easier usage
    array_push( $this->errorMsg, $msg );
}

Which is a lot shorter and a lot easier to understand. It would be best to write some unit tests to verify that it actually does the exact same thing, as this is a major rewrite.

\$\endgroup\$
  • \$\begingroup\$ nice suggestions dude. \$\endgroup\$ – Syntax Error Aug 27 '14 at 12:20
  • \$\begingroup\$ Yeah I was also worried about checkRequiredData function because it gets complicated more and more as we go inside it. \$\endgroup\$ – Syntax Error Aug 27 '14 at 12:21
  • \$\begingroup\$ I initialized the Error Flag from 0 means there is no error in the start, but as soon as the script find any it set it to 1. Do you think this isn't the correct way of doing it, sir? \$\endgroup\$ – Syntax Error Aug 27 '14 at 12:30
  • \$\begingroup\$ @OMI No, it's fine. I was just a bit confused, as the flag doesn't seem to be used from the outside, right? So it could just be a local variable inside checkRequiredData. If you restructure your code as I suggested, you might as well remove it completely. \$\endgroup\$ – tim Aug 27 '14 at 12:33
  • \$\begingroup\$ @OMI I made a small error in my suggested code (I returned true instead of hasError at the end of the function), now it should be working correctly. \$\endgroup\$ – tim Aug 27 '14 at 13:44

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.