Critique Requested on PHP Pagination Class

Question

I did most of my PHP coding back with PHP 4, and among the projects I did was a popular pagination class. I've been meaning to bring it up to date with PHP 5 and OOP practices. I've also been working on updating my database code, using PDO over the deprecated mysql* functions. What I'd like from you is your input and feedback about my updates.

Pagination Class

class Paginator{
    public $items_per_page;
    public $total_items;
    public $current_page;
    public $num_pages;
    public $mid_range;
    public $limit;
    public $limit_start;
    public $limit_end;
    public $return;
    public $querystring;
    public $ipp_array;
    public $get_ipp;
    public $get_page;

    public function __construct($total,$mid_range=7,$ipp_array=array(10,25,50,100,'All')) {
        $this->total_items = $total;
        $this->mid_range = $mid_range;
        $this->ipp_array = $ipp_array;
        $this->items_per_page = (isset($_GET['ipp'])) ? $_GET['ipp'] : $this->ipp_array[0];
        $this->get_ipp = isset($_GET['ipp']) ? $_GET['ipp'] : NULL;
        $this->get_page = isset($_GET['page']) ? (int) $_GET['ipp'] : 1;
        $this->default_ipp = $this->ipp_array[0];

        if($this->get_ipp == 'All') {
            $this->num_pages = 1;
        } else {
            if(!is_numeric($this->items_per_page) OR $this->items_per_page <= 0) $this->items_per_page = $this->ipp_array[0];
            $this->num_pages = ceil($this->total_items/$this->items_per_page);
        }
        $this->current_page = (isset($_GET['page'])) ? (int) $_GET['page'] : 1 ; // must be numeric > 0
        if($_GET) {
            $args = explode("&",$_SERVER['QUERY_STRING']);
            foreach($args as $arg) {
                $keyval = explode("=",$arg);
                if($keyval[0] != "page" And $keyval[0] != "ipp") $this->querystring .= "&" . $arg;
            }
        }
        if($_POST) {
            foreach($_POST as $key=>$val) {
                if($key != "page" And $key != "ipp") $this->querystring .= "&$key=$val";
            }
        }
        if($this->num_pages > 10) {
            $this->return = ($this->current_page > 1 And $this->total_items >= 10) ? "<a class=\"paginate\" href=\"$_SERVER[PHP_SELF]?page=".($this->current_page-1)."&ipp=$this->items_per_page$this->querystring\">Previous</a> ":"<span class=\"inactive\" href=\"#\">Previous</span> ";
            $this->start_range = $this->current_page - floor($this->mid_range/2);
            $this->end_range = $this->current_page + floor($this->mid_range/2);
            if($this->start_range <= 0) {
                $this->end_range += abs($this->start_range)+1;
                $this->start_range = 1;
            }
            if($this->end_range > $this->num_pages) {
                $this->start_range -= $this->end_range-$this->num_pages;
                $this->end_range = $this->num_pages;
            }
            $this->range = range($this->start_range,$this->end_range);
            for($i=1;$i<=$this->num_pages;$i++) {
                if($this->range[0] > 2 And $i == $this->range[0]) $this->return .= " ... ";
                // loop through all pages. if first, last, or in range, display
                if($i==1 Or $i==$this->num_pages Or in_array($i,$this->range)) {
                    $this->return .= ($i == $this->current_page And $this->get_page != 'All') ? "<a title=\"Go to page $i of $this->num_pages\" class=\"current\" href=\"#\">$i</a> \n":"<a class=\"paginate\" title=\"Go to page $i of $this->num_pages\" href=\"$_SERVER[PHP_SELF]?page=$i&ipp=$this->items_per_page$this->querystring\">$i</a> \n";
                }
                if($this->range[$this->mid_range-1] < $this->num_pages-1 And $i == $this->range[$this->mid_range-1]) $this->return .= " ... ";
            }
            $this->return .= (($this->current_page < $this->num_pages And $this->total_items >= 10) And ($this->get_page != 'All') And $this->current_page > 0) ? "<a class=\"paginate\" href=\"$_SERVER[PHP_SELF]?page=".($this->current_page+1)."&ipp=$this->items_per_page$this->querystring\">Next</a>\n":"<span class=\"inactive\" href=\"#\">Next</span>\n";
            $this->return .= ($this->get_page == 'All') ? "<a class=\"current\" style=\"margin-left:10px\" href=\"#\">All</a> \n":"<a class=\"paginate\" style=\"margin-left:10px\" href=\"$_SERVER[PHP_SELF]?page=1&ipp=All$this->querystring\">All</a> \n";
        } else  {
            for($i=1;$i<=$this->num_pages;$i++) {
                $this->return .= ($i == $this->current_page) ? "<a class=\"current\" href=\"#\">$i</a> ":"<a class=\"paginate\" href=\"$_SERVER[PHP_SELF]?page=$i&ipp=$this->items_per_page$this->querystring\">$i</a> ";
            }
            $this->return .= "<a class=\"paginate\" href=\"$_SERVER[PHP_SELF]?page=1&ipp=All$this->querystring\">All</a> \n";
        }
        $this->return = str_replace('&','&amp;',$this->return);
        $this->limit_start = ($this->current_page <= 0) ? 0:($this->current_page-1) * $this->items_per_page;
        if($this->current_page <= 0) $this->items_per_page = 0;
        $this->limit_end = ($this->get_ipp == 'All') ? (int) $this->total_items: (int) $this->items_per_page;
    }
    public function display_items_per_page() {
        $items = NULL;
        natsort($this->ipp_array); // This sorts the drop down menu options array in numeric order (with 'all' last after the default value is picked up from the first slot
        if(is_null($this->get_ipp)) $this->items_per_page = $this->ipp_array[0];
        foreach($this->ipp_array as $ipp_opt) $items .= ($ipp_opt == $this->items_per_page) ? "<option selected value=\"$ipp_opt\">$ipp_opt</option>\n":"<option value=\"$ipp_opt\">$ipp_opt</option>\n";
        return "<span class=\"paginate\">Items per page:</span><select class=\"paginate\" onchange=\"window.location='$_SERVER[PHP_SELF]?page=1&amp;ipp='+this[this.selectedIndex].value+'$this->querystring';return false\">$items</select>\n";
    }
    public function display_jump_menu() {
        $option=NULL;
        for($i=1;$i<=$this->num_pages;$i++) {
            $option .= ($i==$this->current_page) ? "<option value=\"$i\" selected>$i</option>\n":"<option value=\"$i\">$i</option>\n";
        }
        return "<span class=\"paginate\">Page:</span><select class=\"paginate\" onchange=\"window.location='$_SERVER[PHP_SELF]?page='+this[this.selectedIndex].value+'&amp;ipp=$this->items_per_page$this->querystring';return false\">$option</select>\n";
    }
    public function display_pages() {
        return $this->return;
    }
}

