4
\$\begingroup\$

I have custom code where I check request params for existing and make query to database with model

public function search(Request $request)
{
    $position = $request->get("position") ?? null;
    $location = $request->get("location") ?? null;
    $employment = $request->get("employment") ?? null;

    $jobsBy = null;
    $jobs = null;
    if($position) {
        $jobs = Job::where('title', 'LIKE', '%'.$position.'%');

        if($location) {
            $locations = [];
            if(is_array($location)) {
                foreach ($location as $name) {
                    $locations[] = $name;
                }
            } else $locations[] = $location;
            $jobs = $jobs->whereMetaIn("location", $locations);
        }

        if($employment) {
            $employments = [];
            if(is_array($employment)) {
                foreach ($employment as $name) {
                    $employments[] = $name;
                }
            } else $employments[] = $employment;
            $jobs = $jobs->withAnyTag($employments);
        }

        if($jobs->get()->count()) $jobsBy = "position";
    }
    if($location && $jobsBy === null) {
        $locations = [];
        if(is_array($location)) {
            foreach ($location as $name) {
                $locations[] = $name;
            }
        } else $locations[] = $location;
        $jobs = Job::whereMetaIn('location', $locations);

        if($employment) {
            $employments = [];
            if(is_array($employment)) {
                foreach ($employment as $name) {
                    $employments[] = $name;
                }
            } else $employments[] = $employment;
            $jobs = $jobs->withAnyTag($employments);
        }

        if($jobs->get()->count()) $jobsBy = "location";
    }
    if($employment && $jobsBy === null) {
        $employments = [];
        if(is_array($employment)) {
            foreach ($employment as $name) {
                $employments[] = $name;
            }
        } else $employments[] = $employment;
        $jobs = Job::withAnyTag($employments);

        if($jobs->get()->count()) $jobsBy = "employment";
    }

    if($jobs == null || !$jobs->get()->count()) {
        $jobs = Job::where("status", '<', 2);
        $jobsBy = "latest";
    }

    if(isset($jobs) && $jobs instanceof \Illuminate\Database\Eloquent\Builder && $jobsBy != null) {
        $IDS = $jobs->where('status', '<', 2)->pluck('id');

        $partnerCompanyJobs = Job::with(['user' => function($user) {
            $user->without('roles', 'companies')
                 ->select('id', 'name');
        }])->withCount('views')->where('type', 0)
                               ->whereIn('id', $IDS)
                               ->where('status', 1)
                               ->orderBy('updated_at', 'DESC')
                               ->get();

        $companyJobs = Job::with(['user' => function($user) {
            $user->without('roles', 'companies')
                 ->select('id', 'name');
        }])->withCount('views')->where('type', 1)
                               ->where('status', 1)
                               ->whereIn('id', $IDS)
                               ->orderBy('updated_at', 'DESC')
                               ->get();

        $sampleJobs = Job::with(['user' => function($user) {
            $user->without('roles', 'companies')
                 ->select('id', 'name');
        }])->withCount('views')->where('type', 2)
                               ->where('status', 1)
                               ->whereIn('id', $IDS)
                               ->orderBy('updated_at', 'DESC')
                               ->get();

        $somonJobs = Job::withCount('views')->whereIn('type', [3,4])
                               ->whereIn('id', $IDS)
                               ->where('status', 1)
                               ->orderBy('updated_at', 'DESC')
                               ->get();

        $jobs = $partnerCompanyJobs->merge($companyJobs)
                               ->merge($sampleJobs)
                               ->merge($somonJobs);

        if(count($jobs)) {
            $page = $request->page ?? 1;
            $items = $jobs->forPage($page, 5);
            return response()->json([          
                'data' => JobResource::collection($items),
                'total' => count($jobs),
                'jobsBy' => $jobsBy
            ]);
        }
    }

    return response()->json([          
        'data' => [],
        'total' => 0,
        'jobsBy' => ""
    ]);
}

How can I avoid duplication of code here and create an optimal database query in my case? I have a problem processing request parameters. Because of this, much is duplicated in my code. How can I fix such a case?

\$\endgroup\$
  • \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question? for examples, and revise the title accordingly. \$\endgroup\$ – Martin R Jan 6 at 6:12
4
\$\begingroup\$

I'd like to recommend condensing some of your script so that it is easier on the eyes.

This:

if($location) {
    $locations = [];
    if(is_array($location)) {
        foreach ($location as $name) {
            $locations[] = $name;
        }
    } else $locations[] = $location;
    $jobs = $jobs->whereMetaIn("location", $locations);
}

if($employment) {
    $employments = [];
    if(is_array($employment)) {
        foreach ($employment as $name) {
            $employments[] = $name;
        }
    } else $employments[] = $employment;
    $jobs = $jobs->withAnyTag($employments);
}

is more simply written as:

if ($location) {
    $jobs = $jobs->whereMetaIn("location", (array)$location);
}

if ($employment) {
    $jobs = $jobs->withAnyTag((array)$employment);
}

because casting the input as an array will convert a string into a single-element array. If the variable is already an array, then nothing changes.

But do you really want to overwrite the $jobs data formed from $location, if $employment is truthy? Either way that you answer that question, I think an elseif() is in order.

