Best ways to refactor a controller

We are working on a project and the code got a little messy since we were in a rush when whe started developing it. Naming and everything is messy. More than that, a function has over 200 lines of code. What are the best ways to refactor a controller in Laravel?

Here is my ugly, messy code:

class SearchResultController extends Controller
{
    public function doctors(Request $request, ClinicDoctorSpecialty $entries, User $users)
    {
        $loc = Mapper::location('Bucharest');
        $clinics = $users->getByRole('clinic')->where('status_id', 12);
        $map_counter = 0;
        $subspecialties = [];
        $search_specialty = null;
        $search_range = env('DEFAULT_SEARCH_RADIUS');
        $search_max_price = 1000;

        $search = $entries->whereHas('clinic', function ($query) {
            $query->where('status_id', 12);
        })->orderBy('rating', 'desc');
        $offers = Offer::active();
        $search_text = "";

        // Keyword Search
        if (Request::has('term') && Request::get('term') != "") {
            $phrase = Request::get('term');
            $words = toKeywords($phrase);

            $search = $search->whereHas('doctor', function ($query) use ($phrase, $words) {
                $query->whereIn('status_id', [2, 3, 5, 18, 1, 7])->whereHas('profile', function ($q) use ($phrase, $words) {
                    $q->where('firstname', 'LIKE', $phrase . '%')->orWhere('lastname', 'LIKE', '%' . $phrase . '%');
                    if (count($words) > 1) {
                        foreach ($words as $word) {
                            $q->orWhere('firstname', 'LIKE', $word . '%')->orWhere('lastname', 'LIKE', $word . '%');
                        }
                    }
                });
            });
        }

        if (Request::has(trans('filters.range')) && is_numeric(Request::get(trans('filters.range')))) {
            $search_range = Request::get(trans('filters.range'));
        }

        // Specialty filter
        if (Request::has(trans('filters.specialty')) && $search_specialty = Specialty::where('slug', Request::get(trans('filters.specialty')))->first()) {
            $subspecialties = Specialty::where('parent_id', $search_specialty->id)->orderBy('name', 'asc')->get();
            $search_text = $search_specialty->name;
            $search = $search->whereHas('specialty', function ($query) use ($search_specialty) {
                $query->where('id', $search_specialty->id)->orWhere('parent_id', $search_specialty->id);
            });
            $offers = $offers->whereHas('specialties', function ($query) use ($search_specialty) {
                $query->where('id', $search_specialty->id);
            });
        }

        // Subspecialty filter
        if (Request::has(trans('filters.subspecialty')) && $subspecialty = Specialty::where('slug', Request::get(trans('filters.subspecialty')))->first()) {
            $search_text = $subspecialty->name;
            if (!Request::has(trans('filters.specialty'))) {
                $subspecialties = Specialty::where('parent_id', $subspecialty->parent->id)->orderBy('name', 'asc')->get();
            }
            $search = $search->whereHas('specialty', function ($query) use ($subspecialty) {
                $query->where('id', $subspecialty->id);
            });
        }

        // Insurer filter
        if (Request::has(trans('filters.insurer'))) {
            $search = $search->whereHas('clinic', function ($query) {
                $query->whereHas('insurers', function ($query) {
                    $query->where('slug', Request::get(trans('filters.insurer')));
                });
            });
        }

        // Region filter
        if (Request::has(trans('filters.region')) && Request::get(trans('filters.region')) != '-1') {
            $city = Request::get(trans('filters.region'));

            $loc = Mapper::location(Request::get(trans('filters.region')));

            $offers = $offers->whereHas('clinic', function ($query) use ($city) {
                $query->whereHas('profile', function ($q) use ($city) {
                    $q->where('city', $city);
                });
            })->inRandomOrder()->orderBy('created_at', 'desc');
        }

        // Recommended offers

        $offers_counter = $offers->count();
        if ($offers_counter == 0) {
            $offers = $offers->inRandomOrder()->orderBy('created_at', 'desc')->take(10);
            $offers_counter = count($offers);
        }

        // Fetch the collections
        $offers = $offers->take(10)->get();
        $specialties = Specialty::orderBy('filters_order')->get();
        $insurers = Insurer::orderBy('name')->get();

        // Instantiate the map
        Mapper::map($loc->getLatitude(), $loc->getLongitude(), ['zoom' => 11, 'fullscreenControl' => false, 'center' => true, 'marker' => false, 'cluster' => false, 'language' => 'ro']);

        $search = $search->groupBy('clinic_id', 'doctor_id')->get();

        $search = $search->filter(function ($entry) use ($loc, $search_range) {
            if ($entry->clinic->profile->lat && $entry->clinic->profile->lng) {
                if (inRange($loc->getLatitude(), $loc->getLongitude(), $entry->clinic->profile->lat, $entry->clinic->profile->lng, $search_range)) {
                    return true;
                }
            }
        });

        $clinics = $search->groupBy('clinic_id');
        /// Add the clinics to the map
        foreach ($clinics as $entry) {
            $clinic = $entry->first()->clinic;
            if ($clinic->profile) {
                if ($clinic->profile->lat && $clinic->profile->lng) {
                    Mapper::informationWindow($clinic->profile->lat, $clinic->profile->lng, (string)view('internalapi.maps.info', compact(['clinic'])), ['maxWidth'=> 300, 'title' => $clinic->profile->display_name, 'icon' => '/img/focus-marker.png', 'autoClose' => true, 'center' => true]);
                    $map_counter++;
                }
            }
        }

        // Rating filter
        if (Request::has(trans('filters.rating'))) {

            // Filter
            $max = Request::get(trans('filters.rating'));
            $min = $max - .9;
            $search = $search->filter(function ($post) use ($min, $max) {
                return $post->doctor->getDoctorRating($post->clinic->id)['total'] > $min && $post->doctor->getDoctorRating($post->clinic->id)['total'] < $max;
            });

            // Sort
            $search = $search->sortByDesc(function ($post) {
                return $post->doctor->getDoctorRating($post->clinic->id)['total'];
            });
        }

        // Price sorting DESC
        if (Request::has(trans('filters.price')) && Request::get(trans('filters.price')) === trans('filters.desc')) {
            $search = $search->sortByDesc(function ($post) {
                if ($post->doctor->clinics()->find($post->clinic->id) && $post->doctor->clinics()->find($post->clinic->id)->pivot->price_first != null) {
                    return $post->doctor->clinics()->find($post->clinic->id)->pivot->price_first;
                } else {
                    return 0;
                }
            });
        }

        // Price sorting ASC
        if (Request::has(trans('filters.price')) && Request::get(trans('filters.price')) === trans('filters.asc')) {
            $search = $search->sortBy(function ($post) {
                if ($post->doctor->clinics()->find($post->clinic->id) && $post->doctor->clinics()->find($post->clinic->id)->pivot->price_first != null) {
                    return $post->doctor->clinics()->find($post->clinic->id)->pivot->price_first;
                } else {
                    return 999999;
                }
            });
        }

        // Filter to return only the results that have doctors with the clinic attached in the clinic_doctor table
        $search = $search->filter(function ($result) {
            return $result->doctor->clinics()->find($result->clinic->id);
        });

        // Min price filter
        if (Request::has(trans('filters.minprice'))) {
            $search = $search->filter(function ($result, $key) {
                return $result->doctor->clinics()->find($result->clinic->id)->pivot->price >= Request::get(trans('filters.minprice'));
            });
        }

        // Max price filter
        if (Request::has(trans('filters.maxprice'))) {
            $search = $search->filter(function ($result, $key) {
                return $result->doctor->clinics()->find($result->clinic->id)->pivot->price <= Request::get(trans('filters.maxprice'));
            });
        }

        /// Find the highest price
        $resultsOrderedByPriceDesc = $search->sortByDesc(function ($result) {
            return $result->doctor->clinics()->find($result->clinic->id)->pivot->price;
        });

        if (count($resultsOrderedByPriceDesc) > 1) {
            $hightestPricedResult = $resultsOrderedByPriceDesc->first();
            $search_max_price = $hightestPricedResult->doctor->clinics()->find($hightestPricedResult->clinic->id)->pivot->price;
        }

        // Custom pagination
        $currentPage  = Request::get('page') ? (int) Request::get('page') : 1;
        $perPage = 15;
        $path = Request::path();
        $paginator = new Paginator($search);
        $paginator = $paginator->paginate($currentPage, $perPage, $path);
        $search_data = $search;

        $specialties = Specialty::orderBy('filters_order', 'ASC')->get();

        return view('main.search.doctors', compact([
                'search_data',
                'specialties',
                'insurers',
                'offers',
                'specialties',
                'offers_counter',
                'map_counter',
                'paginator',
                'search_specialty',
                'search_text',
                'search_max_price'
            ]));
    }
}

