Laravel 5.8: prevent duplicated code

Question

Sometimes we have this code blocks that is repeated in the same controller again and again, I reviewed the Repository Pattern but I didn't understand how to deal with some kind of these duplication.

Here is a sample of what I got here:

$orders = ( new OrderList() )
            ->where( 'order_state_id', '!=', null )
            ->where( 'address_id', '!=', null )
            ->where( 'user_id', '!=', null )
            ->where( 'id', '>', $this->idLimitation )
            ->whereHas( 'orderDetail', function ( $query ) use ( $supplier_mega ) {
                //In case he is not the super user supplier check for conditions
                if ( Auth::guard( 'supplier' )->user()->super_user != 1 ) {
                    if ( Auth::guard( 'supplier' )->user()->role == 1 ) {
                        $query->where( 'supplier_id', Auth::guard( 'supplier' )->user()->id );
                    } elseif ( in_array( Auth::guard( 'supplier' )->user()->role, [ 2, 3, 4 ] ) ) {
                        $supplier_id = ( new Supplier() )->where( 'id', Auth::guard( 'supplier' )->user()->accountant_id )->first();
                        $query->where( 'supplier_id', $supplier_id->id );
                    }
                }

Now starting from ->whereHas( 'orderDetail', function ( $query ) use ( $supplier_mega ) { is repeated in this controller about 6 times but the rest of the function is not.

How can I deal with such duplication?

Another example here (upload multiple images):

if ( $request->hasFile( 'side_image' ) ) {
            foreach ( $request->side_image as $side_image ) {
                $input['campaign_ads_id'] = $campaignAdsId;
                $filename                 = Str::random( 10 ) . '.' . $side_image->getClientOriginalExtension();
                Image::make( $side_image )->save( 'images/campaignAds/' . $filename );
                $input['side_image'] = $filename;
                ( new CampaignAdsImage() )->create( $input );
            }
        }

This code is repeated in my code twice but of-course the some parts is different like the path where I store the images for instance.

Answer 1

I think you can DRY this up pretty well by refactoring.

This feels like a “ifs are evil” code smell to me. There are three “if” statements within that whereHas. They are packed in fairly tightly, and it makes that part of the code more difficult to read. Trying to refactor that as much as possible can resolve the duplication along the way, and ultimately make things much more readable and manageable.

• Try to give every “if” it’s own method, if for no other reason than the opportunity to give it a very concise name explaining what it does. This greatly improves code readability.

• The “if"s being repeated is a good indicator that you can flip the control around. Try to find a way to visit each “if” once and do all the things you need to do based on the results in one place. In this case, that means breaking the construction of the query into a couple of pieces.

• “if"s are decision points. They are apt to be the focus in future debugging efforts. Push these decision points to as early in the process as you can, and obsess over making that code as simple and as readable as possible.

• Auth::guard( 'supplier' )->user() gets resolved a number of times. Instead, resolve it once as early as possible and catch the results in a class attribute or parameter.

• The “if/elseif” is overreaching; much of the content is the same. Narrow the scope to just resolving the required supplier id, encapsulate it in a method, and call that within the required context. Furthermore, this looks to be a second important variable which gets used multiple times, so resolve this as early as possible as well and get it into an attribute or parameter.

• Inline role ids is a bad idea. It’s possible that the database will change, or a problem will occur on data recover or seeding on deploy to a new environment, leaving different role to id mapping. Even a short exposure to this problem will mean non-admins could be logged in as admins. Further, using the full name inline instead of ids helps improve code readability as it tells more of a story. “This happens for admins” instead of “this happens for some user role, I could look in the database to know which...” Using the name adds a bit of overhead, but it can be eager loaded with the user and makes the code a lot more readable.

• No need to load the accountant user record as we only need the id, which we have account_id anyway. Referencing that directly will save a query here, or more as this block is repeated.

• Database field names can be used to help improve readability, too. $user->role_id would help identify that it’s a foreign key rather than a string. Likewise, putting ‘is_’ or ‘has_’ on the front of booleans helps make them quickly obvious, such as $user->is_superuser. Along with that, setting $casts = [‘us_superuser’ => ‘boolean’,] ; in the models lets you compare it against true/false instead of 1 or 0, which is a lot more readable.

• whereNotNull(‘order_state_id’) is slightly more readable than where(‘order_state_id’, ‘!=’, null). This can be applied to a number of the where() clauses

• Refactor this query builder into a “plain old php” class. Call it a repository if you think it fits, but don’t get too hung up on the name. It could be as easy as a new app/Queries/OrderList.php folder and class. That new class can be injected into the controller. Make it fluent, and abstract decisions out of it. The class should do the lifting, but be dumb. The calling entity should encapsulate the decision making and directing, and only that.

• Replace eloquent with whereHas with query builder, joins, and where clauses. The eloquent and whereHas approach can certainly still work and be much cleaner than it currently is, but I find query builder to be much more readable and consistent across different models and needs when the queries start to get complicated. Perhaps this one is down to a personal preference.

With all of that in mind, here’s where I would land given my understanding of the posted code.

app/Queries/OrderList:

<?php

namespace App\Queries;

use App\Models\OrderList;
use Illuminate\Support\Collection;
use Illuminate\Support\Facades\DB;

class OrderLists
{
    protected $query;

    public function buildQuery(int $idLimitation): self
    {
        $this->query = DB::table('order_lists')
            ->select([
                'order_lists.id as id',
                //etc
            ])
            ->whereNotNull('order_state_id')
            ->whereNotNull('address_id')
            ->whereNotNull('user_id')
            ->where('id', '>', $idLimitation);

        return $this;
    }

    protected function applyNonSuperUserClauses(int $requiredSupplierId):self
    {
        $this->query->leftJoin('order_details', 'order_lists.id', '=', 'order_details.order_list_id');
        $this->query->where('order_details.supplier_id', '=', $requiredSupplierId);

        //Other  join and where clauses as appropriate

        return $this;
    }

    protected function fetchResults():Collection{

        return $this->query->get();
    }

controller:

 public function index(OrderList $orderListQuery)
    {
        $user = Auth::guard('supplier')->user();

        $idLimitation = $this->fetchIdLimitation();

        if ($this->isSuperUser($user)) {
            return view('your.view', [
                'orderList' => $orderListQuery
                    ->buildQuery($idLimitation)
                    ->fetchResults(),
            ]);
        }

       //More secured version is the default behavior.
        return view('your.view', [
            'orderList' => $orderListQuery
                ->buildQuery($idLimitation)
                ->applyNonSuperUserClauses($this->getRequiredSupplierIdByRole($user))
                ->fetchResults(),
        ]);

    }

 protected function isSuperUser($user): bool
    {
        return $user->is_super_user;
    }

 protected function getRequiredSupplierIdByRole($user): ?int
    {
        if ($this->hasAdminRole()) {
            return $user->id;
        }

        if ($this->hasLimitedUserRole()) {
            return $user->accountant_id;
        }
        return null;
    }

 //'Admin' and 'LimitedUser' based on assumptions about role ids 1,2,3,4.
    // Change these to be as concise as possible.
    protected function hasAdminRole($user): bool
    {
        return (in_array($user->role->name, $this->getAdminRoles()));
    }

    protected function getAdminRoles(): array
    {
        return [
            'Admin',
        ];
    }

    protected function hasLimitedUserRole($user): bool
    {
        return (in_array($user->role->name, $this->getLimitedRoles()));
    }

    protected function getLimitedRoles(): array
    {
        return [
            'Middle Manager',
            'Secretary',
            'Janitor',
        ];
    }

This turned into a long reply. Hopefully something here helps!