Simple pagination script

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
MichaelR
Forum Contributor
Posts: 148
Joined: Sat Jan 03, 2009 3:27 pm

Simple pagination script

Post by MichaelR »

Okay, so I've written my own pagination script because I feel the ones that can be found on the internet are too complex. I much prefer to keep things as simple and concise as possible. However, I'm aware that when things are simplified, such things can lose some of their functionality. That's why I'd like you to critique this code and let me know if anything is missing and/or problematic.

Code: Select all

 
 if (isset($_POST["location"]))
  $location = mysql_real_escape_string($_POST["location"]);
 
 elseif (isset($_GET["location"]))
  $location = mysql_real_escape_string($_GET["location"]);
 
 $total = mysql_num_rows(mysql_query("SELECT `id` FROM `users` WHERE `location` = '".$location."'"));
 
 $limit = 10;
 
 $pages = ($total >= 1) ? ceil($total / $limit) : 1;
 
 $page  = ($_GET["page"] >= 1 && $_GET["page"] <= $pages) ? $_GET["page"] : 1;
 
 $lower = $page * $limit - $limit;
 
 $query = mysql_query("SELECT `username` FROM `users` WHERE `location` = '".$location."' LIMIT ".$lower.", ".$limit);
 
 while ($result = mysql_fetch_array($query))
  echo $result["username"]."<br />";
 
 for ($i = 1; $i <= $pages; $i++) {
 
  if ($i == $page)
   echo $i;
 
  elseif ($i <= 3 || $i <= 7 && $page <= 5 || $i == $page - 1 || $i == $page + 1 || $i > $pages - 7 && $page > $pages - 5 || $i > $pages - 3 || $pages <= 11)
   echo "<a href=\"".$link.".php&page=".$page."&location=".$location."\">".$i."</a>";
 
  elseif ($i == 4 || $i == $pages - 3) 
   echo "&hellip;";
 
 }
 
User avatar
califdon
Jack of Zircons
Posts: 4484
Joined: Thu Nov 09, 2006 8:30 pm
Location: California, USA

Re: Simple pagination script

Post by califdon »

The first part looks correct, to me. I don't understand what you're trying to do in that for loop at the bottom. I would recommend that you validate the $_GET['page'] value, though, before using it.
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Re: Simple pagination script

Post by John Cartwright »

Code: Select all

$total = mysql_num_rows(mysql_query("SELECT `id` FROM `users` WHERE `location` = '".$location."'"));
You are unnecesarily querying your entire dataset. If all you need the total number of rows, use COUNT(id) instead.

Code: Select all

$result = mysql_query("SELECT COUNT(`id`) AS `total` FROM `users` WHERE `location` = '".$location."'");
$row = mysql_fetch_assoc($result);
$total = $row['total'];
MichaelR
Forum Contributor
Posts: 148
Joined: Sat Jan 03, 2009 3:27 pm

Re: Simple pagination script

Post by MichaelR »

califdon wrote:The first part looks correct, to me. I don't understand what you're trying to do in that for loop at the bottom. I would recommend that you validate the $_GET['page'] value, though, before using it.
I'll give you a few examples of the for loop in action.

Code: Select all

 
 ($pages == 11)
 
 1 2 3 4 5 6 7 8 9 10 11
 
 ($page <= 5 && $pages == 13)
 
 1 2 3 4 5 6 7 ... 11 12 13
 
 ($page > $pages - 5 && $pages == 13)
 
 1 2 3 ... 7 8 9 10 11 12 13
 
 $page == 7 && pages == 13)
 
 1 2 3 ... 6 7 8 ... 11 12 13
 
And I didn't think I'd have to worry about validating the $_GET["page"] variable. Only if $_GET["page"] is greater than or equal to 1, and less than or equal to $pages, will it be used; else the $page equals 1. Anything other than a valid numeric would fail the if statement, and so the page defaults to 1.
Jcart wrote:You are unnecesarily querying your entire dataset. If all you need the total number of rows, use COUNT(id) instead.
Thank you. And does it matter if I use mysql_fetch_array over mysql_fetch_assoc? Or should I use the latter?
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Re: Simple pagination script

