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 false;
}
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 true;
}
Error Flag
If you would change it to a boolean value, it would also be easier to handle.
$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;
}
function checkRequiredData($dataArray, $requiredFields = NULL) {
if (count($dataArray) > 0) {
$this->setErrorMsg("data array is empty");
return false;
}
if ($requiredFields == NULL) {
$error = false;
foreach ($dataArray as $key => $val) {
if (empty($val)) {
$this->setErrorMsg($key . VALIDATION_ERR);
$error = true;
}
}
return $error;$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.
