alijumaan / laravel-ecommerce

144 stars 71 forks source link

Hide model behavior behind public methods #12

Closed Ch4mpl00 closed 2 years ago

Ch4mpl00 commented 3 years ago

https://github.com/alijumaan/Laravel-Ecommerce/blob/4b88d4b63f121e0dc837a8518f39df93f86d22d1/app/Http/Controllers/Backend/OrderController.php#L27

For example here you're changing order status telling it

$order->update(['status' => 1])

This is not the best approach

  1. Why 1 ? Try to avoid using "magic numbers" in your code. At least you can use a constant Order::STATUS_CONFIRMED
  2. Why not to allow order to decide the way how it will change it's status, by adding public method like:
    class Order extends Model
    {
    public method confirm(): void
    {
      $this->update(['status' => self::STATUS_CONFIRMED])
    }
    }

and then in your controller just call $order->confirm()

But here is another topic just for your information When you change order's status, usually something should happen in other parts of your system e.g. send some email, notify warehouse to hold some products, etc

So you may need to add a model observer to produce an events for such kind of actions.

And one more... In real world app you usually don't want to allow arbitrary order's status change (because as I mentioned before usually it affects outer world). You may need to use kind of state machine and describe a state transitions graph e.g. you may want to disallow to change order status from 'completed' back to 'pending'

alijumaan commented 3 years ago

your notes make sense and I read your review twice I will change the logic to better

Thank you.