Adjusting variables on change of status attribute of my Order Laravel model on the updated event in observer

Question

I have an order model and a client model. A client has many orders, an order belongs to a single client.

In the client model I keep track of how much money a client paid (total_generated_revenue), how many orders he has cancelled (orders_cancelled), and how many of his orders were completed (orders_got).

In the order model I have a status attribute and a total attribute (and the client_id attribute, of course).

When status of an order changes, I change other properties of the object with respect to the exact change that happened in status. Actions on each change are different. Status can either be 0, 1, or 2, so there are 6 possible changes in status. I handle this with 7 if statements:

public function updated(Order $order)
{
 $unchanged_order = $order->getOriginal();
 if($unchanged_order->status == $order->status)
 {
 // skip if status wasn't changed. I use this skip so that we don't have to check all of the following. 
 }
 elseif($unchanged_order->status == 0 && $order->status == 1)
 {
 $order->client->total_generated_revenue += $order->total;
 $order->client->orders_got += 1;
 }
 elseif($unchanged_order->status == 0 && $order->status == 2)
 {
 $order->client->orders_cancelled += 1;
 }
 elseif($unchanged_order->status == 1 && $order->status == 2)
 {
 $order->client->total_generated_revenue -= $order->total;
 $order->client->orders_got -= 1;
 $order->client->orders_cancelled += 1;
 }
 elseif($unchanged_order->status == 1 && $order->status == 0)
 {
 $order->client->total_generated_revenue -= $order->total;
 $order->client->orders_got -= 1;
 }
 elseif($unchanged_order->status == 2 && $order->status == 1)
 {
 $order->client->total_generated_revenue += $order->total;
 $order->client->orders_got += 1;
 $order->client->orders_cancelled -= 1;
 }
 elseif($unchanged_order->status == 2 && $order->status == 0)
 {
 $order->client->orders_cancelled -= 1;
 }
 $order->client->save();
}

Is it considered a bad practice? If so, how can I refactor this code?

Does the name $unchanged_order reflect well enough that I mean the version before change? Should I rename it into $order_before_change?

• mickmackusa: Originally posted at Stack Overflow: stackoverflow.com/q/69044629/2943403
• mickmackusa: Can you clarify what the "magic numbers" 0, 1, and 2 mean within your application?
• Bogdan Ta: @mickmackusa 0 means in progress, 1 means completed, 2 means cancelled

Answer 1

Nine status combinations

Status can either be 0, 1, or 2, so there are 8 possible changes in status.

There are nine (three states multiplied by three states) possible "changes" (state combinations). Although three of them are to stay the same. This is why you need seven if statements (and actually you don't, the last could be a simple else): six to cover the possible changes and one to cover the three possible ways to stay the same. If there were only two statuses, this would be four (two times two) and three (one plus two times one). If four, then sixteen (four times four) and thirteen (one plus four times three).

In general if there are \$n\$ states, then there will be \$n^2\$ combinations and \1ドル + n(n - 1) = n^2 -n + 1\$ conditions. There are \$n\$ combinations that stay the same. So the total number of combinations minus the number where the status stays the same plus one for all those where the status stays the same.

Naming

 $unchanged_order = $order->getOriginal();

I would find $original_order clearer.

 // skip if status wasn't changed. I use this skip so that we don't have to check all of the following. 

Rather than commenting, it would be simpler just to return in that case. As there is no reason to update the client if nothing has changed.

 // Nothing changed; nothing to do. 

Alternatively, this comment is shorter and better explains what's happening.

Alternative

 if ($original_order->state === $order->state) {
 return;
 }
 if ($original_order->state === 1) {
 $order->client->total_generated_revenue -= $order->total;
 $order->client->orders_got -= 1;
 } elseif ($original_order->state === 2) {
 $order->client->orders_cancelled -= 1;
 }
 if ($order->state === 1) {
 $order->client->total_generated_revenue += $order->total;
 $order->client->orders_got += 1;
 } elseif ($order->state === 2) {
 $order->client->orders_cancelled += 1;
 }

If you work it out, this will have all the same results but with only five comparisons rather than the seven that you used.

It doesn't make much difference with two states, but you could also use two switch statements instead of two if/elseif structures.

Status objects

A more advanced form would be to use objects or classes as the states rather than primitive integers. Then you could replace both structures (not the first if) with something like

 $original_order->status->leave($order);
 $order->status->enter($order);

Then you could make simpler methods, like

namespace Order\Status;
class Cancelled implements \Order\Status
{
 public function enter($order)
 {
 $order->client->orders_cancelled++;
 }

No if/else scaffolding.

That would make it easier to add a status, as you'd just have to create one file and implement the required methods. This makes the update code simpler at the cost of making the status more complex.