Refactor

What would be the best way to refactor this code?

class CoursesController extends Controller { public function index(Request $request) {

    //http://localhost:8000/courses?search=logis&lat=53.559003&lng=-2.077371&radius=5

    $radius = ($request->has('radius') ? $request->get('radius') : 10);
    $lat    = ($request->has('lat') ? $request->get('lat') : 53.540930);
    $lng    = ($request->has('lng') ? $request->get('lng') : -2.111366);

    $query = \DB::table('courses')
        ->join('locations', 'courses.id', '=', 'locations.course_id')
        ->join('providers', 'courses.provider_id', '=', 'providers.id')
        ->select('courses.id', 'title', 'courses.description', 'locations.name', 'locations.address', 'providers.company_name', 'providers.company_name', 'providers.logo', \DB::raw('3959 * acos( cos( radians('.$lat.') ) * cos( radians( lat ) ) * cos( radians( lng ) - radians  ('.$lng.') ) + sin( radians('.$lat.') ) * sin( radians( lat ) ) ) as distance') );

    if($request->has('search'))
        $query->where('courses.title', 'LIKE', '%' . $request->get('search') . '%');

    if($request->has('difficulty_level'))
        $query->where('courses.difficulty_level', '=', $request->get('difficulty_level'));

    if($request->has('parking'))
        $query->where('locations.parking', '=', 1);

    if($request->has('accessibility'))
        $query->where('locations.accessibility', '=', 1);

    if($request->has('public_transport'))
        $query->where('locations.public_transport', '=', 1);

    $courses = $query->having('distance', '<', $radius)
    ->orderBy('distance', 'asc')
    ->get();

    $categories = Category::orderBy('category')->lists('category', 'id');

    return view('frontend.courses.index', compact('courses', 'categories'));
}

davorminchorov

10 years ago

Level 53

Move out the query logic out of the controller and put it inside your model(s) or create a different class that will handle your read / search operations .
Create some query scopes
I see a lot of if statements which duplicate but they have only a few different things. These can be put inside one method inside a model class which by passing stuff as parameters, the query where methods will be shortened.
The lines where you get the parameters from the search query string can be cut down by replacing these 3 lines with a helper function which checks for request input.

// searchBy() helper or figure out a better name for it.
public function searchBy($value, $query, $field,  $type) {
    if($value)
    {
        return $query->where($field, $type,  $value);
    }
}

// hasSearchParameter 
public function hasSearchParameter($value, $default) 
{
    if($value) 
    {
        return $value;
    }

    return $default;
}

danny620

10 years ago

Level 7

Sorry to be a pain I'm new to laravel can you give me an example?

JeffreyWay

10 years ago

Best Answer

Level 59

I'll do a lesson on querying like this soon. Maybe today, if I have time.

Lots of ways to tackle it. If it's basic stuff, I would keep it in the controller. Otherwise, I'd make a CourseFilters class, and then defer to it:

public function index(CourseFilters $filters) {
    return Course::filter($filters)->get();
}

3 likes

ProfessorGT

9 years ago

Level 7

@JeffreyWay How would you go about saving a search using your method. Would you just store the entire URL in the DB? or just store the filters? I would be really interested to know your approach for saved searches. :) Thanks!

Please or to participate in this conversation.