0
\$\begingroup\$

I have built a class which validates configuration data. Below is a very simplified version with only the code that's relevant to this question.

class configValidator
{
    private $_errors = [];

    public function validate()
    {
        $this->errorGenerator(1, 'fail');       
        if ($this->_errors) return $this->_errors;

        $this->errorGenerator(2, 'pass');
        if ($this->_errors) return $this->_errors;

        $this->errorGenerator(3, 'fail');
        if ($this->_errors) return $this->_errors;

        $this->errorGenerator(4, 'fail');
        if ($this->_errors) return $this->_errors;
    }

    private function errorGenerator($id, $string)
    {
        if ($string == 'fail')  $this->_errors[] = 'an error occurred on ID ' . $id;
    }
}

$configValidator = new ConfigValidator();
$errors = $configValidator->validate();
if (!empty($errors)) {
    exit(json_encode($errors));
} else {
    echo 'config validations passed';
}

In the validate() method, it is important that errors are returned to the caller, if any exist, after each call to the errorGenerator() method. This is because in the actual version there are arrays with keys and subsequent calls to errorGenerator() are dependent on those array keys existing. If they don't exist, then PHP gives an "undefined index notice".

As written above, the code operates as expected - it displays ["an error occurred on ID 1"] in the browser and does not get as far as executing the other calls to the errorGenerator() method. This is the desired behavior.

However, looking at the validate() method, it seems redundant to type this line 4 times
if ($this->_errors) return $this->_errors;
so I experimented with stripping it out of the validate() method and putting that line into the errorGenerator() method like so:

public function validate()
{
    $this->errorGenerator(1, 'fail');       

    $this->errorGenerator(2, 'pass');

    $this->errorGenerator(3, 'fail');

    $this->errorGenerator(4, 'fail');
}

private function errorGenerator($id, $string)
{
    if ($string == 'fail')  $this->_errors[] = 'an error occurred on ID ' . $id;
    if ($this->_errors) return $this->_errors;
}

This results in the browser displaying config validations passed, which is not the desired outcome. It makes sense that is what's being displayed because moving that line into the errorGenerator() method returns control to the validate() method rather than to the code that called the validate() method.

I'm wondering if there's a way to refactor this code such that it returns the errors to the caller of validate(), yet does not repeat this line
if ($this->_errors) return $this->_errors;

\$\endgroup\$
1
  • \$\begingroup\$ I'd be interested in reviewing the actual code. "subsequent calls to errorGenerator() are dependent on those array keys existing" Is definitely a sign of code smell. There must be a better way to do this. Don't create hacks for errors \$\endgroup\$ – dustytrash Nov 6 '19 at 0:28
2
\$\begingroup\$

Honestly I have no idea what the class is supposed to do. It is not validating anything, there is no input. It just always fails on ID 1 (whatever that means), no matter what. You probably omited too much. And whatever is omited probably deserves a review on its own.

There is no reason to have private $_errors. Change your checks so that the first passes and second fails. Then call validate() twice. The second call will not behave as expected. You better keep it as the method's local variable, or maybe you dont need a variable at all...

Question is why are you returning array of errors, if you only ever return one? But let's say you have reason, then your errorGenerator is doing too much anyway.

public function validate()
{
  if (check1()) {
    return [$this->getErrorMessage(1)];
  }
  if (check2()) {
    return [$this->getErrorMessage(2)];
  }
  return [];
}

private function getErrorMessage(int $id)
{
  return 'an error occurred on ID ' . $id;
}

The check1, check2 methods represent something that is omited in your code samples...

And actualy you can take it a step further and remove getErrorMessage and inline the messages instead. Specific message for each check. Error message "an error occured on ID xy" is a terrible one.

Another way may be using exceptions, but to me it feels like abuse of exceptions. But I know there are a lot of devs who would disagree.

And last point to say: It's not always possible to squash two lines into one line while retaining qualities like readability. If it was possible, every program (no matter the complexity) could then be written as a well readable one liner ;)

\$\endgroup\$
4
  • \$\begingroup\$ In the actual code - by refactoring with the if statements and calling $this->getErrorMessage() within those, and converting check1, check2 into array_key_exists(), these changes allowed for a single line of if ($this->_errors) return $this->_errors; at the end of the method. \$\endgroup\$ – knot22 Nov 6 '19 at 10:18
  • 1
    \$\begingroup\$ @knot22 if a method returns array and last statement is if ($this->_errors) return $this->_errors; then that method does not fulfill its own contract as it does not return array when there are no errors. Just omit the if($this->_errors) and return them directly... Anyway its better to be local $errors, instead of property $this->errors, being property leads to corrupted state after first failed validation. \$\endgroup\$ – slepic Nov 6 '19 at 10:24
  • \$\begingroup\$ Stripped out the if ($this->_errors) - thanks for that pointer. Can you expound on the comment of "being a property leads to corrupted state after first failed validation"? I don't understand that part. \$\endgroup\$ – knot22 Nov 6 '19 at 10:30
  • \$\begingroup\$ @knot22 check the second paragraph of my answer. Its explained there. In short the Array will agregate all errors from consecutive calls to validate(). \$\endgroup\$ – slepic Nov 6 '19 at 11:05

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.