Return to Revisions

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.