christiangerdes's avatar

Can you make this scope cleaner

Can anyone help me clean up this code? It's a scope that filters results to only find products not on sale. I've written a test and added the actual code.

According to the test, the code works but is there a cleaner and better way to do this? I personally like to write clean code and this doesn't look clean to me.

I hope you're able to figure out what determines the sale state of the product from the test below.

function it_only_finds_products_not_on_sale()
{
    $product1 = factory(Product::class)->create(['regular_price' => 1500, 'sale_price' => 1000]);
    $product2 = factory(Product::class)->create(['regular_price' => 1500, 'sale_price' => 1000, 'on_sale_from' => now()->subDay(), 'on_sale_to' => now()->addWeek()]);
    $product3 = factory(Product::class)->create(['regular_price' => 1500, 'sale_price' => 1000, 'on_sale_from' => null, 'on_sale_to' => now()->addWeek()]);
    $product4 = factory(Product::class)->create(['regular_price' => 1500, 'sale_price' => 1000, 'on_sale_from' => now()->addDay(), 'on_sale_to' => null]);

    $collection = new EloquentCollection([
        $product5 = factory(Product::class)->create(['regular_price' => 1500, 'sale_price' => null]),
        $product6 = factory(Product::class)->create(['regular_price' => 1500, 'sale_price' => 2000]),
        $product7 = factory(Product::class)->create(['regular_price' => 1500, 'sale_price' => 2000, 'on_sale_from' => now()->subDay(), 'on_sale_to' => now()->addWeek()]),
        $product8 = factory(Product::class)->create(['regular_price' => 1500, 'sale_price' => 1000, 'on_sale_from' => now()->addDay(), 'on_sale_to' => now()->addWeek()]),
        $product9 = factory(Product::class)->create(['regular_price' => 1500, 'sale_price' => 1000, 'on_sale_from' => now()->subMonth(), 'on_sale_to' => now()->subDay()])
    ]);

    $productsNotOnSale = Product::notOnSale()->get();
    $productsNotOnSale->assertEquals($collection);
    $this->assertTrue($productsNotOnSale[0]->is($product5));
    $this->assertTrue($productsNotOnSale[1]->is($product6));
    $this->assertTrue($productsNotOnSale[2]->is($product7));
    $this->assertTrue($productsNotOnSale[3]->is($product8));
    $this->assertTrue($productsNotOnSale[4]->is($product9));
}
public function scopeNotOnSale($query)
{
    return $query->whereNull('sale_price')
                 ->orWhere(function($query) {
                    return $query->whereNotNull('sale_price')
                                 ->where(function($query) {
                                    return $query->whereRaw('products.regular_price < products.sale_price')
                                                 ->orWhere(function($query) {
                                                    return $query->where(function($query) {
                                                        return $query->where(function($query) {
                                                            return $query->whereNotNull('on_sale_from')
                                                                         ->whereNotNull('on_sale_to');
                                                        })->where('on_sale_from', '>', now());;
                                                    })->orWhere(function($query) {
                                                        return $query->where(function($query) {
                                                            return $query->whereNotNull('on_sale_from')
                                                                         ->whereNotNull('on_sale_to');
                                                        })->where('on_sale_to', '<', now());;
                                                    });
                                                 });
                                 });
                 });
}
0 likes
3 replies
adrian.nuernberger's avatar
Level 23

You can remove all the return's inside the closure.

This should also work

return $query->whereNull('sale_price')
             ->orWhereRaw('regular_price < sale_price')
             ->orWhere(function (Builder $query) {
                 $query->whereRaw('regular_price > sale_price')
                       ->whereNotNull('on_sale_from')
                       ->whereNotNull('on_sale_to')
                       ->where('on_sale_from', '>', now())
                       ->orWhere('on_sale_to', '<', now());
             });
Vilfago's avatar

I think you don't often want products without prices, so maybe a part of this scope could be transfered in a global scope

christiangerdes's avatar

@ADRIAN.NUERNBERGER - Well, that looks much better to me, but it doesn't work when I try to use these scopes in other scopes like so.

public function scopePriceEquals($query, $price)
{
    return $query->where('regular_price', $price)->notOnSale()
                 ->orWhere(function($query) use ($price) {
                    $query->where('sale_price', $price)->onSale();
                 });
}

Please or to participate in this conversation.