Be part of JetBrains PHPverse 2026 on June 9 – a free online event bringing PHP devs worldwide together.

khoanguyenme's avatar

Refactor Controller Code with switch block

I have a controller that will call this QuizHomePage@excute method for show the app homepage.

I gonna to add more tab to my app. Can any want help me to refactor this code?

<?php namespace Quiz\Services;

use Illuminate\Contracts\Auth\Guard;
use Quiz\lib\Repositories\Exam\ExamRepository;

class QuizHomePage {
    /**
     * @var Guard
     */
    private $auth;
    /**
     * @var ExamRepository
     */
    private $test;

    /**
     * @param Guard $auth
     * @param ExamRepository $test
     */
    public function __construct(Guard $auth, ExamRepository $test)
    {
        $this->auth = $auth;
        $this->test = $test;
    }
    public function execute($request)
    {
        switch($request->tab)
        {
            case 'done' :
                if (! $this->auth->check())
                    return redirect('quiz');
                $tests = $this->test->doneTest($this->auth->user());
                $name = 'Các đề bạn đã làm';
                break;
            case null :
                $tests = $this->test->orderBy('tests.created_at','DESC');
                $name = 'Quiz';
                break;
            default :
                return redirect('quiz');
        }

        $doneTestId = ($this->auth->check()) ? $this->test->doneTestId($this->auth->user()) : false;

        $key = $request->url().$request->page;

        $tests = \Cache::tags('tests','index')->remember($key, 10, function() use ($tests)
        {
            return $tests->has('question')->with('tagged','user')->paginate(20);
        });
        $tests->appends($request->only('tab'));

        return view('quiz.index',compact('tests','name','doneTestId'));
    }
} 
0 likes
6 replies
davorminchorov's avatar

Switch statements are considered to be code smells so try to avoid using them. The only place you should use a switch statement might be in a Factory class (using the Factory design pattern).

The switch statement can be refactored into a private method (or class if you are going add more tabs).

$auth->check() can go into it's own private method, and be called in the method where you check the request.

$doneTestId check can go to it's own private method.

$key can go to it's own private method;

caching can go to it's own private method.

$tests->appends(); can go to it's own private method;

The end result will look like this:


public function execute($request) { $this->checkRequest($request->tab); $doneTestId = $this->getDoneTestId(); $key = $this->getKey() $tests = $this->cacheTests($tests); $this->appendTests($tests, $requestTab); return View::make('quiz.index', compact('tests', 'name', 'doneTestId')); }

This is just a quick example, I didn't try it, I don't know if it works, so I might've missed something. (it is just an idea) Also, I don't have a lot of explanation what are you trying to do so I just guessed it.

You can even improve this if you move some of this logic in few classes and call some of the methods in some kind of service so the controller doesn't have to do anything that is not his job.

1 like
khoanguyenme's avatar

@Ruffles thanks for your help. Really this is a seperate class that controller will called when there is a incoming request hit homepage.

There will be tabs like new posts , feature posts, your posts ..... and they share the same view, variables, the only different is the $tests variables and $name (for title).

But your post point me out about Factory method. I will look in to it.

1 like
davorminchorov's avatar

Ok, what about calling a different method of a TabController when you request a tab? For example: when you call the feature posts, call the featurePosts() method etc?

1 like
khoanguyenme's avatar

@Ruffles this is my class so far. Any suggestion?

<?php namespace Quiz\Services;

use Illuminate\Contracts\Auth\Guard;
use Quiz\lib\Repositories\Exam\ExamRepository;
use Illuminate\Cache\Repository as Cache;

class QuizHomePage {
    /**
     * @var Guard
     */
    private $auth;
    /**
     * @var ExamRepository
     */
    private $test;

    private $doneTestId;

    private $request;

    private $result;

    private $name;
    /**
     * @var Cache
     */
    private $cache;

    /**
     * @param Guard $auth
     * @param ExamRepository $test
     * @param Cache $cache
     */
    public function __construct(Guard $auth, ExamRepository $test, Cache $cache)
    {
        $this->auth = $auth;
        $this->test = $test;
        $this->cache = $cache;
    }
    public function execute($request)
    {
        $key = $request->url().$request->tab.$request->page;

        $cache = $this->cache->tags('index','tests');

        if ($cache->has($key))
            return $cache->get($key);

        $this->request = $request;

        $tab = $this->switchMethod();

        $this->{$tab}();

        $this->setDoneTestId();

        $view = $this->makeView();

        $cache->put($key,$view,20);

        return $view;
    }

    private function doneTab()
    {
        if (! $this->auth->check())
            abort(503);
        $this->result = $this->test->doneTest($this->auth->user());

        $this->name = 'Các đề bạn đã làm';
    }

    private function latestTab()
    {
        $this->result = $this->test->orderBy('tests.created_at','DESC');
        $this->name = 'Quiz';
    }
    /**
     * @param mixed $doneTestId
     */
    public function setDoneTestId()
    {
        $this->doneTestId = ($this->auth->check()) ? $this->test->doneTestId($this->auth->user()) : false;
    }

    private function switchMethod()
    {
        $tab = $this->request->tab;

        if (!$tab)
            $tab = 'latest';
        if (!in_array($tab, ['done','latest']))
            abort(404);

        $tab .= 'Tab';

        return $tab;
    }

    private function makeView()
    {
        $tests = $this->result->has('question')->with('tagged','user')->paginate(10);;

        $tests->appends($this->request->only('tab'));

        $name  = $this->name;
        $doneTestId = $this->doneTestId;

        return view('quiz.index',compact('tests','name','doneTestId'))->render();
    }
} 
davorminchorov's avatar
Level 53

It looks ok for a small project, but now you can continue refactoring it with the Single Responsibility Principle in mind, moving things to their own classes so this one doesn't get way too long. (SRP - A class should do only one thing in other words, should only have one reason to change)

https://laracasts.com/lessons/responsibility one of the videos, check other videos on the site.

The Tab methods can be moved into their own Tab or TabRequest class (or TabController).

// alternative name can be PagesController
$this->route->get('tab/done', 'TabController@doneTab'); // or done
// same goes for the other tabs like home, latest your posts etc. 

Create a service class which will do some of these things (if it makes sense).

Currently, it seems like your homepage is doing too much. I see you got caching, eloquent queries (you already use Repositories, put that logic there and just call the methods here or well in the service class), auth logic (maybe these checks can go into the laravel 5 middleware thing - laravel 4 filters), some of those private properties up there are not really needed unless you want to inject them into the constructor so they can get some kind of value.

1 like
khoanguyenme's avatar

@Ruffles thanks for everything. I'm learning about Laravel and OOP so everthing are all new to me. This is a part of my app when migration from L4 to L5 (almost rewrite everything except the large database).

1 like

Please or to participate in this conversation.