Comments on my listing/pagination class?

[kevinc]

Hi, some comments will be cool :)


<?PHP

/**
Here's a generic table layout thing. -- You can populate it yourself!
CREATE TABLE products (
id int(6) NOT NULL auto_increment,
Product_Name varchar(255) default '0',
Product_Description text,
Product_Price varchar(7) default '0',
Product_Postage varchar(7) default '0',
Product_Thumbnail varchar(200) default '0',
Product_InStock varchar(7) default '0',
PRIMARY KEY (id)
) TYPE=MyISAM;

* class Products
*
* This class is used for listing and paging purposes. It's capable of handling many records as well as singular.
* It can be used for anything, as you specify which field values to use.
* It returns the values you specify, so you can use them in your PHP scripts.
*
*
* Please see the attached LICENSE and COPYING files for copyright information.
*
* Kevin Cadman - 2003.
*
* Last Updated - 31/07/2003
*/
class Products {

/**
* Products::$_DB
*
* Private: Database Object Variable
*
*/
var $_DB = FALSE;
/**
* Products::$_Results_Per_Page
*
* Private: How many results to show on one page.
* This must be greater than 2. If this is FALSE all results will be shown.
* If this is FALSE, do not use the Pagination() function.
*/
var $_Results_Per_Page = 6;
/**
* Products::$_Products_Table
*
* Private: The name of the table where the products are kept.
*
*/
var $_Products_Table = "products";
/**
* Products::$_IDField
*
* Private: ID Field in the $_Products_Table.
* This is the Primary Key field which is essential. This is used to uniquely reference each record.
*
*/
var $_IDField = "id";
/**
* Products::$ProductFields
*
* Public: The list of fields which you want returned when querying the products table.
* These must be initialized to false.
*
*/
var $ProductFields = array(
"id" =>FALSE,
"Product_Name" =>FALSE,
"Product_Description" =>FALSE,
"Product_Price" =>FALSE,
"Product_Thumbnail" =>FALSE,
"Product_InStock" =>FALSE,
"Product_Postage" =>FALSE,
"Product_PicLink" =>FALSE
);
/**
* Products::$_UseStock
*
* Private: Specifies whether or not we're going to use the STOCK system.
* This will only return items that are "In Stock" (where the $StockField field is > 0.)
*
*/
var $_UseStock = FALSE;
/**
* Products::$StockField
*
* Private: The field name of the "In Stock" field.
*
*/
var $StockField = "Product_InStock";
/**
* Products::$_OrderByField
*
* Private: The field we are going to order all the results return by.
*
*/
var $_OrderByField = "Product_Name";
/**
* Products::$_OrderHow
*
* Private: The method in which we are going to order the results returned.
* Valid options are asc and desc.
*
*/
var $_OrderHow = "asc";
/**
* Products::$_LimitPosition
*
* Private: Used to determine which page we are currently on.
*
*/
var $_LimitPosition = 0;
/**
* Products::$Affected_Rows
*
* Public: Used to store the amount of total records returned.
* Used in conjunction with $_LimitPosition to determine which page we are currently on.
*
*/
var $Affected_Rows = 0;
/**
* Products::$_Pagination_Seperator
*
* Private: The seperator character to seperate either the numbers(advanced) for the pagination, or the words(simple).
*
*/
var $_Pagination_Seperator = " ";
/**
* Products::$_Pagination_Selected_Style
*
* Private: The style used to show the "selected" page.
* B = Bold, I = Italic, U = Underline (You may use any corresponding HTML.
*
*/
var $_Pagination_Selected_Style = "B";

/**
* Products::Products()
*
* Constructor Function. Initializes the $_DB variable, and the $_LimitPosition variable.
* If the Session is set for LimitPage, use it, otherwise, set it to 0. (Which it already is.)
*
*/
function Products(&$DB,$TableName="") {
$this->_DB = &$DB;
if($TableName != "") {
$this->_Products_Table = $TableName;
}
$this->_LimitPosition = (isset($_SESSION['LimitPage']) ? $_SESSION['LimitPage'] : 0);
}

/**
* Products::Product_Listing()
*
*
*
* Used to return valid Product Listings.
* If no paramter is passed, will return all the records according to the amount per page specified.
* If $ID is passed, it will only return the corresponding record.
* Clause if any additional WHERE statements you want to pass.
* @param integer $ID
* @param string $Clause
*/
function Product_Listing($ID="",$Clause="") {

$query = "SELECT *
FROM ".$this->_Products_Table."
";
if($this->_UseStock) {
$query .= "WHERE
".$this->StockField." > 0
";
if($ID != "") {
$query .= "
&& ".$this->_IDField."='".$ID."'
";
}
} elseif($ID != "") {
$query .= "WHERE
".$this->_IDField."='".$ID."'
";
if($Clause != "") {
$query .= " && ".$Clause."";
}
} elseif($Clause != "" && $ID == "") {
$query .= "WHERE ".$Clause."";
}
$result = $this->_DB->query($query);
$this->Affected_Rows = $result->numRows();
$query .= "
ORDER BY ".$this->_OrderByField." ".$this->_OrderHow."
";
if($this->_Results_Per_Page != FALSE && $ID == "") {
$query .= " LIMIT ".$this->_LimitPosition.",".$this->_Results_Per_Page."
";
}
$result = $this->_DB->query($query);
if(!DB::isError($result)) {
if($ID != "") {
$row = $result->fetchrow();
foreach($this->ProductFields as $Wanted_Field=>$Name) {
$this->ProductFields[$Wanted_Field] = stripslashes($row->$Wanted_Field);
}
} else {
while($row = $result->fetchrow()) {
foreach($this->ProductFields as $Wanted_Field=>$Name) {
$this->ProductFields[$Wanted_Field][] = stripslashes($row->$Wanted_Field);
}
}
}
}
}

/**
* Products::Pagination()
*
* Used to display page seperation control.
* Valid Methods: ADVANCED & SIMPLE
* Advanced will use the numbered method (1,2,3,4)
* Simple will use Previous Page - Next Page
*
* @param string $Pagination_Method
*/
function Pagination($Pagination_Method="ADVANCED") {
$Output = "";
switch(strtoupper($Pagination_Method)) {
case "ADVANCED" :
case "ADV" :
case "1" :
default : {
$this->_LimitPosition = ($this->_LimitPosition == 0 ? $this->_LimitPosition = 1 : $this->_LimitPosition);
for($Counter=0;$Counter<(ceil($this->Affected_Rows / $this->_Results_Per_Page));++$Counter) {
if(round(($this->_LimitPosition / $this->_Results_Per_Page)) == ($Counter)) {
$Output .= $this->_Pagination_Seperator.
"<".$this->_Pagination_Selected_Style.">".($Counter+1)."</".$this->_Pagination_Selected_Style.">"
.$this->_Pagination_Seperator;
} else {
$Output .= '<a href="'.$_SERVER['PHP_SELF'].'?LimitPage='.round(($Counter*$this->_Results_Per_Page)).'">
'.($Counter+1).'</a>'
.$this->_Pagination_Seperator;
}

}
BREAK;
}
case "SIMPLE" :
case "SIM" :
case "2" : {
if($this->_LimitPosition != 0) {
$Output .= '
<a href="'.$_SERVER['PHP_SELF'].'?LimitPage='.($this->_LimitPosition-$this->_Results_Per_Page).'">
Previous Page</a>
'.$this->_Pagination_Seperator;
} else {
$Output .= '
Previous Page
'.$this->_Pagination_Seperator;
}
if(($this->_LimitPosition+$this->_Results_Per_Page) <= $this->Affected_Rows) {
$Output .= '
<a href="'.$_SERVER['PHP_SELF'].'?LimitPage='.($this->_LimitPosition+$this->_Results_Per_Page).'">
Next Page</a>
'.$this->_Pagination_Seperator;
} else {
$Output .= '
Next Page
';
}
BREAK;
}
}
return $Output;

}

}
?>

