Hardcoded parts

Posted On February 21, 2022

CONSTANTS

Have you ever saw something like that?

public function changeState($user, $order_id, $state)
    {
        if ($user->role === 'ADMIN') {

           //... do something 
           $order = $this->find($order_id);
           if ( in_array($order->state, ["CREATED, INPROGRESS"]) )
           {
                if (in_array($state, ["CREATED, "INPROGRESS", "CANCELED", "WAITING"]) )
                {
                    $order->state = $state;
                    return true;
                }
           } 
        }

        return false;
    }

We have a few code smells here. I’ve discussed some of them in other articles, such as nested ‘if’ statements and unclear intentions of those conditions. What else should be done to improve it? The answer is: avoid hard-coded definitions of values. Use CONST objects instead. Let’s get started.

First, create ‘const’ classes. We should have two: one for admin roles and another for order states.

class Roles
{
    const ADMIN = 'ADMIN';
    const GUEST = 'GUEST';
    const REGULAR = 'REGULAR';
}

class OrderStates 
{
    const CREATED = "CREATED";
    const INPROGRESS = "INPROGRESS";
    const WAITING = "WAITING;"
    const CANCELED = "CANCELED"
}

Next, add functions to the classes, which will replace the parts of conditions that use those values. Instead of using: “if ($user->role === ‘ADMIN’)”, consider creating something like:

class Roles
{
    //...

    public static function isAdmin($role)
    {
        if($role === self::ADMIN) {
            return true;
        }

        return false;
    }

}

For “if ($state in [“CREATED, “INPROGRESS”, “CANCELED”, “WAITING”])”, which probably is checking if the provided state is in allowed states, we can add a function like:

class OrderStates 
{
    //...

    public static function all()
    {
        $class = new ReflectionClass(__CLASS__);
        return $class->getConstants();

    }

    public static function isValid($state)
    {
        $available_states = self::all();
        return in_array($state, $available_states);
    }

}

Another unclear condition is if ( in_array($order->state, ["NEW", "INPROGRESS"]) ). I’m sure you’re wondering what the intention behind it is. Since I’m the creator of this code, I can clarify 🙂 I wanted to check if the order’s state is allowed to be changed.

Let’s add a method to the class.

class OrderStates 
{
    //...

    public static function allowedToChange($state)
    {
        $allowed_to_change = [
            self::CREATED,
            self::INPROGRESS
        ]

        return in_array($state, $allowed_to_change);
    }

}

Both classes will look like that:

class Roles
{
    const ADMIN = 'ADMIN';
    const GUEST = 'GUEST';
    const REGULAR = 'REGULAR';

    public static function isAdmin($role)
    {
        if($role === self::ADMIN) {
            return true;
        }

        return false;
    }

}

class OrderStates 
{
    const CREATED = "CREATED";
    const INPROGRESS = "INPROGRESS";
    const WAITING = "WAITING;"
    const CANCELED = "CANCELED"

    public static function all()
    {
        $class = new ReflectionClass(__CLASS__);
        return $class->getConstants();

    }

    public static function isValid($state)
    {
        $available_states = self::all();
        return in_array($state, $available_states);
    }

    public static function allowedToChange($state)
    {
        $allowed_to_change = [
            self::CREATED,
            self::INPROGRESS
        ]

        return in_array($state, $allowed_to_change);
    }

}

It’s time for the initial code refactor using new classes and other known rules. The result will be:

use Roles;
use OrderStates;
//...

    public function changeState($user, $order_id, $state)
    {
        if(!OrderStates::isValid($state) ) {
            return false;
        }

        if (Roles::isAdmin($user->role) ) {

           //... do something 
           $order = $this->find($order_id);
           $this->changeState($order, $state);

        }

        return false;
    }

    private function changeState($order, $state)
    {
        if ( OrderStates::allowedToChange($order->state) ) {

            $order->state = $state;
            return true;
       } 

       return false;
    }

I believe it looks much better now, but there’s still one aspect that needs improvement. Let’s consider: who owns the order state data? I believe it’s the class of the $order object. The rule is that the owner of the data should determine whether it can be changed and should have the capability to change it. Based on this, we should move this to the “owner” class.

Finally

the code should look like that:

use Roles;
//...

    public function changeState($user, $order_id, $state)
    {
        if (Roles::isAdmin($user->role) ) {

           //... do something 

           $order = $this->find($order_id);
           return $order->changeState($state);
        }

        return false;
    }

use OrderStates;

class Order 
{
    //...

    public function changeState($state)
    {
        if(!OrderStates::isValid($state) ) {
            return false;
        }

        if (OrderStates::allowedToChange($this->state) ) {
            $this->state = $state;
            return true;
        } 

       return false;
    }
}  

class Roles
{
    const ADMIN = 'ADMIN';
    const GUEST = 'GUEST';
    const REGULAR = 'REGULAR';

    public static function isAdmin($role)
    {
        if($role === self::ADMIN) {
            return true;
        }

        return false;
    }
}

class OrderStates 
{
    const CREATED = "CREATED";
    const INPROGRESS = "INPROGRESS";
    const WAITING = "WAITING;"
    const CANCELED = "CANCELED"

    public static function all()
    {
        $class = new ReflectionClass(__CLASS__);
        return $class->getConstants();
    }

    public static function isValid($state)
    {
        $available_states = self::all();
        return in_array($state, $available_states);
    }

    public static function allowedToChange($state)
    {
        $allowed_to_change = [
            self::CREATED,
            self::INPROGRESS
        ]

        return in_array($state, $allowed_to_change);
    }
}