Critique my pagination class

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.

Moderator: General Moderators

Post Reply
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Critique my pagination class

Post by social_experiment »

Hello,

This is a pagination class that i created and I am looking for feedback on it. I created it because pagination comes up often when i work for clients and having to constantly copy functions and amend them is becoming tedious. This class displays the paginated results from a database in a tabular form with navigation just below that.

When i created it i thought about implementing it without having the user (of the script) enter database information like username, password etc. It should be easy to incorporate into an existing system and use existing resources. The constructor accepts the arguments needed for the script to work in the following order:

1. A mysqli link identifier
2. An array with field names
3. A name of a page where the pagination will be displayed (sans an extension)
4. Table name
5. Count column (Prefrably a PK that is auto_increment)
6. The amount of items per page

Because the script uses an existing connection I suppressed all the SQL errors but if they should occur a notification message will be issued along with the mysqli_error() explanation (if any). Database connection errors in this instance is the responsibilitty of the person using the script and not of the script itself (another pluspoint IMO).

If you do test the script (a pre-emptive Thank you is in order) i'd welcome the following feedback:

Q1. Is it easy to use? I would like newbies to also use this but it might contain code that is more within the grasp of intermediate users.
Q2. How can you break it? Things like "I entered an invalid table name" or "I injected some additional sql code.." would be helpful.
Q3. How can it be improved? My goal wasn't to create the silver bullet for pagination but instead a class that can be easily implemented with minimal effort and something that would solve a common problem for many users.

I tested the script on a XAMPP server running PHP 5.2.5 and MySQL server version 3.23.32.
Attachments
_pagination.zip
pagination class
(1.86 KiB) Downloaded 509 times
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: Critique my pagination class

Post by Jonah Bron »

social_experiment wrote:[...] a pre-emptive Thank you is in order [...]
Not sure if pre-emptive is the right word... :wink:

Didn't test it, but browsed the code a bit. Here are some things I noticed.

You should change your class name/etc. so that it can be dropped into a lib directory and autoloaded. Here's the conventions: http://groups.google.com/group/php-stan ... l-proposal . That will make it much easier for people to use it, especially if they're using a framework. You would probably call your class SE_Pagination.

In createPagination(), you call die() several times. I think you should change those to exceptions, so that the caller can wrap it in a try...catch statement. You may also want to create a few Exception class for encapsulating different errors.

In createPages(), it seems like there ought to be some more flexibility in the links. Perhaps you should require a callback that you pass the link URL that passes back a link. That way the developer can define the class, the message (e.g. ">" instead of "Next").

The same with createPagination(). There should be a way for the developer to choose how the output is formatted. If the idea is that you would just change values in the library, then it's not a good one. Libraries should remain static.

The exception messages should be placed in constants, instead of the code. It's my opinion that it's best practice to keep variable strings out of the program logic. Names like ERROR_INVALID_PAGE, ERROR_FIELD_NOT_ARRAY are good examples.

