Pagination - Suggestions to refine my code?

Hello again. Not so much a problem, I have some basic pagination code that I’ve tried to build in such a way that it can be implemented for the various $_GET query I am using in my CMS.

I’m sure there must be a way to refine it however as at the moment I’m having to do something similar to this:

if (isset($_GET['category']) && ctype_digit($_GET['category']))
	{
		
			$catID = mysql_real_escape_string($_GET['category']);
			
		$count = mysql_query("SELECT COUNT(*) FROM relationships WHERE category_id = $catID") or die(mysql_error());
		$total = mysql_fetch_row($count);
		$numrows = $total[0];
		
			$paginate = pagination($numrows);
				$offset = $paginate[0];
				$rowsperpage = $paginate[1];
				$totalpages = $paginate[2];
				$currentpage = $paginate[3];
				
				$result = mysql_query("SELECT categories.category_name 
     										, posts.post_id
     										, posts.post_title
     										, posts.post_content
     										, posts.date_created
											, posts.post_author
  										FROM categories 
											LEFT OUTER
  											JOIN relationships 
    											ON relationships.category_id = categories.category_id
											LEFT OUTER
  											JOIN posts 
    											ON posts.post_id = relationships.post_id
 											WHERE categories.category_id = $catID LIMIT $offset, $rowsperpage");
				
				while($row = mysql_fetch_array($result)) {
					
					$data = array('type' => 'category',
							  'id' => $row['post_id'],
							  'title' => $row['post_title'],
							  'content' => $row['post_content'],
							  'author' => $row['post_author'],
							  'date' => $row['date_created'],
							  'category' => $row['category_name']);
					displayPosts($data);
				}	
					$query = '&category='.$catID;
					displayPagination($totalpages,$currentpage,$query);
	}

Maybe it’s ok but to me that seems needlessly bulky. I have to return the 4 variables from the pagination function because they don’t transfer between pagination() and displayPagination().

This is my pagination script for working out total pages based on the current “skip” page.

function pagination($numrows) {
	// number of rows to show per page
		$rowsperpage = getSetting('posts_per_page');
	// find out total pages
		$totalpages = ceil($numrows / $rowsperpage);

	// get the current page or set a default
		if (isset($_GET['skip']) && ctype_digit($_GET['skip'])) {
  			$currentpage = (int) $_GET['skip'];									// cast var as int
		} 
		else {
   			$currentpage = 1;													// default page num
		} 

	// if current page is greater than total pages...
		if ($currentpage > $totalpages) {
   			$currentpage = $totalpages;											// set current page to last page
		}
		if ($currentpage < 1) {													// if current page is less than first page...
  			$currentpage = 1;													// set current page to first page
		}
							 
	$offset = ($currentpage - 1) * $rowsperpage;								// the offset of the list, based on current page
		$paginate = array ($offset, $rowsperpage, $totalpages,$currentpage);
	return $paginate;

}

And this is my script to display the necessary links after the posts for the current page have been printed. $query is to let me pass a query for the URL so it retains the particular category it is meant to be displaying.

function displayPagination($totalpages, $currentpage,$query='') {
		// range of num links to show
		$range = 3;
	if ($totalpages < 1) 
	{	
		return;
	}
	if ($totalpages > 1) 
	{
				
		if ($currentpage > 1) 
		{															// if not on page 1, don't show back links
   			echo " <a href='{$_SERVER['PHP_SELF']}?skip=1".$query."'><<</a> ";				// show << link to go back to page 1
   		 	$prevpage = $currentpage - 1;												// get previous page num
   			echo " <a href='{$_SERVER['PHP_SELF']}?skip=$prevpage".$query."'><</a> ";		// show < link to go back to 1 page
		} 

		for ($x = ($currentpage - $range); $x < (($currentpage + $range) + 1); $x++) 
		{	// loop to show links to range of pages around current page
   			if (($x > 0) && ($x <= $totalpages)) {										// if it's a valid page number...
      			if ($x == $currentpage) 
				{												// if we're on current page...
         			echo " [<b>$x</b>] ";												// 'highlight' it but don't make a link
      			} 
				else 
				{																		// if not current page...
         			echo " <a href='{$_SERVER['PHP_SELF']}?skip=$x".$query."'>$x</a> ";	// make it a link
      			} 
   			}  
		} 
                 
        
		if ($currentpage != $totalpages) {													// if not on last page, show forward and last page links
  			$nextpage = $currentpage + 1;													// get next page
    		echo " <a href='{$_SERVER['PHP_SELF']}?skip=$nextpage".$query."'>></a> ";			// echo forward link for next page 
  			echo " <a href='{$_SERVER['PHP_SELF']}?skip=$totalpages".$query."'>>></a> ";	 	// echo forward link for lastpage
		} 
	}
} 

Maybe that is as neat as it can get but while I’m trying to figure it out, I can’t find a way to make it more efficient, it seems cluttered to me and requires a lot of repeating code for each query such as category, author, date, etc.

Any advice is appreciated, thanks for reading.

Rule of thumb: If you have to define a variable twice with the same data, you’re probably doing it inefficiently.

        $catID = mysql_real_escape_string($_GET['category']);

You’ve already verified that category is ctype_digit; you dont really need to real_escape_string it too. (And yes, I know it’s going into a mySQL function - but you dont need to validate it twice, given that ctype_digit is a fairly strict validation).

  $count = mysql_query("SELECT COUNT(*) FROM relationships WHERE category_id = $catID") or die(mysql_error());

I’d have to get one of the mySQL gurus to back me up on this, but COUNT(*) i believe is slower than COUNT(afieldname). Just a streamlining thing.

Now we get to the meat of it - the functions.
First of all, I wouldnt declare pagination a function - maybe an INCLUDE, but not a function - why? Because if you INCLUDE it instead of function call it, you dont have to extract the variables again, they’re already in scope.

So turn this…


            $paginate = pagination($numrows);
                $offset = $paginate[0];
                $rowsperpage = $paginate[1];
                $totalpages = $paginate[2];
                $currentpage = $paginate[3];

into this…


include_once('paginate.php');

Take your paginate function, plop it into paginate.php (probably with the paginationDisplay function in there for cleanliness/reuse), drop the $paginate definition and the return line, strip off the function part. Let it run like a PHP page.
(There’s nothing harmful in there for someone to run anyway)

Then leave the display function call as is - it’ll be fed the variables you declared in your include.

Thanks StarLion, you’ve been a big help. Good point on the CType, I didn’t think, just did mysql_escape out of habit.

For COUNT I’m not sure it’d make a huge difference here, there are only two columns but I can see how it’d make sense elsewhere. Though I was under the impression it only counted rows not columns. I also thought that in MySQL, the number of rows was stored as an invisible header and it was just returning that. I’m sure that’s true of SOMETHING to do with SQL databases.

Is there anyway to retain pagination as a function rather than an include? I thought that was what GLOBAL was for but it doesn’t seem to work. It doesn’t have to not be an include, I’d just prefer to keep certain functions together in a particular function file if I can help it.

Not sure why you absolutely need to retain it as a function, but…
global is usually a last-resort because it can overwrite variables in the global scope. That said, it should work;


function pagination($numrows) {
 global $offset,$rowsperpage,$totalpages,$currentpage;
 //Rest of the function as normal, no return.
}

It doesn’t absolutely have to be, I’d just prefer it to be. Thanks for the help StarLion, I may yet change my mind and decide to just do an include, I will see how it goes.