1
\$\begingroup\$

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

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.

\$\endgroup\$
4
\$\begingroup\$

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@example.com>
 */
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.

\$\endgroup\$
  • \$\begingroup\$ Awesome, just the kind of input I'm looking for, thanks. Could you expand on point #4? \$\endgroup\$ – j08691 Nov 12 '13 at 21:29
  • \$\begingroup\$ Try to create methods (functions in OO) that have a name that explains what the do and try to keep the method's body (the actual code) short, so that's easy to grasp what's going on. For instance, start by calling a method for each if/else if/else you have. I extend my answer with an example in a minute. \$\endgroup\$ – Fleshgrinder Nov 12 '13 at 21:40
  • \$\begingroup\$ You've added a sample defining class Database extends FictivePHPDBClass. Please, please, please: it would appear you're advocating/approving of extending from classes like PDO! Don't... PDO child classes are like broken pencils: pointless. I've been quite verbose about the subject here! \$\endgroup\$ – Elias Van Ootegem Nov 13 '13 at 12:51
  • \$\begingroup\$ It's only an example for coding style and not a call to extend core classes. I illustrated it with something one often stumbles upon in endless projects. Personally I totally agree with your point in almost 99% of the cases (there are exceptions). \$\endgroup\$ – Fleshgrinder Nov 13 '13 at 13:14
2
\$\begingroup\$

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!

\$\endgroup\$
  • \$\begingroup\$ Thanks! I understand what you and Fleshgrinder are saying about the long constructor, but I'm not sure what I might gain by breaking it up. It really only has a single purpose and that's to do the bulk of the work building the actual pagination. Am I missing the obvious here? How should I break the constructor up and what will the user gain? \$\endgroup\$ – j08691 Nov 15 '13 at 0:07
  • \$\begingroup\$ Grouping with spaces of properties isn't good if you want to properly document them all because the visibility is too far away. Always using {} = consistency. Double quotes are faster than single quotes, this is a common disbelief (see micro-optimization.com/single-vs-double-quotes) and embedding rather than concatenating is faster as well. Other than that I agree on all your points. @j08691 it greatly improves maintainability, but it hurts performance. \$\endgroup\$ – Fleshgrinder Nov 15 '13 at 2:10
  • \$\begingroup\$ @Fleshgrinder: What do you mean the visibility is too far away? You can use doccomments like normal. Always using {} isn't consistency, its a preference. Its only necessary for complex variables. As for your double vs single quotes argument, I'll have to do some of my own research on that, because I've seen a similar site that proposed the exact opposite, either way, the difference is negligible. \$\endgroup\$ – mseancole Nov 15 '13 at 2:49
  • \$\begingroup\$ @j08691: It improves maintainability, as fleshgrinder said, and the loss in performance is negligible. The users will not gain anything from it, but those extending your code will better be able to read it and extend it, thus you will better be able to read it and extend it. A couple of examples of how you could abstract methods from your constructor, initiation() to initialize properties and getQueryString() to produce the query string based on input type. There are more in your code, these are just a couple. \$\endgroup\$ – mseancole Nov 15 '13 at 2:56
  • \$\begingroup\$ @mseancole - Thanks. In my older code the constructor was smaller and the class had more methods, but I thought combining them would make more sense and streamline things, since the constructor really is doing the heavy lifting in this particular class. \$\endgroup\$ – j08691 Nov 15 '13 at 3:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.