5
\$\begingroup\$

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;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ I'd suggest separating html from php as much as possible. You can try returning array from paginationAdvanced() that will be converted to html somewhere else. This will make your code shorter and more readable, which in turn might inspire other users check and improve your code. \$\endgroup\$ – Al.G. Dec 25 '15 at 19:48
  • 1
    \$\begingroup\$ I can't wrap my head around what some numbers are doing in your if-statements, e.g. in $lastpage < 7 + ($adjacents * 2). If those are actually magic numbers (e.g. you decided at some point you wanted to have 7 pages displayed), consider replacing them with a variable. \$\endgroup\$ – Sumurai8 Dec 25 '15 at 19:54
7
\$\begingroup\$

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>";
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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