Please be precise with your conditionals. $jobs == null is more concisely written as !$jobs, but is not the same as $jobs === null. Likewise regarding $jobsBy != null.

Since $jobs is unconditionally declared, you can omit the isset() check.

Unless I misunderstand, it also feels like a good idea to execute and cache the following as a variable so that it can be used multiple times:

['user' => function($user) { $user->without('roles', 'companies')->select('id', 'name'); }]

You should also cache count($jobs) before or within your conditional, so that you are not making the same count() call twice.


Some more thoughts after initially posting my review:

  1. withAnyTag() will happily accept mixed data (string or array).
    https://github.com/rtconner/laravel-tagging/blob/laravel-5/docs/usage-examples.md

  2. When executing method calls on an object, just use the arrow syntax without =. In other words, you don't need to re-declare $jobs each time.

I don't have a Laravel project to play/test with (so there is no guarantee that this will work seamlessly, just consider it a collection of suggestions), but these are some of my thoughts on a script rewrite:

public function search(Request $request)
{
    $position = $request->get("position") ?? null;
    $location = $request->get("location") ?? null;
    $employment = $request->get("employment") ?? null;

    $jobsBy = null;
    $jobs = null;
    if ($position) {
        $jobs = Job::where('title', 'LIKE', '%' . $position . '%');
        $jobs->where('status', '<', 2);
        if ($location) {
             $jobs->whereMetaIn("location", (array)$location);
        }
        if ($employment) {
            $jobs->withAnyTag($employment);
        }
        if ($jobs->get()->count()) {
            $jobsBy = "position";
        }
    }

    // if no jobs by position, try with location...
    if ($location && !$jobsBy) {
        $jobs = Job::whereMetaIn('location', (array)$location);
        $jobs->where('status', '<', 2);
        if ($employment) {
             $jobs->withAnyTag($employment);
        }
        if ($jobs->get()->count()) {
            $jobsBy = "location";
        }
    }

    // if no jobs by location, try with employment...
    if ($employment && !$jobsBy) {
        $jobs = Job::withAnyTag($employment);
        $jobs->where('status', '<', 2);
        if ($jobs->get()->count()) {
            $jobsBy = "employment";
        }
    }

    // if not jobs, fallback to latest jobs...
    if (!$jobsBy) {
        $jobs = Job::where("status", '<', 2);
        $jobsBy = "latest";
    }

    if ($jobs !== null && $jobs instanceof \Illuminate\Database\Eloquent\Builder) {
        $ids = $jobs->pluck('id');

        $userFilter = ['user' => function($user) {
            $user->without('roles', 'companies')
                 ->select('id', 'name');
        }];

        $partnerCompanyJobs = Job::with($userFilter)
            ->withCount('views')
            ->where('type', 0)
            ->whereIn('id', $ids)
            ->where('status', 1)
            ->orderBy('updated_at', 'DESC')
            ->get();

        $companyJobs = Job::with($userFilter)
            ->withCount('views')->where('type', 1)
            ->where('status', 1)
            ->whereIn('id', $ids)
            ->orderBy('updated_at', 'DESC')
            ->get();

        $sampleJobs = Job::with($userFilter)
            ->withCount('views')->where('type', 2)
            ->where('status', 1)
            ->whereIn('id', $ids)
            ->orderBy('updated_at', 'DESC')
            ->get();

        $somonJobs = Job::withCount('views')
            ->whereIn('type', [3, 4])
            ->whereIn('id', $ids)
            ->where('status', 1)
            ->orderBy('updated_at', 'DESC')
            ->get();

        $jobs = $partnerCompanyJobs
            ->merge($companyJobs)
            ->merge($sampleJobs)
            ->merge($somonJobs);

        $jobCount = count($jobs);
        if ($jobCount) {
            $page = $request->page ?? 1;
            $items = $jobs->forPage($page, 5);
            return response()->json([
                'data' => JobResource::collection($items),
                'total' => $jobCount,
                'jobsBy' => $jobsBy
            ]);
        }
    }

    return response()->json([
        'data' => [],
        'total' => 0,
        'jobsBy' => ""
    ]);
}
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Idk why it was important to remove the keys, but just casting to Array no longer removes the keys. array_values could be used to retain the original behaviour. \$\endgroup\$ – slepic Jan 6 at 9:58
  • \$\begingroup\$ Why are the keys important? Do you see something to suggest that the array needs to be reindexed, or that it isn't already indexed? \$\endgroup\$ – mickmackusa Jan 6 at 12:26
  • \$\begingroup\$ I have no idea, those arrays are passed to methods I know nothing about. But OP got rid of the keys, so I was just pointing out that your solution is not equivalent. OP should know whether it matters in his case... \$\endgroup\$ – slepic Jan 6 at 16:54
  • \$\begingroup\$ I've change part of your code. Because it is not work for me correctly. You can see changed part of code here. Please update your answer then I'll accept it. @mickmackusa \$\endgroup\$ – Andreas Hunter Jan 8 at 6:26
  • \$\begingroup\$ Does that $userFilter declaration work as intended? \$\endgroup\$ – mickmackusa Jan 8 at 6:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.