Refactor Laravel controller

I am trying to refactor my code. My question is: how can I refactor a method that saves data in several tables at the same time?

I think writing this code is essential. On the other hand, it is too long for a controller's method. Can I move some of them to a Model?

I have a product that it has some relations such as Price, specials, maintenance, etc.

public function store(AddProductRequest $request)
{
    $shopperId = 1;

    $product = new Product([
        'title' => $request->title,
        'slug' => to_slug_fa($request->title, Product::class),
        'detail' => $request->detail,
        'color_id' => $request->colorId,
        'category_id' => $request->categoryId,
        'img' => $request->img,
        'description' => $request->description,
    ]);

    $category = Category::find($request->categoryId);

    $category->Product()->save($product);

    $code = create_code($request->categoryId, $product['id'], $request->colorId);

    $product->update(['code' => $code]);

    $price = new Price([
        'price' => $request->price,
        'product_id' => $product->id,
    ]);

    $product->price()->save($price);

    $stocks = [];

    foreach ($request->sizeIds as $value) {
        if ($request->quantity[$value] != null) {
            $stocks[] = new Stock([
                'stock' => $request->quantity[$value],
                'product_id' => $product->id,
                'size_id' => $value,
                'shopper_id' => $shopperId,
            ]);
        }
    }

    $product->stock()->saveMany($stocks);

    $productAttributes = [];

    foreach ($request->attributeIds as $value) {
        if (isset($request->productAttribute[$value])) {
            $productAttributes[] = new ProductAttribute([
                'product_id' => $product->id,
                'attribute_id' => $value,
                'value' => $request->productAttribute[$value],
            ]);
        }
    }

    $product->ProductAttribute()->saveMany($productAttributes);

    $productDetail = [];

    foreach ($request->productDetails as $value) {
        if (isset($value)) {
            $productDetail[] = new ProductDetail([
                'description' => $value,
                'parent_id' => $product->id,
            ]);
        }
    }

    $product->ProductDetail()->saveMany($productDetail);

    $product_Maintenance = [];

    foreach ($request->productMaintenance as $value) {
        if (isset($value)) {
            $product_Maintenance[] = new ProductMaintenance([
                'description' => $value,
                'parent_id' => $product->id,
            ]);
        }
    }

    $product->ProductMaintenance()->saveMany($product_Maintenance);

    return back();
}

Solution 1:

There are some lines that are redundant (serve no purpose) depending on your fillable attributes.

If category_id is a fillable attribute in Product, then these lines don't do anything, because the category_id will already be set

    $category = Category::find($request->categoryId);

    $category->Product()->save($product);

This could be moved to a created/saved event I think.

    $code = create_code($request->categoryId, $product['id'], $request->colorId);

    $product->update(['code' => $code]);

If product_id is a fillable attribute in Price, then won't do anything because you're already setting the product_id a couple of lines before that.

    $product->price()->save($price);

Same deal with this

    $product->stock()->saveMany($stocks);
    $product->ProductAttribute()->saveMany($productAttributes);
    $product->ProductMaintenance()->saveMany($product_Maintenance);

Whenever you have something like

$results = []
foreach ($something as $item) {
    $result[] = ...;
}

It can be replaced by a map operation. Whether you decide that adds complexity or not is up to you.

If I had to rewrite it, I'd use collection methods to handle all the looping parts, inline the code in Product, and use create whenever possible. This means all models must have their $fillable or $guarded properties set.

class Product extends Model
{
    protected static function booted()
    {
        static::created(function ($product) {
            // you might need to inline the create_code function here
            $product->code = create_code($product->category_id, $product->id, $product->color_id); 
            $product->save();
        });
    }
}

public function store(AddProductRequest $request)
{
    $shopperId = 1;

    $product = Product::create([
        'title' => $request->title,
        'slug' => to_slug_fa($request->title, Product::class),
        'detail' => $request->detail,
        'color_id' => $request->colorId,
        'category_id' => $request->categoryId,
        'img' => $request->img,
        'description' => $request->description,
    ]);
    
    $product->price()->create(['price' => $request->price]);

    collect($request->sizeIds)
        ->filter(fn($value) => $request->quantity[$value] != null);
        ->map(fn($value) => [
            'stock' => $request->quantity[$value],
            'size_id' => $value,
            'shopper_id' => $shopperId,
        ])
        ->each(fn($value) => $product->stock()->create($value));

    collect($request->attributeIds)
        ->filter(fn($value) => isset($request->productAttribute[$value]))
        ->map(fn($value) => [
            'attribute_id' => $value,
            'value' => $request->productAttribute[$value],
        ])
        ->each(fn($value) => $product->ProductAttribute()->create($value));

    collect($request->productDetails)
        ->filter(fn($value) => isset($value)) // or just ->filter()
        ->map(fn($value) => [
            'description' => $value,
        ])
        ->each(fn($value) => $product->ProductDetail()->create($value));

    collect($request->productMaintenance)
        ->filter(fn($value) => isset($value)) // or just ->filter()
        ->map(fn($value) => [
            'description' => $value,
        ])
        ->each(fn($value) => $product->ProductMaintenance()->create($value));

    return back();
}

As an aside. I do not think this is a bloated method. You see exactly what is happening. You're saving a Product and its relationships. You can move logic around if you want, or even split it into different methods.

public function store(AddProductRequest $request)
{
    $shopperId = 1;

    $product = Product::create([
        'title' => $request->title,
        'slug' => to_slug_fa($request->title, Product::class),
        'detail' => $request->detail,
        'color_id' => $request->colorId,
        'category_id' => $request->categoryId,
        'img' => $request->img,
        'description' => $request->description,
    ]);
    
    $this->storePrice($product, $request);
    $this->storeStocks($product, $request, $shopperId);
    $this->storeProductAttributes($product, $request);
    $this->storeProductDetails($product, $request);
    $this->storeProductMaintenances($product, $request);

    return back();
}

private function storePrice(Product $product, $request)
{
    $product->price()->create(['price' => $request->price]);
}

private function storeStocks(Product $product, $request, $shopperId);
{
    collect($request->sizeIds)
        ->filter(fn($value) => $request->quantity[$value] != null);
        ->map(fn($value) => [
            'stock' => $request->quantity[$value],
            'size_id' => $value,
            'shopper_id' => $shopperId,
        ])
        ->each(fn($value) => $product->stock()->create($value));
}

private function storeProductAttributes(Product $product, $request);
{
    collect($request->attributeIds)
        ->filter(fn($value) => isset($request->productAttribute[$value]))
        ->map(fn($value) => [
            'attribute_id' => $value,
            'value' => $request->productAttribute[$value],
        ])
        ->each(fn($value) => $product->ProductAttribute()->create($value));
}

private function storeProductDetails(Product $product, $request);
{
    collect($request->productDetails)
        ->filter(fn($value) => isset($value)) // or just ->filter()
        ->map(fn($value) => [
            'description' => $value,
        ])
        ->each(fn($value) => $product->ProductDetail()->create($value));
}

private function storeProductMaintenances(Product $product, $request)
{
    collect($request->productMaintenance)
        ->filter(fn($value) => isset($value)) // or just ->filter()
        ->map(fn($value) => [
            'description' => $value,
        ])
        ->each(fn($value) => $product->ProductMaintenance()->create($value));
}

Or create a special class that will do everything™ like an App\Action\CreateProduct class

public function store(AddProductRequest $request)
{
    new CreateProduct($request->validated());

    return back();
}

But that won't make the logic itself any simpler.

Refactor Laravel controller

Solution 1:

Related

Recent Posts