zachleigh

8 years ago

Level 47

First, I dont think I'd use localized request keys. Seems troublesome and pointless.

I would make filter classes for all the different filters and I would automate all the calls to those filters. Currently, you are doing if (Request::has(trans('key_name'))) all over the place. Would be much cleaner to simply loop through the request variables and then call the necessary filter class if it existed. Something like this: Controller:

$search = $entries->whereHas('clinic', function ($query) {
    $query->where('status_id', 12);
})->orderBy('rating', 'desc');

foreach ($request->all() as $variable) {
    $filterName = 'Full\\Namespace\\' . ucfirst($variable) . 'Filter';

    if (class_exists($filterName) {
        $filter = new $filtername();

        $filter->apply($search);
    }
}

Example filter class:

class PriceFilter
{
    public method apply($search)
    {
        if (Request::get('filters.price') === trans('filters.desc')) {
            $search = $search->sortByDesc(function ($post) {
            if ($post->doctor->clinics()->find($post->clinic->id) && $post->doctor->clinics()->find($post->clinic->id)->pivot->price_first != null) {
                return $post->doctor->clinics()->find($post->clinic->id)->pivot->price_first;
            } else {
                return 0;
            }
        });
    }
}

This is simplified and I obviously dont pass any variables to the filter class, but something like this would drastically reduce the amount of garbage in your controller.

1 like

sergiuwd

8 years ago

Level 3

That's right. It would. I was also thinking I could create some functions in the same controller, since it's only used for searching, and then call them like $search = $this->priceFilter(Request::get(trans('filters.price'), $search);

sergiuwd

8 years ago

Level 3

Since it's a platform that will be translated in many languages and the URL query parameters will not remain the same, I can't name the filter using the name of the param.

Please or to participate in this conversation.