[jstarkey]

switch(strtoupper($Pagination_Method)) {
case "ADVANCED" :
case "ADV" :
case "1" :
default : {
$this->_LimitPosition = ($this->_LimitPosition == 0 ? $this->_LimitPosition = 1 : $this->_LimitPosition);
for($Counter=0;$Counter<(ceil($this->Affected_Rows / $this->_Results_Per_Page));++$Counter) {
if(round(($this->_LimitPosition / $this->_Results_Per_Page)) == ($Counter)) {
$Output .= $this->_Pagination_Seperator.
"<".$this->_Pagination_Selected_Style.">".($Counter+1)."</".$this->_Pagination_Selected_Style.">"
.$this->_Pagination_Seperator;
} else {
$Output .= '<a href="'.$_SERVER['PHP_SELF'].'?LimitPage='.round(($Counter*$this->_Results_Per_Page)).'">
'.($Counter+1).'</a>'
.$this->_Pagination_Seperator;
}

}
BREAK;
}



You could avoid a few lines there. You've got a switch statement that's doing a lot of testing but in the end the action will be the same regardless.

Also, I've avoided using /* */ for comments. It makes it hard to comment out blocks of code when debugging.

[Weedpacket]

You could also save a bit of calculation by moving it out of the loop - if an expression doesn't depend
on $Counter, then it will be the same every time:


if($this->_LimitPosition==0) $this->_LimitPostion = 1;

$pages = ceil($this->Affected_Rows / $this->_Results_Per_Page);
$current_page = round($this->_LimitPostion / $this->_Results_Per_Page);

$current_page_label_before = $this->_Pagination_Seperator
. '<' . $this->_Pagination_Selected_Style . '>';

$current_page_label_after = '</' . $this->_Pagination_Selected_Style . '>'
. $this->_Pagination_Seperator;

$page_label_before = '<a href="' . $_SERVER['PHP_SELF'] . '?LimitPage=';
$page_label_after = '</a>' . $this->_Pagination_Seperator;

for($Counter=0; $Counter<$pages; ++$Counter)
{
if($current_page == $Counter)
{
$Output .= $current_page_label_before
. ($Counter+1)
. $current_page_label_after;
} else {
$Output .= $page_label_before . ($Counter*$this->_Results_Per_Page) . '">'
. ($Counter+1) . $page_label_after;
}
}

Though one might take that too far ... that stray '">' in the second branch is a wee bit unsettling.

Oh, and I just noticed, it's spelled "Separator" :)

[mcgruff]

Pagination could be required in many different scripts and so a pagination class ought to be reusable without recoding. You need to make this much more abstract.

Arguments required by the paginator object should be defined in the calling script - don't attempt to do it inside the pagination class itself. What if you need to use a multi-table JOIN query? It's better to define the query string (minus the LIMIT clause) in the calling script and create a pagination class method which simply tacks on a LIMIT clause.

Also, I would recommend some extensive refactoring. Many methods in your code take on too much work: split them into small fns which have just one task.

An object design starts with the interface. What do you want from a paginator class?

(1) a (LIMIT) $query result resource
(2) a pagination hyperlink

..and nothing, but NOTHING else (well maybe a total num rows, should it be required).

Putting all that together, as well as a constructor and the above getters, you might define "private" methods as follows (which ought to be pretty much self-explanatory):

_setTotalNumRows()
_setTotalPages()
_checkPageChoice() - (might not be valid)
_setOffset()
_setPagedQuery()
_setPaginationLink()

The pagination class doesn't know or care what it's trying to paginate - and doesn't do anything else except paginate.

It's also maybe a good idea to parcel out the pagination link code into its own object (try a factory recall pattern - see phppatterns.com). That means you can create different classes for different types of link (next/previous, alphabetical, numeric whatever) and simply plug in whichever one you need for a script. Putting them all in one class creates unecessary bloat - only one will actually be used in a given script.

[jmcneese]

why constantly reinvent the wheel?

PEAR::Pager (http://pear.php.net/package/Pager)

[BuzzLY]

Because if you didn't, we'd still be using wheels chipped out of stone...

Sometimes one takes on a project like this to learn. And PEAR is not the be-all, end-all solution.

[tekky]

Originally posted by jmcneese
why constantly reinvent the wheel?

PEAR::Pager (http://pear.php.net/package/Pager)

I dont use... PEAR... and I'm betting many others dont either... there for... pagination is still needed by some :P

[mcgruff]

Originally posted by jmcneese
why constantly reinvent the wheel?

PEAR::Pager (http://pear.php.net/package/Pager)

This particular wheel is square.. (eg the pagination link should IMHO be a variable factory product object).

In fact, all the PEAR classes I've looked at look like sh**. Eclipse is a better place to look for good OOP design.