php - Extract to method with empty returns

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());
    }
}
Copy code

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();
}
Copy code

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;
    }
}
Copy code

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());
    }
}
Copy code

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.