Page 1 of 1

Critique my pagination class

Posted: Tue Dec 21, 2010 1:17 am
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.

Re: Critique my pagination class

Posted: Tue Dec 21, 2010 12:59 pm
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.

Re: Critique my pagination class

Posted: Tue Dec 21, 2010 10:34 pm
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.

Re: Critique my pagination class

Posted: Wed Dec 22, 2010 5:55 am
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.

Re: Critique my pagination class

Posted: Wed Dec 22, 2010 12:28 pm
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
);

Re: Critique my pagination class

Posted: Wed Dec 22, 2010 1:22 pm
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.

Re: Critique my pagination class

Posted: Wed Dec 22, 2010 4:51 pm
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:

Re: Critique my pagination class

Posted: Sun Jan 02, 2011 6:45 am
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)?