PHP Laravel Order generation Custom Class

Question

Here is a custom class I use to handle different types of orders in Laravel. This question includes the code regarding Order Type A. I want to know whether I can simplify the code. I have different Order Types with the same functions but different in behaviour. That's why I'm using an abstract class to extend all order types.

abstract class BaseOrder
{
    abstract public function getOrder(Order $order);

    abstract public function storeOrder(Request $request);

    ...
}

class OrderTypeA extends BaseOrder
{
    private $type = 'A';

    public function getOrder(Order $order)
    {
        return $order->load('items.product')
            ->makeHidden(['order_no', 'created_at', 'updated_at', 'created_by']);
    }

    public function storeOrder($request)
    {
        $returnedValues = DB::transaction(function () use ($request) {

            $order = new Order;
            $order->type = $this->type;
            $order->from = $request->json('from');
            $order->amount = $request->json('amount');
            $order->status = 2;
            $order->save();

            foreach ($request->json('items') as $item) {
                $order->items()->create([
                    'product_id' => $item['product_id'],
                    'quantity' => $item['quantity'],
                    'total' => $item['total'],
                ]);
            }

            return compact('order');
        });

        return $returnedValues['order']->load('items');
    }

    public function updateOrder($request)
    {
        $returnedValues = DB::transaction(function () use ($request) {

            if ($request->id) {
                $order = Order::findOrFail($request->id);
                $order->amount = $request->json('amount');
                $order->status = 1;
                $order->save();

                $order->items()->delete();

                foreach ($request->json('items') as $item) {
                    $order->items()->create([
                        'product_id' => $item['product_id'],
                        'quantity' => $item['quantity'],
                        'total' => $item['total'],
                    ]);
                }
            } else {
                $order = new Order;
                $order->type = $this->type;
                $order->from = $request->json('from');
                $order->to = null;
                $order->amount = $request->json('amount');
                $order->status = 1;
                $order->save();

                foreach ($request->json('items') as $item) {
                    $order->items()->create([
                        'product_id' => $item['product_id'],
                        'quantity' => $item['quantity'],
                        'total' => $item['total'],
                    ]);
                }
            }
            return compact('order');
        });

        return $returnedValues['order']->load('items');
    }

    public function generateOrder($customer)
    {
        $order = new stdClass();
        $order->id = null;
        $order->type = $this->type;
        $order->from = $customer->id;
        $order->to = null;
        $order->status = 1;

        $totalAmount = 0;
        $items = [];
        $stocks = Stock::where('type', 1)
            ->where('customer_id', $customer->id)
            ->whereRaw('stock <= reorder_level')
            ->get();

        if (count($stocks) > 0) {
            foreach ($stocks as $key => $stock) {
                if ($stock->minimum_order_quantity != 0) {
                    $priceListItem = PriceList::where('product_id', $stock->product_id)->latest()->first();

                    $item = new stdClass;
                    $item->id = $key;
                    $item->product_id = $stock->product_id;
                    $item->product = Product::findOrFail($stock->product_id);
                    $item->order_id = null;
                    $item->quantity = (int) $stock->minimum_order_quantity;
                    $item->total = ((int) $stock->minimum_order_quantity * ((float) $priceListItem->price));
                    $item->is_checked = true;
                    $item->created_at = null;
                    $item->updated_at = null;

                    $totalAmount += $item->total;
                    $items[] = $item;
                }
            }
        }

        $order->amount = $totalAmount;
        $order->items = $items;

        return $order;
    }

    public function deleteOrder(Order $order)
    {
        if ($order->status == 'Draft') {
            $order->forceDelete();
        } else {
            $order->delete();
        }
        return $order;
    }
}

Answer 1

main “question”

I want to know whether I can simplify the code more.

Redundant code

There are three places in the code where an order is either created or loaded before being saved and having its items saved. This code could be abstracted to a separate method - perhaps on BaseOrder, though it is difficult to know without seeing the other subclassses - that accepts a request object and status. If that request object has an id then it will:

• call Order::findOrFail()
• call $order->items()->delete();

otherwise it will:

• call Order::create() (or instantiate with the new operator if preferred)
• Set the type property
• Set the from property
• Sets the to property to null?

And then in both cases:

• sets the amount property
• Saves the order
• loops over $request->json('items') to create items.

If that method is moved up to the base class, then I would consider whether type really needs to be a member/instance variable. If not, it could just be a constant that is set it each class and referenced with the static class (thanks to Late static binding). Additionally, could the class name be used? E.g, $this->type = str_replace('OrderType', '', static::class);

Other suggestions

returning early to minimize indentation

In the foreach loop within the generateOrder() method

  if ($stock->minimum_order_quantity != 0) { 
      $priceListItem = PriceList::where('product_id', $stock->product_id)->latest()->first();

Look at how long that line is. Also consider that it is indented 5 times! One tactic to reduce indentation levels is to return early - in this case use continue:

if ($stock->minimum_order_quantity === 0) {
    continue;
}
$priceListItem = PriceList::where('product_id', $stock->product_id)->latest()->first();

Also consider following PSR-12 section 2.3 and limit the line length to 80 characters:

Lines SHOULD NOT be longer than 80 characters; lines longer than that SHOULD be split into multiple subsequent lines of no more than 80 characters each.

That long line could be split to multiple for readability:

$priceListItem = PriceList::where('product_id', $stock->product_id)
                          ->latest()
                          ->first();

arbitrary values

There are a couple places where the status field is set to an integer value e.g.

   $order->status = 2;

And

$order->status = 1;

I can’t tell from the code what those values mean, and neither will anybody else (possibly including your future self!).

It would be wise to declare constants for those- I can only guess as to what to call them but use appropriate names-

class OrderTypeA extends BaseOrder
{
    const STATUS_CREATED = 1;
    const STATUS_UPDATED = 2;

Maybe it makes sense to declare those on BaseOrder?

Then if somebody sees a line like this:

$order->status = self::STATUS_UPDATED;

That person can tell what the value is. Also if the value ever changes it can be updated in a single spot instead of everywhere it is used throughout the code.