Refactoring Example - guard clauses (they are used for)

Refactoring Example – guard clauses (they are used for)

I think the best learning way is to try on real causes. Below I will present a case from our real project. How we handle the refactoring of a small part of this older code. This article will not present all the details of this project. A few things have been simplified to make it more understandable.

Story: This part of code is responsible to assing a proper user to the order. We have some assignments in the system. Based on certain conditions, we need to decide whether and to which type of user we should delegate an order. It’s a small “monster” so scroll down as fast as you can…

class DelegateBackOrderToPreviousOperatorManager { /** @var int */
    protected $tuning_team_id = 0;
    
    /**
    * Delegates an order back to user who delegated it
    * @param Order $order
    * @return void    */
    public function checkAndDelegateBack (Order $order)    {

    if ($order->hasNotAcceptedOperator()) {
        $current_not_accepted_operator = $order->currentOperator;
        $previous_operator = $this->getPreviousOperator($order);
        $this->tuning_team_id = $order->seller->setting->team_id;

        if ($previous_operator) {
            if ($this->previousIsNotATunerAndCurrentIsNotATunerAndIsOnline($previous_operator, $current_not_accepted_operator)
            || $this->bothAreTunersAndPreviousIsOnline($previous_operator, current_not_accepted_operator)) {
                //delegate to previous
                $order->setCurrentOperator(4previous_operator, 1);
                $previous_operator->notify(new OrderDelegatedBack($order, current_not_accepted_operator));
                Log: :info('Order ID:'.$order->id.
                    ' was delegatedback to user: '.$previous_operator->fullname());
                return;
            }

            if ($this->previousIsNotATunerAndCurrentIsTuner($previous_operator, $current_not_accepted_operator)
                || $this->bothAreTunersAndCurrentIsOffline($previous_operator, $current_not_accepted_operator)) {
                //delegate to team
                    $order->setCurrentTeam($this->tuning_team_id);
                    Log: :info('Order ID: ',$order->id,' was delegated to TUNING TEAM!');
                    return;
            }
            
            Log: :info('Order ID:',$order->id,' was not delegated back - because previous user is OFFLINE!');
                return;
        }

        Log: :info('Order ID:',$order->id,' was not delegated back - because there was not previous user!');
            return;
    }

    Log: :info('Order ID:',$order->id,' was not delegate back - because is actually accepted!');
    return;
}

/**
*@param Order $order
*@return User|null    */
protected function getPreviousOperator(Order $order) {
        $log = $order->histories()->where('current_operator_id, '>', 0)->where('current_operator_accepted', 0)->latest()->first();
        return $log ? $log->author ; null;
}

/**
*@param User $prewious_operator
*@param User $current_not_accepted_operator
*@return bool    */
protected function previousIsNotATunerAndCurrentIsNotATunerAndIsOnline(User $previous_operator, User current_not_accepted_operator) {

    return |$previous_operator->IsMemberOf($this->tuning_team_id) && |current_not_accepted_operator->IsMemberOf($this->tuning_team_id) && $current_not_accepted_operator->IsOnline();
}

/**
*@param User $previous_operator
*@param User $current_not_accepted_operator
*@return bool    */
protected function previousIsNotATunerAndCurrentIsTuner(User $previous_operator, User $current_not_accepted_operator) {

    return |$previous_operator->IsMemberOf($this->tuning_team_id)
        && $current_not_accepted_operator->IsMemberOf($this->tuning_team_id)
        && $previous_operator->IsOnline();
}

/**
*@param User $previous_operator
*@param User $current_not_accepted_operator
@return bool   */
protected function bothAreTunersAndCurrentIsOffline(User $previous_operator, User $current_not_accepted_operator) {

        return $previous_operator->IsMemberOf($this->tuning_team_id)
            && $current_not_accepted_operator->IsMemberOf($this->tuning_team_id)
            && |$previous_operator->IsOnline();
    }
}

class DelegateBackOrderToPreviousOperatorManager { /** @var int */

protected $tuning_team_id = 0;

/**

* Delegates an order back to user who delegated it

* @param Order $order

* @return void */

public function checkAndDelegateBack (Order $order) {

if ($order->hasNotAcceptedOperator()) {

$current_not_accepted_operator = $order->currentOperator;

$previous_operator = $this->getPreviousOperator($order);

$this->tuning_team_id = $order->seller->setting->team_id;

if ($previous_operator) {

if ($this->previousIsNotATunerAndCurrentIsNotATunerAndIsOnline($previous_operator, $current_not_accepted_operator)

|| $this->bothAreTunersAndPreviousIsOnline($previous_operator, current_not_accepted_operator)) {

//delegate to previous

$order->setCurrentOperator(4previous_operator, 1);

$previous_operator->notify(new OrderDelegatedBack($order, current_not_accepted_operator));

Log: :info('Order ID:'.$order->id.

' was delegatedback to user: '.$previous_operator->fullname());

return;

}

if ($this->previousIsNotATunerAndCurrentIsTuner($previous_operator, $current_not_accepted_operator)

|| $this->bothAreTunersAndCurrentIsOffline($previous_operator, $current_not_accepted_operator)) {

//delegate to team

$order->setCurrentTeam($this->tuning_team_id);

Log: :info('Order ID: ',$order->id,' was delegated to TUNING TEAM!');

return;

}

Log: :info('Order ID:',$order->id,' was not delegated back - because previous user is OFFLINE!');

return;

}

Log: :info('Order ID:',$order->id,' was not delegated back - because there was not previous user!');

return;

}

Log: :info('Order ID:',$order->id,' was not delegate back - because is actually accepted!');

return;

}

/**

*@param Order $order

*@return User|null */

protected function getPreviousOperator(Order $order) {

$log = $order->histories()->where('current_operator_id, '>', 0)->where('current_operator_accepted', 0)->latest()->first();

return $log ? $log->author ; null;

}

/**

*@param User $prewious_operator

*@param User $current_not_accepted_operator

*@return bool */

protected function previousIsNotATunerAndCurrentIsNotATunerAndIsOnline(User $previous_operator, User current_not_accepted_operator) {

return |$previous_operator->IsMemberOf($this->tuning_team_id) && |current_not_accepted_operator->IsMemberOf($this->tuning_team_id) && $current_not_accepted_operator->IsOnline();

}

/**

*@param User $previous_operator

*@param User $current_not_accepted_operator

*@return bool */

protected function previousIsNotATunerAndCurrentIsTuner(User $previous_operator, User $current_not_accepted_operator) {

return |$previous_operator->IsMemberOf($this->tuning_team_id)

&& $current_not_accepted_operator->IsMemberOf($this->tuning_team_id)

&& $previous_operator->IsOnline();

}

/**

*@param User $previous_operator

*@param User $current_not_accepted_operator

@return bool */

protected function bothAreTunersAndCurrentIsOffline(User $previous_operator, User $current_not_accepted_operator) {

return $previous_operator->IsMemberOf($this->tuning_team_id)

&& $current_not_accepted_operator->IsMemberOf($this->tuning_team_id)

&& |$previous_operator->IsOnline();

}

Few comments about it:

As you can see above, the code is difficult to read or even copy to show to someone else.

Code smells:

Long method and class names (more than 3 words. This probably means that we have too much logical responsibility in a single function class).
multiple nested conditions + more then 2 indents levels on methods.

The good thing about this situation is that we at least have a flow diagram. It explains the flow of data and the business logic behind it. Comparing the diagram, we see that previously the code was created from top to bottom. (This is probably why it nested the if conditions.)

Refactor decisions:

replace nested conditions with guard clauses
rename the methods and class using max 3 words (if not possible then it means that the code should be changed/split)
tell don’t ask rules

We also decide, that to use guard clauses, we need to start from the goal – so our point is that the destination are the 3 results (no action, delegate to previous, delegate to team), and based on that we will add the guards which check conditions.

After few renaming iterations and methods changes we reached a first version of refactored code which looks like that:

class OrdersDelegateManager {
    protected $tuning_team_id = 0;
    
    public function delegate(Order $order) {
    
        if (|$order-> hasNotAcceptedOperator()) {
            Log: :info('Order ID',$order->id,' was not delegate back - because is actually accepted!');
            return;
        }

        $previous_operator = $this-> getPreviousOperator($order);
        if ($previous_operator) { return; }

        $this-> tuning_team_id = $order-> seller->settings->team_id;
        $previous_tuner = $previous_operator->IsMemberOf($this->tuning_team_id);
        $current_tuner = $order->currentOperator->IsMemberOf($this->tuning_team_id);
        $previous_online = $previousOperator->IsOnline();


        if ($this-> previousDelegationValid($previous_tuner, $current_tuner, $previous_online)) {
            $this-> delegateToPrevious($order, $previous_operator, $order->currentOperator);
            return;
        }

        if ($this-> TeamDelegationValid($previous_tuner, $current_tuner, $previous_online) {
            $this-> delegateToTeam($order);
            return;
        }

        Log: :info('Order ID:',$order->id,' was not delegated back - because previous user is OFFLINE!');
    }

    protected function previousDelegationValid($previous_tuner, $current_tuner, $previous_online) {

         if ($previous_tuner === true && $current_tuner === true && $previous_online === true) {
             return true;
         }

         if ($previous_tuner === false && $current_tuner === false && $previous_online === false) {
             return true;
         }
         
         return false;
    }

    protected function teamDelegationValid($previous_tuner, $current_tuner, $previous_online) {

        if ($previous_tuner === true && $current_tuner === true && $previous_online === false) {
            return true;
        }

        if ($previous_tuner === false && $current_tuner === true) {
            return true;
        }

        return false;
    }
    protected function delegateToPrevious(Order $order, User $previous_operator, User $current_operator) {

        $order->setCurrentOperator($previous_operator, 1);
        $previous_operator->notify(new OrderDelegatedBack($order, $current_operator));
        Log: :info('Order ID:',$order->id,' was delegated back to user: ',$previous_operator->fullname());
    }
    protected function delegateToTean($order) {

        $order->setCurrentTeam($this->tuning_team_id);
        Log: :info('Order ID:',$order->id,' was delegated to TUNING TEAM!');
    }

    protected function getPreviousOperator(Order $order) {

        $log = $order->histories()->where('current_operator_id', '>', 0)->where('current_operator_accepted', 0)->latest()->first();

        if ($log) {
            return $log->author;
        }

        Log: :info('Order ID:',$order->id,' was not delegated back - because there was not previous user!');
            return false;
    }
}

class OrdersDelegateManager {

protected $tuning_team_id = 0;

public function delegate(Order $order) {

if (|$order-> hasNotAcceptedOperator()) {

Log: :info('Order ID',$order->id,' was not delegate back - because is actually accepted!');

return;

}

$previous_operator = $this-> getPreviousOperator($order);

if ($previous_operator) { return; }

$this-> tuning_team_id = $order-> seller->settings->team_id;

$previous_tuner = $previous_operator->IsMemberOf($this->tuning_team_id);

$current_tuner = $order->currentOperator->IsMemberOf($this->tuning_team_id);

$previous_online = $previousOperator->IsOnline();

if ($this-> previousDelegationValid($previous_tuner, $current_tuner, $previous_online)) {

$this-> delegateToPrevious($order, $previous_operator, $order->currentOperator);

return;

}

if ($this-> TeamDelegationValid($previous_tuner, $current_tuner, $previous_online) {

$this-> delegateToTeam($order);

return;

}

Log: :info('Order ID:',$order->id,' was not delegated back - because previous user is OFFLINE!');

}

protected function previousDelegationValid($previous_tuner, $current_tuner, $previous_online) {

if ($previous_tuner === true && $current_tuner === true && $previous_online === true) {

return true;

}

if ($previous_tuner === false && $current_tuner === false && $previous_online === false) {

return true;

}

return false;

}

protected function teamDelegationValid($previous_tuner, $current_tuner, $previous_online) {

if ($previous_tuner === true && $current_tuner === true && $previous_online === false) {

return true;

}

if ($previous_tuner === false && $current_tuner === true) {

return true;

}

return false;

}

protected function delegateToPrevious(Order $order, User $previous_operator, User $current_operator) {

$order->setCurrentOperator($previous_operator, 1);

$previous_operator->notify(new OrderDelegatedBack($order, $current_operator));

Log: :info('Order ID:',$order->id,' was delegated back to user: ',$previous_operator->fullname());

}

protected function delegateToTean($order) {

$order->setCurrentTeam($this->tuning_team_id);

Log: :info('Order ID:',$order->id,' was delegated to TUNING TEAM!');

}

protected function getPreviousOperator(Order $order) {

$log = $order->histories()->where('current_operator_id', '>', 0)->where('current_operator_accepted', 0)->latest()->first();

if ($log) {

return $log->author;

}

Log: :info('Order ID:',$order->id,' was not delegated back - because there was not previous user!');

return false;

}

Comments:

The code is not so neasted, buts it’s still hard to read (to many things going on in this class), it’s to much “procedular” then “OOP”

We made a decision to split it to smaller classes, so that each of two successfull results become a separate object with it’s own conditions to check. After that the main manager class looks like below. As you can see the main class is quite small and readable now. We have 3 possible results of it, but only two boundaries to check.

Can we delegate to previous user?
Can we delegate to team? And we have the tell – don’t ask rule used.

class OrderDelegateManager {

    protected $tuning_team_id = 0;

    public function delegate(Order $order) {

        if (|$order->hasNotAcceptedOperator()) {
            Log: :info('Order ID:',$order->id,' was not delegated back - because is actually accepted');
            return;
        }

        $previous_operator = $this->getPreviousOperator($order);

        if (|$previous_operator) {
            return;
        }

        if (new PreviousDelegator()->delegate($order, $previous_operator, $order->currentOperator)) {
            Log: :info('Order ID:',$order->id,' was delegated back to user: ',$previous_operator->fullname());
            return;
        }

        if (new TeamDelegator()->delegate($order, $previous_operator, $order->currentOperator)) {
            Log: :info('Order ID:',$order->id,' was delegated to TUNING TEAM!');
            return;
        }

        Log: :info('Order ID:',$order->id,' was not delegated back - because previous user is OFFLINE!');
    }

    protected function getPreviousOperator(Order $order) {

        $log = $order->histories()->where('current_operator_id, '>', 0)->where('current_operator_accepted', 0)->latest()->first();

        if ($log) { return $log->author; }

        Log: :info('Order ID:',$order->id,' was not delegated back - because there was not previouse user!');
        return false;
    }
}

class OrderDelegateManager {

protected $tuning_team_id = 0;

public function delegate(Order $order) {

if (|$order->hasNotAcceptedOperator()) {

Log: :info('Order ID:',$order->id,' was not delegated back - because is actually accepted');

return;

}

$previous_operator = $this->getPreviousOperator($order);

if (|$previous_operator) {

return;

}

if (new PreviousDelegator()->delegate($order, $previous_operator, $order->currentOperator)) {

Log: :info('Order ID:',$order->id,' was delegated back to user: ',$previous_operator->fullname());

return;

}

if (new TeamDelegator()->delegate($order, $previous_operator, $order->currentOperator)) {

Log: :info('Order ID:',$order->id,' was delegated to TUNING TEAM!');

return;

}

Log: :info('Order ID:',$order->id,' was not delegated back - because previous user is OFFLINE!');

}

protected function getPreviousOperator(Order $order) {

$log = $order->histories()->where('current_operator_id, '>', 0)->where('current_operator_accepted', 0)->latest()->first();

if ($log) { return $log->author; }

Log: :info('Order ID:',$order->id,' was not delegated back - because there was not previouse user!');

return false;

}

Those two additional classes looks like below. They are not perfect yet!!! (there is few lines duplicated in the code, but I’m sure you can handle it on your own and made it better (maybe add some interface and extract abstract class? Add some doc block?) Also I’m sure you can see where the guard clauses are?

class PreviousDelegator {

    public function delegate($order, $previous_operator, $current_operator) {

        $tuning_team_id = $order->seller->settings0>team_id;
        $previous_tuner = $previous_operator->IsMemberOf($tuning_team_id);
        $current_tuner = $order->currentOperator->IsMemberOf($tuning_team_id
        $previous_online = $previous_operator->IsOnline();

        if  ($this->checkConditions($previous_tuner, $current_tuner, $previous_online)) {
            $order->setCurrentOperator($previous_operator, 1);
            $previous_operator->notify(new OrderDelegatedBack($order, $current_operator));
            return true;
        }

        return false;
    }

    protected function checkConditions($previous_tuner, $current_tuner, $previous_online) {

        if ($previous_tuner === true && $current_tuner === true && $Previous_online === true) {
            return true;
        }

        if ($previous_tuner === false && $current_tuner === false && $previous_online === true) {
            return true;
        }

        return false;
    }
}

class PreviousDelegator {

public function delegate($order, $previous_operator, $current_operator) {

$tuning_team_id = $order->seller->settings0>team_id;

$previous_tuner = $previous_operator->IsMemberOf($tuning_team_id);

$current_tuner = $order->currentOperator->IsMemberOf($tuning_team_id

$previous_online = $previous_operator->IsOnline();

if ($this->checkConditions($previous_tuner, $current_tuner, $previous_online)) {

$order->setCurrentOperator($previous_operator, 1);

$previous_operator->notify(new OrderDelegatedBack($order, $current_operator));

return true;

}

return false;

}

protected function checkConditions($previous_tuner, $current_tuner, $previous_online) {

if ($previous_tuner === true && $current_tuner === true && $Previous_online === true) {

return true;

}

if ($previous_tuner === false && $current_tuner === false && $previous_online === true) {

return true;

}

return false;

}

class TeamDelegator {

    public function delegate($order, $previous_operator, $current_operator) {

        $tuning_team_id = $order->seller->settings->team_id;
        $previous_tuner = $previous_operator->IsMemberOf($tuning_team_id);
        $current_tuner = $order->currenOperator->IsMemberOf($tuning_team_id);
        $previous_online = $previous_operator->IsOnline();

        if ($this->checkConditions($previous_tuner, $current_tuner, $previous_online)) {
            $order->setCurrentTeam($tuning_team_id);
            return true;
        }

        return false;
    }

    protected function checkConditions($previous_tuner, $current_tuner, $previous_online) {

        if ($previous_tuner === true && $current_tuner === true && $previous_online === false) {
            return true;
        }

        if ($previous_tuner === false && $current_tuner === true) {
            return true;
        }

        return false;
    }
}

class TeamDelegator {

public function delegate($order, $previous_operator, $current_operator) {

$tuning_team_id = $order->seller->settings->team_id;

$previous_tuner = $previous_operator->IsMemberOf($tuning_team_id);

$current_tuner = $order->currenOperator->IsMemberOf($tuning_team_id);

$previous_online = $previous_operator->IsOnline();

if ($this->checkConditions($previous_tuner, $current_tuner, $previous_online)) {

$order->setCurrentTeam($tuning_team_id);

return true;

}

return false;

}

protected function checkConditions($previous_tuner, $current_tuner, $previous_online) {

if ($previous_tuner === true && $current_tuner === true && $previous_online === false) {

return true;

}

if ($previous_tuner === false && $current_tuner === true) {

return true;

}

return false;

}

Subsequent articles

Static Factory Methods aka Named Constructors: Read more…

Refactoring Example – guard clauses: Read more…

Hard-coded parts: Read more…

Dependency Injection code smells: Read more…

Naming conventions: Read more…

Improving conditions clauses – few small things with hudge impact: Read more…

Clean Code – how to start: Read more…