php - Extract to method with empty returns

Solution:

How I would do it:

interface CustomerValidationInterface
{
    public function isValid(Customer $customer): bool;
}

class CustomerFooValidator implements CustomerValidationInterface
{
    public function isValid(Customer $customer)
    {
        return $customer->foo === 'FOO';
    }
}

class CustomerBarValidator implements CustomerValidationInterface
{
    public function isValid(Customer $customer)
    {
        return $customer->bar === 'BAR';
    }
}

class CustomerValidator
{
    private $validators;

    /**
     * @param CustomerValidationInterface[] $validators
     */
    public function __construct(array $validators)
    {
        $this->validators = $validators;
    }

    public function check(Customer $customer)
    {
        foreach ($this->validators as $validator) {
            if ($validator->isValid($customer) === false) {
                return 'error';
            }
        }

        return 'success'
    }
}

you can future proof your code, if you need more things to validate in the future, you just need to create a new class implementing your interface and pass to the main validator class.

Update customer status IMHO should do what it says, update the customer status, not check for validation. You should do validation 1 step before this.

Solution:

I would suggest to refactor your code so that the updateCustomerStatus() method only contains business logic, i.e. checking the invariants on the customer before allowing to update it.

Currently you would call UI logic inside this method (even if the echo is happening in a separate method). I'd like to move the actual rendering of the UI out of updateCustomerStatus().

And I will even go a step further an also move the database access portion (findCustomerById()) out of the business logic method.

This way you can also test the business logic in isolation without needing to worry about the UI and the database.

So here is the direction in which I would move when refactoring your code:

So let's start top-down with some fictional public method that orchestrates the steps and calls your private methods:

public function doUpdateCustomerStatusStuff(string $customerId): void
{
    $customer = $this->findCustomerById($customerId);
    $operationResult = $this->updateCustomerStatus($customer);

    if ($operationResult->failed()) {
        $this->showMessage($operationResult->getMessage());
    }
}

As you can see it first fetches the customer from the database so that your business logic can now operate on a customer object without having database dependencies.

In the next step we look into the refactored updateCustomerStatus() method:

private function updateCustomerStatus(Customer $customer): OperationResult
{
    if ($this->firstCondition($customer)) {
        return OperationResult::fail('first condition text');
    }

    if ($this->secondCondition($customer)) {
        return OperationResult::fail('second condition text');
    }

    $this->updateCustomer($customer);

    return OperationResult::succeed();
}

I now pass in the Customer object rather than the id so that the logic code can already do it's job.

Now to your original question:

Instead of calling the UI method (with the echo statement) I rather return some Result object that indicates if the operation succeeded or failed and which also provides an optional message. I added some static methods to the OperationResult class just for convenience.

Let's look at the class, it is not complex but does it's job:

class OperationResult
{
    /**
     * @var bool
     */
    private $success;
    /**
     * @var string
     */
    private $message;

    private function __construct(bool $success)
    {

        $this->success = $success;
    }

    private function setMessage(string $message)
    {
        $this->message = $message;
    }

    public static function fail(string $message)
    {
        $result = new OperationResult(false);
        $result->setMessage($message);
    }

    public static function succeed()
    {
        return new OperationResult(true);
    }

    public function failed()
    {
        return !$this->success;
    }

    public function succeeded()
    {
        return $this->success;
    }
}

Now back to the fictional doUpdateCustomerStatusStuff() method:

public function doUpdateCustomerStatusStuff(string $customerId): void
{
    $customer = $this->findCustomerById($customerId);
    $operationResult = $this->updateCustomerStatus($customer);

    if ($operationResult->failed()) {
        $this->showMessage($operationResult->getMessage());
    }
}

Now you can control when and how to output a message (if any) AFTER the business logic with all checking and updating rules has been performed.

You can even go a step further and move all business logic methods (such as updateCustomerStatus() into a separate class which only contains business logic).

Note: I just assumed that firstCondition() and secondCondition() would check for error conditions. But if that is not the case you can of course perform the output of the message also depending on if there is some message in the doUpdateCustomerStatusStuff() method.