Sample Instantiation

<?php
include('paginator.class.php');
try {
    $conn = new PDO('mysql:host=localhost;dbname=test', 'user', 'pass');
    $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); 
    $num_rows = $conn->query('SELECT COUNT(*) FROM City')->fetchColumn(); 

    $pages = new Paginator($num_rows,9,array(15,3,6,9,12,25,50,100,250,'All'));
    echo $pages->display_pages();
    echo "<span class=\"\">".$pages->display_jump_menu().$pages->display_items_per_page()."</span>";

    $stmt = $conn->prepare('SELECT City.Name,City.Population,Country.Name,Country.Continent,Country.Region FROM City INNER JOIN Country ON City.CountryCode = Country.Code ORDER BY City.Name ASC LIMIT :start,:end');
    $stmt->bindParam(':start', $pages->limit_start, PDO::PARAM_INT);
    $stmt->bindParam(':end', $pages->limit_end, PDO::PARAM_INT);
    $stmt->execute();
    $result = $stmt->fetchAll();

    echo "<table><tr><th>City</th><th>Population</th><th>Country</th><th>Continent</th><th>Region</th></tr>\n";
    foreach($result as $row) {
        echo "<tr><td>$row[0]</td><td>$row[1]</td><td>$row[2]</td><td>$row[3]</td><td>$row[4]</td></tr>\n";
    }
    echo "</table>\n";
    echo $pages->display_pages();
    echo "<p class=\"paginate\">Page: $pages->current_page of $pages->num_pages</p>\n";
    echo "<p class=\"paginate\">SELECT * FROM table LIMIT $pages->limit_start,$pages->limit_end (retrieve records $pages->limit_start-".($pages->limit_start+$pages->limit_end)." from table - $pages->total_items item total / $pages->items_per_page items per page)";
} catch(PDOException $e) {
    echo 'ERROR: ' . $e->getMessage();
}
?>