Post by VladSun »

Jcart wrote:You are unnecessarily querying your entire dataset. If all you need the total number of rows, use COUNT(id) instead.

Code: Select all

$result = mysql_query("SELECT COUNT(`id`) AS `total` FROM `users` WHERE `location` = '".$location."'");
$row = mysql_fetch_assoc($result);
$total = $row['total'];
There is some ready-to-use functionality in MySQL for solving this problem:
[sql]SELECT SQL_CALC_FOUND_ROWS id, username, email FROM users WHERE username < 'B' LIMIT 10;SELECT FOUND_ROWS();[/sql]

No coma after SQL_CALC_FOUND_ROWS .
Last edited by VladSun on Sun Jan 04, 2009 8:52 am, edited 1 time in total.
There are 10 types of people in this world, those who understand binary and those who don't
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Re: Simple pagination script

Post by VladSun »

MichaelR wrote:And I didn't think I'd have to worry about validating the $_GET["page"] variable. Only if $_GET["page"] is greater than or equal to 1, and less than or equal to $pages, will it be used; else the $page equals 1. Anything other than a valid numeric would fail the if statement, and so the page defaults to 1.

Code: Select all

$_GET['page'] = "2;<script>alert('XSS')</script>"
You don't have an echo $page in your current code, but it's not for sure you will not use it somewhere else. So - always validate, sanitize and filter the user input.
MichaelR wrote:Thank you. And does it matter if I use mysql_fetch_array over mysql_fetch_assoc? Or should I use the latter?
mysql_fetch_array will return a two times longer array than mysql_fetch_assoc - it will contain both associative array and a zero based indexed array. So, I'd use mysql_fetch_assoc, or mysql_fetch_object, or mysql_fetch_row, but not mysql_fetch_array.
There are 10 types of people in this world, those who understand binary and those who don't
MichaelR
Forum Contributor
Posts: 148
Joined: Sat Jan 03, 2009 3:27 pm

Re: Simple pagination script

Post by MichaelR »

VladSun wrote:You don't have an echo $page in your current code, but it's not for sure you will not use it somewhere else. So - always validate, sanitize and filter the user input.
Thank you, I will remember to do so. Better safe than sorry, I guess.
mysql_fetch_array will return a two times longer array than mysql_fetch_assoc - it will contain both associative array and a zero based indexed array. So, I'd use mysql_fetch_assoc, or mysql_fetch_object, or mysql_fetch_row, but not mysql_fetch_array.
Would that be a blanket rule? Or just for something like this pagination script? So, say I wished to select a user's data:

Code: Select all

 
$sql = "SELECT * FROM `users` WHERE `id` = ".$_SESSION["id"]." LIMIT 1";
 
$resultone = mysql_fetch_array(mysql_query($sql));
$resulttwo = mysql_fetch_assoc(mysql_query($sql));
 
Would assoc be of better use here, as well?
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Re: Simple pagination script

Post by VladSun »

MichaelR wrote:

Code: Select all

 
$sql = "SELECT * FROM `users` WHERE `id` = ".$_SESSION["id"]." LIMIT 1";
 
$resultone = mysql_fetch_array(mysql_query($sql));
$resulttwo = mysql_fetch_assoc(mysql_query($sql));
 
Would assoc be of better use here, as well?
Add

Code: Select all

print_r($resultone);
print_r($resulttwo);
and you will see what's the difference.
:)
There are 10 types of people in this world, those who understand binary and those who don't
MichaelR
Forum Contributor
Posts: 148
Joined: Sat Jan 03, 2009 3:27 pm

Re: Simple pagination script

Post by MichaelR »

VladSun wrote:

Code: Select all

 
print_r($resultone);
print_r($resulttwo);
and you will see what's the difference.
Ah, yes. I see. I'll never use something like $result[0] in these situations, so it seems superfluous to use fetch_array. Thanks again.
Post Reply