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.
Just finished up writing this for my current project, wondered if anyone would like to improve it and/or provide criticism. I feel it's something alot of people could make use of, and can be learnt from easily because it's not included in some huge pagination class. Working example shown here (you can grab the CSS from there too). Closely follows the way Digg handle their page links.
There are some minor bugs in it, and the logic isn't great - this is where you come in
One thing, not related to the logic, is the use of $_SERVER['PHP_SELF'], which is not safe to use (can contain user injected input, XSS). Luke blogged about it here http://www.mc2design.com/blog/php_self- ... ternatives
I really don't like the way it changes the number of available options. It's inconsistent, and that's a very bad thing in an interface. There should always be, for example, 7 paging links. Eg
@matthijs: Thanks for the heads-up on $_SERVER['PHP_SELF'] - I usually stay away from just # because it makes URL's look ugly, but from now on I'll be using basename(__FILE__).
@onion2k: Yeah, I'm aware of that - although I don't think anyone would notice it when it's actually in use. Besides, I said I was following the way Digg (and, f.e. phpBB forums) handle their page numbers, and as far as I can tell, they too have this same problem. I'll look into fixing that anyway.
@aborint: I'm not really looking to put it in a class; from my point of view, I often re-code things like this to give my brain something to do, and I think some procedural code logic to look at is good to get people started on writing their own. I don't really think it warrants a class (could be a part of a class to handle the whole pagination issue), because all it really is is one function.
jayshields wrote:@aborint: I'm not really looking to put it in a class; from my point of view, I often re-code things like this to give my brain something to do, and I think some procedural code logic to look at is good to get people started on writing their own. I don't really think it warrants a class (could be a part of a class to handle the whole pagination issue), because all it really is is one function.
Here is a very quick hack at turning this into a class. It centralizes some things, but there is still a lot I would want to improve. Ultimately you should be able to replace all of the HTML template with your own code and be able to set all the various options like CSS class names, number of items in the list, number at the beginning and end show, etc. I would also like it to sort out the number of pages based on the number of records and the records per page.
class Paginator
{
public $page = 0;
public $pagetotal = 0;
public $base;
public $prevLabel = '« Previous';
public $nextLabel = 'Next »';
public $disabledCssClass = 'disabled';
public $enabledCssClass = 'enabled';
function __construct($page, $pagetotal)
{
$this->page = isset($page) ? (int)$page : 1;
$this->pagetotal = $pagetotal;
$this->base = basename($_SERVER['SCRIPT_NAME']);
}
public function getLink($page, $label)
{
// bounds check
if ($page < 1) {
$page = 1;
}
if ($page > $this->pagetotal) {
$page = $this->pagetotal;
}
// show label only for current page, or link
if($page == $this->page) {
return "<li class=\"disabled\">$label</li>\n";
} else {
return '<li><a href="' . $this->base . '?page=' . $page . "\">$label</a></li>\n";
}
}
public function render()
{
$out = '';
//Show list
$out .= "<ul class=\"pagelinks\">\n";
//Show the previous page link
$out .= $this->getLink($this->page - 1, $this->prevLabel);
//If the amount of pages is 2 or more
if($this->pagetotal >= 2)
//Show the first 2 page links
for($x = 1; $x <= 2; $x++) {
$out .= $this->getLink($x, $x);
}
//If the page number is more than 8
if($this->page > 8 )
{
//Show a spacer
$out .= '<li class="disabled">...</li>';
//Loop through page numbers 5 before the current page, and to a maximum of 5 after
for($x = ($this->page - 5); $x <= ($this->pagetotal - 2) && $x <= ($this->page + 5); $x++) {
//Show the page link
$out .= $this->getLink($x, $x);
}
}
//If the total amount of pages is 3 or more
else if($this->pagetotal >= 3)
{
//Loop through page numbers starting from 3 and going to a maximum of 5 after the current page
for($x = 3; $x <= ($this->pagetotal - 2) && $x <= ($this->page + 5); $x++) {
//Show the page link
$out .= $this->getLink($x, $x);
}
}
//If the current page is more than 7 pages from the last page
if($this->page < ($this->pagetotal - 7))
{
//Show a spacer
$out .= '<li class="disabled">...</li>';
}
//A special case if the page total is 3 (only one ending link is needed instead of 2)
if($this->pagetotal == 3) {
//Show the page link
$out .= $this->getLink($this->pagetotal, $this->pagetotal);
//If it's anything else over 2
} elseif($this->pagetotal > 3) {
//Loop through the final 2 page numbers
for($x = ($this->pagetotal - 1); $x <= $this->pagetotal; $x++) {
//Show the page link
$out .= $this->getLink($x, $x);
}
}
//Show the next page link
$out .= $this->getLink($this->page + 1, $this->nextLabel);
$out .= "</ul>\n";
return $out;
}
}
Just BBcode telling me that I shouldn't have all those constants in the code! They should be properties so that you can make the list of links any size you want.
Anyway, turning my pagination code into a class was the best decision I ever made.
The next best decision would be to put my html generating code somewhere else.