Live Demos

• Demo 1
• Demo 2

Areas of Concern

I noticed in my query that if I try to execute the SQL with:

$stmt->execute( array(':start'=>$pages->limit_start,':end'=>$pages->limit_end) );

I get an error unless I also use $conn->setAttribute(PDO::ATTR_EMULATE_PREPARES, FALSE);. However I can get around that if I use bindParam instead:

$stmt->bindParam(':start', $pages->limit_start, PDO::PARAM_INT);
$stmt->bindParam(':end', $pages->limit_end, PDO::PARAM_INT);
$stmt->execute();

Should I be using $conn->setAttribute(PDO::ATTR_EMULATE_PREPARES, FALSE); or the separate bindParam calls?bindParam seems like a decent solution, whereas using ATTR_EMULATE_PREPARESseems a little hacky.

Within the class, am I setting the visibility (public, private, protected) of my parameters and methods properly?

If you see anything that jumps out at you like "what in the world was he thinking?", you can assume that I was dealing with something I hadn't done before. Any tips or suggestions overall would be appreciated.

Answer 1

Okay, I’m going to list anything and everything that came in my mind while reading your code.

1. Start documenting your code for better maintainability
2. Validate every and any user input
3. Use proper camelCase notation for property and method names
4. Separate your code into more methods (especially in __construct)
5. Keep everything protected unless you have a good reason to keep it private or expose it with public
6. Use single or double quotes throughout your code, but don’t mix it (consistency)
7. Enclose your variables in {} if you embed them in strings
8. Use single quotes for your HTML if you use double quotes for creating the string (prevent escaping mistakes)
9. Use namespace’s (even if you don’t need them now)

An example class:

<?php

/*!
 * My license
 */
namespace \Fleshgrinder;

/**
 * Example class for Code Review at Stack Exchange.
 *
 * @author Fleshgrinder <[email protected]>
 */
class ExampleClass {

    /**
     * The example class's example property.
     *
     * @var mixed
     */
    protected $camelCasePropertyName;

    /**
     * Instantiate new example class.
     *
     * @var string $param1
     *   Example parameter 1.
     * @var string $param2 [optional]
     *   Optional example parameter 2.
     * @throws \RuntimeException
     */
    public function __construct($param1, $param2 = "foo") {
        if ($param2 == "foo") {
            $this->camelCasePropertyName = $this->camelCaseMethodName();
        }
        elseif ($param2 == "bar") {
            throw new \RuntimeException("No bar!");
        }
        else {
            $this->camelCasePropertyName = $param1;
        }
    }

    /**
     * Get <var>$str</var> wrapped in <code>span</code> HTML element.
     *
     * @param string $str
     *   The string to wrap.
     * @return string
     *   The wrapped string.
     */
    protected function camelCaseMethodName($str) {
        return "<span class='foo'>{$str}</span>";
    }

}

---

Example regarding creation of methods that help to understand the code better (see comments):

<?php

// No comments to keep it short, but always write comments!
class Database extends FictivePHPDBClass {

    public function __construct($user, $password, $host, $port, $database) {
        $this->setUser($user);
        $this->setPassword($password);
        $this->setHostname($host);
        $this->setPort($port);
        $this->connect();
        $this->selectDatabase($database);
    }

    protected function setUser($user) {
        if (empty($user) || !is_string($user)) {
            throw new \IllegalArgumentException("Username cannot be empty.");
        }
        $this->user = $user;
    }

    protected function setPassword($password) {}

