Hard-coded parts and create constants (how to use them)
Constants
Have you ever met something like this:
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 |
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"]) { $order->state = $state; return true; } } } return false; } |
We have a few code smells here. I’ve talked about a few of these in previous articles, which are nested if statements and the intent of these conditions is unclear. What else should be done to improve it?
Answer is: avoid the hard-coded definitions of values, simply use CONST objects instead. Let’s start.
Firstly, create const classes. We should have two – 1st for admin roles and 2nd for order states.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 |
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" } |
Secondly, add functions into the classes, which will replace the conditions parts that use these values.
Instead of: “If ($user->roles === “ADMIN”), create something like:
|
1 2 3 4 5 6 7 8 9 10 11 12 13 |
class Roles { //... public static function isAdmin($role) { if(role === self::ADMIN) { return true } return false } } |
For “if ($state in[“CREATED”, “INPROGRESS”, “CANCELED”, “WAITING”]”, which is checking if the provided state is in the allowed states, therefore we can add a function like:
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 |
class OrderStates { //... public state 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 don’t know what is the intention of it, but since i’m the creator of this code, i know that I simply wanted to check if order, in which is in state, is allowed to be changed.
Lets add method in the class
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 |
class OrderStates { //... public static function allowedToChange($state) { $allowed_to_change = [ self::CREATED, self::INPROGRESS ] return in_array($state, $allowed_to_change); } } |
As a result, both classes will look like that:
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 |
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" const = "INPROGRESS" const = "WAITING" const = "CANCELED" public state function all() { $class = new ReflectionClass (__CLASS__); return $class->getConstants(); } public state function isValid($state) { $available_states = self::all(); return in_array($state, $available_states); } public state function allowedToChange($state) { $allowed_to_change - [ self::CREATED self:INPROGRESS ] return in_array($state, $allowed_to_change); } } |
It’s time for the initial code reflector with using new classes and other knowing rules. The result of that will be:
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 |
use Roles; use OrderStates; //... public funtion changeState($user, $user_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 think it looks much better now, but there’s still one thing to improve. Lets consider – who is the owner of the order state data? I think it’s the $order object, because the rule is, that the owner of the data should decide if it can be changed and is able to change it. Therefore we should move it to the “owner” class.
Finally
The code should look like that:
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 |
use Roles //... public function changeState($user, $user_id, $state) { if(Roles::isAdmin($user->role) ) { //... $order = $this->find($order_id); return $order->changeState($state); } return false; } |
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 |
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 } } |
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 |
class Roles { const ADMIN = 'ADMIN'; const GUEST = 'GUEST'; const REGULAR = 'REGULAR'; public static function isAdmin($role) { if($role === self::ADMIN) { return true; } return false; } } |
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 |
class OrderStates { const CREATED = "CREATED"; const INPROGRESS = "INPROGRESS"; const WAITING = "WAITING"; const CANCELED = "CANCELED" public static function all() { $class= new RelfectionClass (__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) } } |
Subsequent articles
- Static Factory Methods aka Named Constructors: Read more…
- Refactoring Example – guard clauses: 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…
