5
\$\begingroup\$

I'm using the haversine formula to count distance from selected post-code to desired destination.

I have the following scope in App\Models\Business.php:

public function scopeDistance($query, $latitude, $longitude, $radius)
{
    $query->getQuery()->orders = [];
    return $query->select('*')
                 ->selectRaw("( 3959 * acos( cos( radians($latitude) ) * cos( radians( latitude ) )  * cos( radians( longitude ) - radians($longitude) ) + sin( radians($latitude) ) * sin(radians(latitude)) ) ) AS distance")
                 ->having('distance', '<=', $radius)
                 ->orderBy('distance');
}

Everything seems pretty ok, except the case when I use the paginate() method instead of get() in builder.

There are solutions out there, but I couldn't get these to work or were too complicated. I came up with this solution and I would like to hear feedback in terms of performance or memory usage.

This is the query at the end:

"select count(*) as count from (select ( 3959 * acos( cos( radians(51.5872718) ) * cos( radians( latitude ) )  * cos( radians( longitude ) - radians(-0.549759) ) + sin( radians(51.5872718) ) * sin(radians(latitude)) ) ) AS distance from `businesses` having `distance` <= ?) as harversine"

and custom_pagination function:

function custom_paginator($builder, $per_page)
{
    $path = current_route();
    $current_page = \Illuminate\Pagination\Paginator::resolveCurrentPage();

    if ( ! isset($builder->getQuery()->columns[1])) $count = $builder->count();
    else
    {
        $query = clone $builder->getQuery();
        $query->columns = [ $query->columns[1] ];
        $query->orders = null;
        $count = array_get(\DB::select("select count(*) as count from ({$query->toSql()}) as haversine", $query->getBindings()), 0)->count;
    }

    return new \Illuminate\Pagination\LengthAwarePaginator(
        $builder->forPage($current_page, $per_page)->get(),
        $count, $per_page, null, compact('path')
    );
}

Is there anything I should change or improve?

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

I think that working with a natural language style query builder has sent your implementation a little astray. I don't understand why you need to select a count of rows from a sub-queried result set when you could just count the rows returned from the query itself, without even pulling the result set down into the application.

Outside of that, I find your code hard to read.

  • Don't mix use of brackets on each side of if-else conditional
  • Consider using PSR-2 compliant styles
    • putting start of brackets for functions/methods, conditionals and looping constructs on same line.
    • Don't mix snake_case and camelCase within your user-written code.
    • One instruction per line.
  • Limit line lengths to ~80 characters.
\$\endgroup\$
4
  • \$\begingroup\$ No it's not working, i wouldn't create such a topic otherwise.... Haversine formula has dynamically generated variable in query called distance and it returns an error, when trying to paginate results with paginate() method. What do you mean by "when you could just count the rows returned from the query itself" - I have been trying everything to get count, especially from the first query, but i was getting the same error with missing distance var. Also things like pulling collection and do count($collection); $collection->forPage($x, $y) is just totally bad. \$\endgroup\$
    – rkd.me
    Mar 7, 2017 at 1:45
  • \$\begingroup\$ Also I won't use start of bracket on the same line, because it's less readable for me, from what i know programmers have free will of the way they write these. And the last, i do not mix any of the snake_case with camelCase - One is reserved for variables and the other one for methods. Lot's of developers go this way. \$\endgroup\$
    – rkd.me
    Mar 7, 2017 at 1:47
  • \$\begingroup\$ @rk-dev.net When you make any kind of SELECT query, you typically have the ability from database to determine the number of rows returned by the SELECT without having to buffer the results into the application. In PDO this would be similar to PDOStatement::rowCount() or in mysqli mysqli_result::$num_rows. Not that familiar with how Eloquent handles this. \$\endgroup\$
    – Mike Brant
    Mar 7, 2017 at 14:30
  • \$\begingroup\$ @rk-dev.net You are correct that you are free to define your own styles, however there has been lots of movement over the last several year in the professional PHP community to move towards more consistent style usage in PHP (thus PSR recommendations). This is just a code review and my comment was to simply consider PSR-2 styling conventions, which should help to make your code easier to read to others in the PHP community. \$\endgroup\$
    – Mike Brant
    Mar 7, 2017 at 14:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.