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.
Okay, so I've written my own pagination script because I feel the ones that can be found on the internet are too complex. I much prefer to keep things as simple and concise as possible. However, I'm aware that when things are simplified, such things can lose some of their functionality. That's why I'd like you to critique this code and let me know if anything is missing and/or problematic.
The first part looks correct, to me. I don't understand what you're trying to do in that for loop at the bottom. I would recommend that you validate the $_GET['page'] value, though, before using it.
califdon wrote:The first part looks correct, to me. I don't understand what you're trying to do in that for loop at the bottom. I would recommend that you validate the $_GET['page'] value, though, before using it.
I'll give you a few examples of the for loop in action.
And I didn't think I'd have to worry about validating the $_GET["page"] variable. Only if $_GET["page"] is greater than or equal to 1, and less than or equal to $pages, will it be used; else the $page equals 1. Anything other than a valid numeric would fail the if statement, and so the page defaults to 1.
Jcart wrote:You are unnecesarily querying your entire dataset. If all you need the total number of rows, use COUNT(id) instead.
Thank you. And does it matter if I use mysql_fetch_array over mysql_fetch_assoc? Or should I use the latter?
$result = mysql_query("SELECT COUNT(`id`) AS `total` FROM `users` WHERE `location` = '".$location."'");
$row = mysql_fetch_assoc($result);
$total = $row['total'];
There is some ready-to-use functionality in MySQL for solving this problem:
[sql]SELECT SQL_CALC_FOUND_ROWS id, username, email FROM users WHERE username < 'B' LIMIT 10;SELECT FOUND_ROWS();[/sql]
No coma after SQL_CALC_FOUND_ROWS .
Last edited by VladSun on Sun Jan 04, 2009 8:52 am, edited 1 time in total.
There are 10 types of people in this world, those who understand binary and those who don't
MichaelR wrote:And I didn't think I'd have to worry about validating the $_GET["page"] variable. Only if $_GET["page"] is greater than or equal to 1, and less than or equal to $pages, will it be used; else the $page equals 1. Anything other than a valid numeric would fail the if statement, and so the page defaults to 1.
You don't have an echo $page in your current code, but it's not for sure you will not use it somewhere else. So - always validate, sanitize and filter the user input.
MichaelR wrote:Thank you. And does it matter if I use mysql_fetch_array over mysql_fetch_assoc? Or should I use the latter?
mysql_fetch_array will return a two times longer array than mysql_fetch_assoc - it will contain both associative array and a zero based indexed array. So, I'd use mysql_fetch_assoc, or mysql_fetch_object, or mysql_fetch_row, but not mysql_fetch_array.
There are 10 types of people in this world, those who understand binary and those who don't
VladSun wrote:You don't have an echo $page in your current code, but it's not for sure you will not use it somewhere else. So - always validate, sanitize and filter the user input.
Thank you, I will remember to do so. Better safe than sorry, I guess.
mysql_fetch_array will return a two times longer array than mysql_fetch_assoc - it will contain both associative array and a zero based indexed array. So, I'd use mysql_fetch_assoc, or mysql_fetch_object, or mysql_fetch_row, but not mysql_fetch_array.
Would that be a blanket rule? Or just for something like this pagination script? So, say I wished to select a user's data: