Pagination PHP function - Can this code be made more efficient?

Question

First of all, I didn't write this code from scratch, I'm pretty sure I took it from an online tutorial and modified it slightly to suit my needs. Can the code be improved in any way? I feel it's quite long winded and repetitive for a relatively simple task.

function paginationAdvanced($select,$query,$per_page,$page,$url){   
    global $con; 
    $query = mysqli_query($con, "{$select} {$query}") or die(mysql_error()); 
    $total = mysqli_num_rows($query);
    $adjacents = "2"; 

    $prevlabel = "Prev";
    $nextlabel = "Next";
    $lastlabel = "Last";

    $page = ($page == 0 ? 1 : $page);
    $start = ($page - 1) * $per_page;                          

    $prev = $page - 1;                          
    $next = $page + 1;

    $lastpage = ceil($total/$per_page);

    $lpm1 = $lastpage - 1; // //last page minus 1

    $results_to = $start + $per_page;

    if ($results_to > $total) { $results_to = $total; }

    $pagination = "";
    if($lastpage > 1){   
        $pagination .= "<nav><ul class='pagination'>";
        $pagination .= "<li class='page-item'><a class='page-link active' href='#'>Displaying " . ($start+1) . " to " . $results_to . " of " . $total . " results</a></li>";
        //$pagination .= "<li><span class='page_info'>Page {$page} of {$lastpage}</span></li>";

            if ($page > 1) $pagination.= "<li><a class='page-link' href='{$url}page={$prev}' aria-label='Previous'><span aria-hidden='true'>&laquo;</span><span class='sr-only'>Previous</span></a></li>";

        if ($lastpage < 7 + ($adjacents * 2)){   
            for ($counter = 1; $counter <= $lastpage; $counter++){
                if ($counter == $page)
                    $pagination.= "<li class='page-item active'><a class='page-link'>{$counter}</a></li>";
                else
                    $pagination.= "<li class='page-item'><a class='page-link' href='{$url}page={$counter}'>{$counter}</a></li>";                    
            }

        } elseif($lastpage > 5 + ($adjacents * 2)){

            if($page < 1 + ($adjacents * 2)) {

                for ($counter = 1; $counter < 4 + ($adjacents * 2); $counter++){
                    if ($counter == $page)
                        $pagination.= "<li class='page-item active'><a class='page-link'>{$counter}</a></li>";
                    else
                        $pagination.= "<li class='page-item'><a class='page-link' href='{$url}page={$counter}'>{$counter}</a></li>";                    
                }
                $pagination.= "<li class='dot'>...</li>";
                $pagination.= "<li class='page-item'><a class='page-link' href='{$url}page={$lpm1}'>{$lpm1}</a></li>";
                $pagination.= "<li class='page-item'><a class='page-link' href='{$url}page={$lastpage}'>{$lastpage}</a></li>";  

            } elseif($lastpage - ($adjacents * 2) > $page && $page > ($adjacents * 2)) {

                $pagination.= "<li class='page-item'><a class='page-link' href='{$url}page=1'>1</a></li>";
                $pagination.= "<li class='page-item'><a class='page-link' href='{$url}page=2'>2</a></li>";
                $pagination.= "<li class='dot page-item'>...</li>";
                for ($counter = $page - $adjacents; $counter <= $page + $adjacents; $counter++) {
                    if ($counter == $page)
                        $pagination.= "<li class='page-item active'><a class='page-link'>{$counter}</a></li>";
                    else
                        $pagination.= "<li class='page-item'><a class='page-link' href='{$url}page={$counter}'>{$counter}</a></li>";                    
                }
                $pagination.= "<li class='dot page-item'>..</li>";
                $pagination.= "<li class='page-item'><a class='page-link' href='{$url}page={$lpm1}'>{$lpm1}</a></li>";
                $pagination.= "<li class='page-item'><a class='page-link' href='{$url}page={$lastpage}'>{$lastpage}</a></li>";      

            } else {

                $pagination.= "<li class='page-item'><a class='page-link' href='{$url}page=1'>1</a></li>";
                $pagination.= "<li class='page-item'><a class='page-link' href='{$url}page=2'>2</a></li>";
                $pagination.= "<li class='dot'>..</li>";
                for ($counter = $lastpage - (2 + ($adjacents * 2)); $counter <= $lastpage; $counter++) {
                    if ($counter == $page)
                        $pagination.= "<li class='page-item active'><a class='page-link'>{$counter}</a></li>";
                    else
                        $pagination.= "<li class='page-item'><a class='page-link' href='{$url}page={$counter}'>{$counter}</a></li>";                    
                }
            }
        }

            if ($page < $counter - 1) {
                $pagination.= "<li class='page-item'><a class='page-link' aria-label='Next' href='{$url}page={$next}'><span aria-hidden='true'>&raquo;</span><span class='sr-only'>Next</span></a></li>";
                $pagination.= "<li class='page-item'><a class='page-link' aria-label='Last' href='{$url}page=$lastpage'><span aria-hidden='true'>{$lastlabel}</span><span class='sr-only'>Last</span></a></li>";
            }

        $pagination.= "</ul></nav>";        
    }

    return $pagination;
}

Answer 1

Styling and readability

The styling of your code is inconsistent. You sometimes use spaces after a comma, you sometimes do not. You sometimes use a space between the curly brace and the thing it belongs to, you sometimes do not. You sometimes use whitespace around operators, and you sometimes do not. This makes the code harder to read, and I recommend fixing this.

Your indentation is inconsistent. This might lead to the programmer assuming a statement belongs to an other block it actually belongs to. I recommend fixing all indentation.

Your code has no comments, other than to comment out actual code and one obvious one: $lastpage - 1; // //last page minus 1. I would not have expected that code to do that. I recommend puting a comment above blocks of logic to tell what that block does, which makes it more intuitive what those few lines of code actually do.

You have a variable that wears two hats. $query is presumably first a string containing part of the SQL. Then you turn it into something that contains a mysqli_result or FALSE.

You have a variable that is initialised as a string ($adjecents), but is used as an integer everywhere. Just because it works does not mean it is good practice to do so.

Correctness

mysqli_query(..) can return a mysqli_result, the boolean FALSE if the query failed or the boolean TRUE if the query is not a SELECT, SHOW, DESCRIBE or EXPLAIN statement. You do not check for TRUE.

Security

You are using mysqli without using prepared statements. You expect $select and $query to be safe to use within a query. This might set you up for an SQL injection attack when someone (maybe you) uses this function assuming the function sanitizes the variable. Instead, learn how to use prepared statements and use those instead.

You are printing the result of mysql_error() to the page. This is fine in a development setting, but you do not want to do this on a production setup. The real question is probably if you even want your whole page to fail if the server is having a wobbly (technical term) when generating something trivial as pagination. Probably not.

Efficiency

The efficiency of your code seems fine if you want to create accurate pagination (that is: you want to actually show exactly how many results there are). Often you can get away by just displaying 1 to whatever your magic number is for the first page to not spend unnecessary time querying the database and generating pagination the user will likely not use anyway. If the user accidentally requests a page that is further than there are actually pages, you just display the last page. For page 2 till the last page you display the actual pagination, since the user is actually browsing through the pages and thus likely interested in that magic number.

The real problem of your code is that you use the same construct (a for-loop) over and over in different if-statements, while the things that code generates is only marginally different. You could instead identify which different situations you have, and when they can occur, and condense all this down to one for-loop.

//This code is only to show the general idea
if( $leftbound > 1 ) {
  //We do not display page 1, so display dots
}
for( $counter = $leftbound; $counter < $rightbound; $counter++ ) {
  //Print the pages
}
if( $rightbound < $maxpages ) {
  //We do not display the last page, so display dots
}

Similarly, you have two nearly identical <li> items to add to your pagination, the only difference being a class it seems, as shown below. This code duplication is not necessary. You can do one of two things. You can test if $counter == $page, and assign either "active" or "" to a variable, then use a generalized string. The other way you can work around this, is by having two for-loops, one from the left bound till $page - 1, and one from $page + 1 till the right bound. If $page - 1 < $leftbound, then the first loop will simply not run. Similarly, if $page + 1 > $rightbound, then the last loop will not run.

            if ($counter == $page)
                $pagination.= "<li class='page-item active'><a class='page-link'>{$counter}</a></li>";
            else
                $pagination.= "<li class='page-item'><a class='page-link' href='{$url}page={$counter}'>{$counter}</a></li>";