Not a big issue, but PHPDoc comments start with /**, not /* :)

And lastly, lots of frameworks have pagination classes built in. You may wish to look at the Skeleton (which I happen to work on) pagination lib for some inspiration: https://code.google.com/p/skeleton/sour ... Pagination

And lastly, do your changes under version control? If not, I really recommend it.
User avatar
Christopher
Site Administrator
Posts: 13592
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Critique my pagination class

Post by Christopher »

social_experiment wrote:Q1. Is it easy to use? I would like newbies to also use this but it might contain code that is more within the grasp of intermediate users.
It is probably pretty easy if you use MySQL, the mysqli extension and want links exactly like you generate.
social_experiment wrote:Q2. How can you break it? Things like "I entered an invalid table name" or "I injected some additional sql code.." would be helpful.
This is an almost endless list in my experience, depending how how many features and niceties you want to add. And I think how you check all the values with _sanitizeValue() looks wrong. If you expect and int then only allow an int, etc.
social_experiment wrote:Q3. How can it be improved? My goal wasn't to create the silver bullet for pagination but instead a class that can be easily implemented with minimal effort and something that would solve a common problem for many users.
Unfortunately it is not a problem that can be solved that easily -- event though it seems like it might be.
Jonah Bron wrote:And lastly, lots of frameworks have pagination classes built in. You may wish to look at the Skeleton (which I happen to work on) pagination lib for some inspiration: https://code.google.com/p/skeleton/sour ... Pagination
This is a good example of a general purpose solution that ends up being many classes to cover all the permutations of different programmer wants and needs. That's not a bad this, but shows the complexity needed for a generalized solution.
(#10850)
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Critique my pagination class

Post by social_experiment »

Jonah Bron wrote:Not sure if pre-emptive is the right word...
:lol: Yeah it's infact very wrong but it's the thought that counts :)

@Jonah Bron: Thanks for the feedback & hints 8) . My intention wasn't create something to use in a framework, many of them (as you point out) already have pagination classes or something to that effect. I had in mind something that would work if you are using a non-framework script and don't understand pagination but want to use it. I looked at the pagination scripts you suggested and they do cover a wide range of things and there are things that I did think about but never added them, i'll rewrite it and see if i can make it more flexable.

@Christopher: Thank you :) Yeah. A question with many different answers pagination is. As i mentioned in my reply to Jonah, i didn't set out to create a framework type class but something simple to use and easy to understand. I most definitely neglected some of the things and those i tend to fix in the next version of the script.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: Critique my pagination class

Post by Jonah Bron »

I think your class should concentrate less on the actual content, and more on the number links. For the content, it's my opinion that you should just generate a partial SQL query, like this:

Code: Select all

LIMIT 5, 30
That is passed back to the caller so that they can it can execute it's query, and output the content. Or even just an associative array; that way the class can be used to paginate arrays.

Code: Select all

array(
    'offset' => 5,
    'limit' => 30
);
User avatar
Christopher
Site Administrator
Posts: 13592
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Critique my pagination class

Post by Christopher »

social_experiment wrote:@Christopher: Thank you :) Yeah. A question with many different answers pagination is. As i mentioned in my reply to Jonah, i didn't set out to create a framework type class but something simple to use and easy to understand. I most definitely neglected some of the things and those i tend to fix in the next version of the script.
The problem is that one person's simple may not be clear to another person. And with things like pagination, users usually fall into several groups. Some just want the calculations done; others want link generation; others want the SQL querying done for them. The design discussion for the Skeleton pagination classes was a 400+ comment thread (you can read it if you like). It provides everything from a 1-2 line do-it-all solution ... to may specific classes/methods to create the exact pagination solution you want. And there is the problem of diminishing returns the more features you add.
(#10850)
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Critique my pagination class

Post by social_experiment »

Jonah Bron wrote:I think your class should concentrate less on the actual content...
Yeah, after looking at those other examples I realize im taking a path that might seem simple but is infact limiting and restricting. Using a straight forward "SELECT" might show all the records but it's stopping the user from choosing other types of displays (ORDER BY, WHERE field = value, etc). Adding the page option gives the ability to display the records on another page but that requires additional checking and security that could be bypassed.

@Christopher: It seems that the best one can hope for is to create a class and hope that somebody finds it useful :idea:
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Critique my pagination class

Post by social_experiment »

I've revised my previous attempt and made the following changes:

- Added error constants
- Created multiple classes, a core that contains basic functions used for pagination and 3 additional classes (extending the core class) which are geared towards certain types of pagination (SQL, SQLi and pagination using an array)
- Changed sanitize method
- Pagination can now be created from more complex queries instead of only SELECT fields FROM table
- Improved the method for aquiring the name of the page where pagination will be used

When throwing an exception, should i stick with only a generic error message or use options like mysql_error() to display more information about the error situation (for instance syntax errors in the queries)?
Attachments
pagination.zip
Revised pagination class.
(6.6 KiB) Downloaded 478 times
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
Post Reply