Form data handler class

Question

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;
}

}

Answer 1

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.