returning control to caller

Question

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;

Answer 1

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