Optimization of a Laravel controller that passes data and views

Question

I am trying to build a website that shows events. And I use the following controller. Please note that the urls ($view and $course) etc. are renamed and are not the ones used on the real website!

Controller:

<?php

namespace App\Http\Controllers;

use Carbon\Carbon;
use Illuminate\Http\Request;
use Illuminate\Support\Facades\DB;

use App\Models\Eventary;


class EventaryEventsController extends Controller
{

    // function for the /kursangebote/{$course} pages
    public function showEvents($course)
    {
        // filter the courses and set the views and id
        if ($course == 'workshops') {
            $view   = 'pages.course.view-1';
            $id     = '1';
        }

        if ($course == 'events') {
            $view   = 'pages.course.view-2';
            $id     = '2';
        }

        if ($course == 'salsa') {
            $view   = 'pages.course.view-3';
            $id     = '3';
        }

        if ($course == 'dance4') {
            $view   = 'pages.course.view-4';
            $id     = '4';
        }

        if ($course == 'dance5') {
            $view   = 'pages.course.view-5';
            $id     = '5';
        }

        if ($course == 'dance6') {
            $view   = 'pages.course.view-6';
            $id     = '6';
        }

        if ($course == 'dance7') {
            $view   = 'pages.course.view-7';
            $id     = '7';
        }

        if ($course == 'dance8') {
            $view   = 'pages.course.view-8';
            $id     = '8';
        }

        // get the course data from the database
        $events = Eventary::query()
            ->orderBy('title')
            ->orderBy('start')
            ->where('category', $id)
            ->where('start', '>', Carbon::now())
            ->get();

        // pass through the data to the correct views
        return view($view, [
            "events" => $events
        ]);

    }


    // function for the /events page
    public function workshopsList()
    {
        // get the course data from the database
        $events = Eventary::query()
            // show all events with the category 1 (events & workshops)
            ->where('category', '1')
            ->where('start', '>', Carbon::now())
            ->get();


        // pass through the data to the correct views
        return view('pages.course.event-list', [
            "events" => $events
        ]);

    }


    // function for the /kalender page
    public function calendarList()
    {
        // get the course data from the database
        $events = Eventary::query()
            // show all events with the category 1 (events & workshops)
            ->where('start', '>', Carbon::now())
            ->get();


        // pass through the data to the correct views
        return view('pages.calendar', [
            "events" => $events
        ]);

    }


    // function for fullcalendar json generation
    public function feed()
    {
        // get database
        $events = DB::table('eventaries')->get();

        return json_encode($events, JSON_PRETTY_PRINT);

    }



}

there is a lot of code that gets reused. So I think that this is something that I should improve. But are there any other suggestions?

Answer 1

Put your repeated or complicated queries into scopes on the model class. Here I also use the helper now to avoid importing Carbon:

public function scopeFuture($query)
{
    $query->where('start', '>', now());
}

You can even use this scope in other scopes. If you find yourself using the same scope on most to all queries, you could use a global scope, but I'm not a fan since they're usually unexpected and confusing.

---

What does your route look like for /kursangebote{$course}? If you're not constraining what $course can be, then this code will error out when someone inevitably tries to visit /kursangebotedance (no number). If you do have constraints on the route, I'd suggest not and instead making your controller responsible for returning a sensible response, e.g. if you can't match it to a category, abort(404).

Also it looks like you're expecting some partially uppercase URLs which is a confusing user experience. Everything should be lowercase, with words separated by hyphens (unless they're separated with a slash), as that's better for SEO.

---

What does the first "show all events with the category 1 (events & workshops)" mean? Aren't "Events" category 2? What does the second instance of this comment mean? You're not filtering on category there.

---

Try to avoid the magic of arbitrary numbers like '1' if you can. If you can't, consider using enums (PHP 8.1) to use a clear name instead.

---

The name "pages.course.view-N" is suboptimal. You don't need the word "view" because you know it's a view: it's a .blade.php file, and it's located in your "views" directory. What you don't know from looking at the file name is what type of dance it's for.

Style guides recommend using snake_case for view files, not hyphens.

If your "views" directory only contains a "pages" directory, then I would remove the "pages" directory. Even if it doesn't, it sounds like it's a catchall and not very descriptive.

---

The chain of if statements seems to me like it's indicating a deeper problem, but it's hard to give a suggestion without seeing more code. While you can use some other language construct here (such as match), I think you need a bigger rewrite.

What's the purpose of having so many different views ("course.view-N")? Is there a way that you can rewrite your view code to not need so many different views?

And why not store categories in the database? Right now, if you wanted to add more categories, you have to write more code. A database would allow you to expand and have an admin interface to add more categories as needed — or remove some.

Answer 2

I don't use Laravel, but yes, you can greatly reduce the preparatory PHP scripting and make it more "D.R.Y.". You could use a lookup array and then null coalesce to fallback values, use a switch() block, but the most modern technique (if you are running PHP8), is to use a match() block -- in my opinion, the beautiful fusion of a lookup and a switch block. (Basic demo)

public function showEvents(string $course)
{
    [$view, $id] = match($course) {
        'Workshops' => ['pages.course.view-1', '1'],
        'Events' => ['pages.course.view-2', '2'],
        'Salsa' => ['pages.course.view-3', '3'],
        'dance4' => ['pages.course.view-4', '4'],
        'dance5' => ['pages.course.view-5', '5'],
        'dance6' => ['pages.course.view-6', '6'],
        'dance7' => ['pages.course.view-7', '7'],
        'dance8' => ['pages.course.view-8', '8'],
        default => ['pages.course.view-1', '1'],
    };

    return view($view, [
        "events" => Eventary::query()
                ->orderBy('title')
                ->orderBy('start')
                ->where('category', $id)
                ->where('start', '>', Carbon::now())
                ->get();
    ]);
}

As shown above, I prefer to avoid single-use variables in my code unless they add value to comprehension/readability/line-brevity. Applying that stance to your other methods...

public function calendarList()
{
    return view('pages.calendar', [
        "events" => Eventary::query()
            ->where('start', '>', Carbon::now())
            ->get()
    ]);
}

public function feed()
{
    return json_encode(DB::table('eventaries')->get(), JSON_PRETTY_PRINT);
}