Refactoring Example – guard clauses
Posted On October 20, 2021
I believe the most effective way to learn is through hands-on experience with real-world cases. Below, I will discuss a case study from one of our actual projects, focusing on how we managed the refactoring of a specific section of legacy code. Please note that this article won’t delve into all the intricate details of the project; some aspects have been simplified for the sake of clarity.
class DelegateBackOrderToPreviousOperatorManager { /** @var int */
2. protected $tuning_team_id = 0;
3.
4. /**
5. * Delegates an order back to user who delegated it
6. * @param Order $order
7. * @return void */
8. public function checkAndDelegateBack(Order $order) {
9.
10. if ($order->hasNotAcceptedOperator()) {
11. $current_not_accepted_operator = $order->currentOperator;
12. $previous_operator = $this->getPreviousOperator($order);
13. $this->tuning_team_id = $order->seller->settings->team_id;
14.
15. if ($previous_operator) {
16. if ($this->previousIsNotATunerAndCurrentIsNotATunerAndIsOnline($previous_operator, $current_not_accepted_operator)
17. || $this->bothAreTunersAndPreviousIsOnline($previous_operator, $current_not_accepted_operator)) {
18. //delegate to previous
19. $order->setCurrentOperator($previous_operator, 1);
20. $previous_operator->notify(new OrderDelegatedBack($order, $current_not_accepted_operator));
21. Log::info('Order ID:'.$order->id.
22. ' was delegated back to user: '.$previous_operator->fullname());
23. return;
24. }
25.
26. if ($this->previousIsNotATunerAndCurrentIsTuner($previous_operator, $current_not_accepted_operator)
27. || $this->bothAreTunersAndCurrentIsOffline($previous_operator, $current_not_accepted_operator)) {
28. //delegate to team
29. $order->setCurrentTeam($this->tuning_team_id);
30. Log::info('Order ID:'.$order->id.' was delegated to TUNING TEAM !');
31. return;
32. }
33.
34. Log::info('Order ID:'.$order->id.' was not delegated back - because previous user is OFFLINE !');
35. return;
36. }
37.
38. Log::info('Order ID:'.$order->id.' was not delegated back - because there was not previous user !');
39. return;
40. }
41.
42. Log::info('Order ID:'.$order->id.' was not delegated back - because is actually accepted !');
43. return;
44. }
45.
46. /**
47. * @param Order $order
48. * @return User|null */
49. protected function getPreviousOperator(Order $order) {
50. $log = $order->histories()->where('current_operator_id', '>', 0)->where('current_operator_accepted', 0)->latest()->first();
51. return $log ? $log->author : null;
52. }
53.
54. /**
55. * @param User $previous_operator
56. * @param User $current_not_accepted_operator
57. * @return bool */
58. protected function previousIsNotATunerAndCurrentIsNotATunerAndIsOnline(User $previous_operator, User $current_not_accepted_operator) {
59.
60. return !$previous_operator->isMemberOf($this->tuning_team_id) && !$current_not_accepted_operator->isMemberOf($this->tuning_team_id) && $current_not_accepted_operator->isOnline();
61. }
62.
63. /**
64. * @param User $previous_operator
65. * @param User $current_not_accepted_operator
66. * @return bool */
67. protected function previousIsNotATunerAndCurrentIsTuner(User $previous_operator, User $current_not_accepted_operator) {
68.
69. return !$previous_operator->isMemberOf($this->tuning_team_id)
70. && $current_not_accepted_operator->isMemberOf($this->tuning_team_id);
71. }
72.
73. /**
74. * @param User $previous_operator
75. * @param User $current_not_accepted_operator
76. * @return bool */
77. protected function bothAreTunersAndPreviousIsOnline(User $previous_operator, User $current_not_accepted_operator) {
78. return $previous_operator->isMemberOf($this->tuning_team_id)
79. && $current_not_accepted_operator->isMemberOf($this->tuning_team_id)
80. && $previous_operator->isOnline();
81. }
82.
83. /**
84. * @param User $previous_operator
85. * @param User $current_not_accepted_operator
86. * @return bool */
87. protected function bothAreTunersAndCurrentIsOffline(User $previous_operator, User $current_not_accepted_operator) {
88.
89. return $previous_operator->isMemberOf($this->tuning_team_id)
90. && $current_not_accepted_operator->isMemberOf($this->tuning_team_id)
91. && !$previous_operator->isOnline();
92. }
93.
94. }
The Scenario:
The code above is designed to assign the appropriate user to an order. Within our system, we have various orders, and based on specific conditions, we must determine whether—and to which type of user—we should delegate the order. This segment of code is a bit complex, so feel free to scroll quickly if you’re eager to get to the heart of the matter.
Few comments about it:
As you can see above the code is hard to read and even hard to copy-past to show to someone else 😊
Code smells:
- Long methods and class names (more then 3 words, which probably means that we have to much logic responsibilities in single class or function)
- multiple nested conditions + more then 2 indents levels on methods
The good thing in this situation is that we have at least the flow diagram, which explain the data flow and business logic behind. Looking at this diagram we see that previously the code has been created from top to down (and probably that’s why he 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 OrdersDelegateManger {
2. protected $tuning_team_id = 0;
3.
4. public function delegate(Order $order) {
5.
6. if (!$order-> hasNotAcceptedOperator()) {
7. Log::info('Order ID:'.$order->id.' was not delegated back - because is actually accepted !');
8. return;
9. }
10.
11. $previous_operator = $this-> getPreviousOperator($order);
12. if (!$previous_operator) { return; }
13.
14. $this-> tuning_team_id = $order-> seller->settings->team_id;
15. $previous_tuner = $previous_operator->isMemberOf($this->tuning_team_id);
16. $current_tuner = $order->currentOperator->isMemberOf($this->tuning_team_id);
17. $previous_online = $previous_operator->isOnline();
18.
19.
20. if ($this-> previousDelegationValid($previous_tuner, $current_tuner, $previous_online)) {
21. $this-> delegateToPrevious($order, $previous_operator, $order->currentOperator);
22. return;
23. }
24.
25. if ($this-> teamDelegationValid($previous_tuner, $current_tuner, $previous_online) {
26. $this-> delegateToTeam($order);
27. return;
28. }
29.
30. Log::info('Order ID:'.$order->id.' was not delegated back - because previous user is OFFLINE !');
31. }
32.
33. protected function previousDelegationValid($previous_tuner, $current_tuner, $previous_online) {
34.
35. if ($previous_tuner === true && $current_tuner === true && $previous_online === true) {
36. return true;
37. }
38.
39. if ($previous_tuner === false && $current_tuner === false && $previous_online === true) {
40. return true;
41. }
42.
43. return false;
44. }
45.
46. protected function teamDelegationValid($previous_tuner, $current_tuner, $previous_online) {
47.
48. if ($previous_tuner === true && $current_tuner === true && $previous_online === false) {
49. return true;
50. }
51.
52. if ($previous_tuner === false && $current_tuner === true) {
53. return true;
54. }
55.
56. return false;
57. }
58.
59. protected function delegateToPrevious(Order $order, User $previous_operator, User $current_operator) {
60.
61. $order->setCurrentOperator($previous_operator, 1);
62. $previous_operator->notify(new OrderDelegatedBack($order, $current_operator));
63. Log::info('Order ID:'.$order->id.' was delegated back to user: '.$previous_operator->fullname());
64. }
65.
66. protected function delegateToTeam($order) {
67.
68. $order->setCurrentTeam($this->tuning_team_id);
69. Log::info('Order ID:'.$order->id.' was delegated to TUNING TEAM !');
70. }
71.
72. protected function getPreviousOperator(Order $order) {
73.
74. $log = $order->histories()->where('current_operator_id', '>', 0)->where('current_operator_accepted', 0)->latest()->first();
75.
76. if ($log) {
77. return $log->author;
78. }
79.
80. Log::info('Order ID:'.$order->id.' was not delegated back - because there was not previous user !');
81. return false;
82. }
83. }
Notes:
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 “procedural” then “OOP”
We made a decision to split it to smaller classes, so that each of two successful results become a separate object with its 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 OrdersDelegateManger {
3.
4. protected $tuning_team_id = 0;
5.
6. public function delegate(Order $order) {
7.
8. if (!$order->hasNotAcceptedOperator()) {
9. Log::info('Order ID:'.$order->id.' was not delegated back - because is actually accepted !');
10. return;
11. }
12.
13. $previous_operator = $this->getPreviousOperator($order);
14.
15. if (!$previous_operator) {
16. return;
17. }
18.
19. if (new PreviousDelegator()->delegate($order, $previous_operator, $order->currentOperator)) {
20. Log::info('Order ID:'.$order->id.' was delegated back to user: '.$previous_operator->fullname());
21. return;
22. }
23.
24. if (new TeamDelegator()->delegate($order, $previous_operator, $order->currentOperator)) {
25. Log::info('Order ID:'.$order->id.' was delegated to TUNING TEAM !');
26. return;
27. }
28.
29. Log::info('Order ID:'.$order->id. ' was not delegated back - because previous user is OFFLINE !');
30. }
31.
32. protected function getPreviousOperator(Order $order) {
33.
34. $log = $order->histories()->where('current_operator_id', '>', 0)->where('current_operator_accepted', 0)->latest()->first();
35.
36. if ($log) { return $log->author; }
37.
38. Log::info('Order ID:'.$order->id.' was not delegated back - because there was not previous user !');
39. return false;
40. }
41. }
These 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 blocks?)😊 Also I’m sure you can see where the guard clauses are?
class PreviousDelegator {
2.
3. public function delegate($order, $previous_operator, $current_operator) {
4.
5. $tuning_team_id = $order->seller->settings->team_id;
6. $previous_tuner = $previous_operator->isMemberOf($tuning_team_id);
7. $current_tuner = $order->currentOperator->isMemberOf($tuning_team_id);
8. $previous_online = $previous_operator->isOnline();
9.
10. if ($this->checkConditions($previous_tuner, $current_tuner, $previous_online)) {
11. $order->setCurrentOperator($previous_operator, 1);
12. $previous_operator->notify(new OrderDelegatedBack($order, $current_operator));
13. return true;
14. }
15.
16. return false;
17. }
18.
19. protected function checkConditions($previous_tuner, $current_tuner, $previous_online) {
20.
21. if ($previous_tuner === true && $current_tuner === true && $previous_online === true) {
22. return true;
23. }
24.
25. if ($previous_tuner === false && $current_tuner === false && $previous_online === true) {
26. return true;
27. }
28.
29. return false;
30. }
31. }
1. class TeamDelegator {
2.
3. public function delegate($order, $previous_operator, $current_operator) {
4.
5. $tuning_team_id = $order->seller->settings->team_id;
6. $previous_tuner = $previous_operator->isMemberOf($tuning_team_id);
7. $current_tuner = $order->currentOperator->isMemberOf($tuning_team_id);
8. $previous_online = $previous_operator->isOnline();
9.
10. if ($this->checkConditions($previous_tuner, $current_tuner, $previous_online)) {
11. $order->setCurrentTeam($tuning_team_id);
12. return true;
13. }
14.
15. return false;
16. }
17.
18. protected function checkConditions($previous_tuner, $current_tuner, $previous_online) {
19.
20. if ($previous_tuner === true && $current_tuner === true && $previous_online === false) {
21. return true;
22. }
23.
24. if ($previous_tuner === false && $current_tuner === true) {
25. return true;
26. }
27.
28. return false;
29. }
30. }