Search controller logic to search through models and manufacturers

Question

I have the following Laravel controller that searches through two models - Manufacturer and SearchTerm (which contains models of manufacturers, synonyms of models as well as common misspellings) using four different conditions: the first two for exact matches and the last two for partial matches.

I don't have a lot of experience with programming logic like this, and while it does thankfully work, it also seems to run very slow on my local server (2-3 seconds returning results from tables that are currently less than a hundred rows).

Controller

class SearchController extends Controller
{
    public function index(Manufacturer $manufacturer) {
        if (request("search")) {

        $exact_terms = SearchTerm::with("manufacturer")->where('term', request("search"))->get();
        if($exact_terms->isNotEmpty()) {
            echo "Exact search term match!";
            $results = $exact_terms;

            foreach ($results as $result) {
                $slug = $result->manufacturer->slug;
            }

            return redirect()->action([ManufacturerController::class, 'index'], ["manufacturer" => $slug]); 
        }

        $exact_make = Manufacturer::where('slug', request("search"))->get();
        if($exact_make->isNotEmpty()) {
            echo "Exact manufacturer match!";
            $results = $exact_make;

            foreach ($results as $result) {
                $slug = $result->slug;
            }
            
            return redirect()->action([ManufacturerController::class, 'index'], ["manufacturer" => $slug]); 
        }

        $search_terms = SearchTerm::with("manufacturer")->where('term', "like", '%' . request("search") . '%')->get()->unique("manufacturers");
        if($search_terms->isNotEmpty()) {
            echo "Search term match!";
            foreach ($search_terms as $search_term) {
                $search_term = $search_term->manufacturer;
            }
            $results = collect()->add($search_term); // Needed to always return a collection to the view, even if made up of one object
        } 

        $search_make = Manufacturer::where('slug', 'like', '%' . request("search") . '%')->get();
        if($search_make->isNotEmpty()) {
            echo "Manufacturer match!";
            $results = $search_make;
        }

    $results = isset($results) ? $results : collect();
    }
    
    return view("search", [
    "manufacturer" => $manufacturer,
    "results" => $results,
]);
}
}

View

@if(!$results->isEmpty())

<h1 class="my-5"> {{ $results->count() }} {{ $results->count() == 1 ? "result" : "results" }} found for <i>{{ request("search") }}</i></h1>

@foreach ($results as $result)
<img src="{{ asset("/images/badges/".$result->slug.".svg")}}" class="search-result-badge" alt="...">
<h2>{{$result->name}}</h2>

@endforeach    
@else
<p><h1>Sorry! We couldn't find any results for <i>"{{ request("search") }}"</i>.</h1></p>
<br>
@endif

Answer 1

Controller

For loops with discarded $results

This is weird:

foreach ($results as $result) {
    $slug = $result->slug; // or similar
}

You go through all that effort to get potentially a lot of matches, and then only use the last result. Why? If you only want one result, then you shouldn't get() a bunch of results. Only get the first(), perhaps after sorting appropriately. For example:

// First one:
$exact_terms = SearchTerm::with("manufacturer")->where('term', request("search"))->first();

// Highest ID:
$exact_terms = SearchTerm::with("manufacturer")->where('term', request("search"))->orderBy('id', 'DESC')->first();

I expect this is where the performance problem is.

Echos

What is the point of echo here? The only time that you would see it was while testing, so it should be removed after you've finished debugging those sections.

Sanitizing user input

The user can mess around with your like searches by entering a lot of %s in request("search") which may affect performance a little. I would strip these out, in addition to backslash and underscore.

(To clarify, you will need to keep the percents you are concatenating around request("search") as this is the only way to perform the query you want.)

Invert if

Your code has a bug, and this shows it. Where does $results come from when request("search") is missing? (Remember, frontend validation can be bypassed.)

if (!request("search")) {
    return view("search", ["manufacturer" => $manufacturer, "results" => $results, ]);
}
// Otherwise...

You'll have to have a return at the bottom but that's a small price to pay for not having so much indenting

Make the collection in a more readable way

collect()->add($search_term); becomes collect([$search_term]);, which is the same but more readable.

Null coalesce

$results = isset($results) ? $results : collect(); becomes $results ??= collect();. If you're not using PHP 7.4 to be able to do this, then update!

Return earlier with $search_terms

You throw away the results from a match with $search_terms when you have a match for $search_make. Not doing that would also allow you to streamline that part of the code.

View

Use whitespace for readability

Indent properly! The same goes for the controller.

Use spaces inside braces: {{ $content }}, not {{$content}}. Also use spaces around concatenation: $str . $str, not $str.$str.

Alt text

Don't use "..." as an alt text because that doesn't help a screen reader to know what's there. It seems appropriate here to leave it blank (to tell the screen reader to skip the image) as the image doesn't add anything to a screen reader beyond what they already can understand with $result->name on the next line.

Pluralization

In Laravel 9 you can use the helper str()->plural() (with the new, shorter syntax that makes it worth using in Blade). If you plan to localize, then you'll need to use trans_choice instead.

Invalid HTML

It's not valid to have a h1 inside a p.

Invert if

Specifically, !$results->isEmpty() can become $results->isNotEmpty() which is probably more readable.

My rewrite of your view

@if($results->isNotEmpty())
    <h1 class="my-5"> {{ $results->count() . ' ' . str('result')->plural($results->count()) }} found for <i>{{ request("search") }}</i></h1>
    @foreach ($results as $result)
        <img src="{{ asset("/images/badges/" . $result->slug . ".svg")}}" class="search-result-badge" alt="">
        <h2>{{ $result->name }}</h2>
    @endforeach
@else
    <h1>Sorry! We couldn't find any results for <i>"{{ request("search") }}"</i>.</h1>
    <br>
@endif

PS I'm not sure if asset("/images/badges/" . $result->slug . ".svg" is vulnerable to directory traversal. I hope your slug is not user-generated, or is otherwise heavily sanitized.