    protected function setHostname($hostname) {}

    protected function setPort($port) {}

    protected function connect() {
        $phpDB = parent::connect($this->user, $this->password, $this->hostname, $this->port);
        if ($phpDB->errorCount > 0) {
            throw new \Fleshgrinder\Exception\DatabaseException("Couldn't connect to {$this->hostname} database.");
        }
        $this->connection = $phpDB;
    }

    protected function selectDatabase($database) {
        if (empty($database) || !is_string($database)) {
            throw new \IllegalArgumentException("Bla bla ...");
        }
        if (!($this->connection instanceof \FictivePHPDBClass)) {
            throw new \LogicException("Bla bla ...");
        }
        $this->connection->selectDatabase($database);
        if ($this->connection->errorCount > 0) {
            throw new \Fleshgrinder\Exception\DatabaseException("Bla bla ...");
        }
    }

}

That's stuff you often see in OO PHP code, wrapping some other classes and trying to make some real OO code out of them. But what I really want to show you is the fact that the methods are short, easy to understand because of that and that you already know what each method does only by reading their name.

Answer 2

You can use commas to separate similarly scoped class properties. This will make your code a little easier to read and make it easier to modify.

public
    $items_per_page,
    $total_items,
    $current_page,
    //etc...
    $get_page
;

You probably shouldn't rely on GET variables. Passing these values in as parameters allows for more extensibility. That being said, I think your constructor might be doing a tad too much already and you might consider using some individual setters. Look into the Single Responsibility Principle. This goes along with what Fleshgrinder said about using more methods.

Please, always use braces on your statements, especially on long complex ones. PHP, by default, does not support true braceless syntax, otherwise you would be able to neglect them everywhere. Doing so makes your code much more legible. The advantages of neglecting them, if there even are any, are negligible.

if(!is_numeric($this->items_per_page) OR $this->items_per_page <= 0) $this->items_per_page = $this->ipp_array[0];

//compared to
if(!is_numeric($this->items_per_page) OR $this->items_per_page <= 0) {
    $this->items_per_page = $this->ipp_array[0];
}

The same applies for foreach loops. In addition, ternary statements are nifty, but should only be used when doing so does not detract from legibility. In the following code I have no idea what's going on.

foreach($this->ipp_array as $ipp_opt) $items .= ($ipp_opt == $this->items_per_page) ? "<option selected value=\"$ipp_opt\">$ipp_opt</option>\n":"<option value=\"$ipp_opt\">$ipp_opt</option>\n";

//compared to
foreach($this->ipp_array as $ipp_opt) {
    if($ipp_opt == $this->items_per_page) {
        $items .= "<option selected value=\"$ipp_opt\">$ipp_opt</option>\n";
    } else {
        $items .= "<option value=\"$ipp_opt\">$ipp_opt</option>\n";
    }
}

Additionally, the above could be modified to reduce repetition like so:

$selected = $ipp_opt == $this->items_per_page ? 'selected' : '';
$items .= "<option $selected value=\"$ipp_opt\">$ipp_opt</option>\n";

In response to some of Fleshgrinder's points:

Use proper cameCase notation for property and method names

This is a stylistic choice, though one I agree with. This is entirely up to you.

Keep everything protected unless you have a good reason to keep it private or expose it with public

Good advice, but to expand upon this, an element only has to be public if it should be accessible or modifiable outside of its current scope (the current class).

Use single or double quotes throughout your code, but don’t mix it (consistency)

Again, another stylistic point... sort of... Single quotes are string literals. This means that PHP interprets the contents between single quotes as being nothing more than a string. Double quotes are processed by PHP for evaluation, thus you are able to put PHP variables directly in the string and have them be properly escaped. As such, double quotes are ever-so-slightly slower, but it is negligible. Consistency is important, but don't let it stop you from using single/double quotes as they were meant to be used.

Enclose your variables in {} if you embed them in strings

This is only necessary if the variable you are including is complex. For example:

$string = "This is $simple, and this is {$complex[ 'elem' ]}";

Sorry this is kind of rushed, I might come back and add to this later. Hope this helps!