patternphpMinor
Pagination filtering
Viewed 0 times
paginationfilteringstackoverflow
Problem
I want to check if there is page variable (in URL) and if it is, if it's correct - it cannot be number
Could it be improved anyway, or is it the best and shortest it can be?
1 because this is default page and it should be valid int from 2 to max number of pages (not octal and not hexadecimal). ['min_range' => 2, 'max_range' => $totalPages]]);
if ($selPage === false) {
exit('redirection'); // this line for test only - in fact make 301 redirection here to correct url
}
}
echo $selPage;Could it be improved anyway, or is it the best and shortest it can be?
Solution
Important:
Asking for the "best and shortest" code is a contradiction. If you write something as short as possible, chances are you're taking short-cuts in terms of error handling, maintainability, and readability. Take, for example these two snippets of code. Both do exactly the same thing, but one is so horribly obfuscated that nobody in their right mind would consider it to be good code:
Now I realize that this code is really rather pointless, but it's simply to prove a point. Which of these two snippets of code would you rather be confronted with, when trying to debug something? Or which snippet of code would be easier to explain? I don't know you, but assuming you're not a masochist, I'm going to assume you prefer the second version...
It contains comments, explaining what bit of code does what, it uses (more or less) meaningful variable names, and most of all, there isn't a single line of code that does more than it needs to. Cramming everything in one statement is the shortest route to introducing odd bugs, and because you've crammed everything into a one-liner, those bugs can prove tricky to fix.
Anyway, let's look at your code:
You have a fundamental flaw in your code: even if the
There's no way you'll ever get the actual requested page. I'd suggest you write a function to get the correct page number:
Wrap this call in a try-catch block, and redirect the user if the exception is thrown. The benefit of this function is that you can easily re-use this function to validate other types of data:
At first glance, this code seems slightly more verbose, but not much, and it's a "price" worth paying considering the benefits:
But all in all, I'd suggest you look into existing paginator tools. There are plenty of them, some of them are really quite good, like the KnpLabs paginatorBundle.
Asking for the "best and shortest" code is a contradiction. If you write something as short as possible, chances are you're taking short-cuts in terms of error handling, maintainability, and readability. Take, for example these two snippets of code. Both do exactly the same thing, but one is so horribly obfuscated that nobody in their right mind would consider it to be good code:
$a = range('a', 'z');
for ($a=range('a','z'),$i=0,$j=count($a),$s='',$k=mt_rand()%j;$i<$j;++$i, unset($a[$k]))
{
while (!isset($a[$k])) $k = mt_rand()%j;
$s .= $a[mt_rand()%$j];
}
//compared to
$chars = range('a', 'z');//array of all chars to shuffle
$len = count($chars);//get the length, avoid calling count too many times
$string = '';//the string containing the chars in random order
for ($i=0;$i<$len;++$i)
{
do
{//keep generating a random key, until we find one that exists
$k = mt_rand()%$len;
} while(!isset($chars[$k]));
$string .= $chars[$k];//add char to string
unset($chars[$k]);//unset the char
}Now I realize that this code is really rather pointless, but it's simply to prove a point. Which of these two snippets of code would you rather be confronted with, when trying to debug something? Or which snippet of code would be easier to explain? I don't know you, but assuming you're not a masochist, I'm going to assume you prefer the second version...
It contains comments, explaining what bit of code does what, it uses (more or less) meaningful variable names, and most of all, there isn't a single line of code that does more than it needs to. Cramming everything in one statement is the shortest route to introducing odd bugs, and because you've crammed everything into a one-liner, those bugs can prove tricky to fix.
Anyway, let's look at your code:
You have a fundamental flaw in your code: even if the
page parameter is set, and is valid, the first statement of your code is overwriting it. Suppose the page parameter is 4, the first thing you do is:$_GET['page'] = '01';//<-- reassignmentThere's no way you'll ever get the actual requested page. I'd suggest you write a function to get the correct page number:
/**
* @param array $params
* @param string $key
* @param array $filter
* @param mixed $default = null
* @return mixed
* @throws \InvalidArgumentException
*/
function getValue(array $params, $key, array $filter, $default = null)
{
if (!$params || !isset($params[$key])
return $default;//no GET params, or key is not set, return default
$value = $params[$key];//use the raw value
//validate
$validated = filter_var($value, $filter['type'], $filter['options']);
if ($validated === false)
{//validation failed
throw new \InvalidArgumentException(
sprintf(
'%s is an invalid value',
$value
)
);
}
return $validated;
}
//usage:
$page = getValue(
$_GET,
'page',
array(
'type' => FILTER_VALIDATE_INT,
'options' => array(
'min_range' => 1,//1 is valid, if user was on page 2, and requests page 1 again
'max_range' => 10
),
1//default value
);Wrap this call in a try-catch block, and redirect the user if the exception is thrown. The benefit of this function is that you can easily re-use this function to validate other types of data:
$email = getValue(
$_POST,
'email',
array(
'type' => FILTER_VALIDATE_EMAIL,
'options' => null
);
if ($email === null)
//handle $_POST['email'] not set errorAt first glance, this code seems slightly more verbose, but not much, and it's a "price" worth paying considering the benefits:
- Reusable function
- one call to validate
$_GET,$_POST,$_COOKIEand$_SESSIONkeys
- Can be used on any array (DB results, API responses)
- Easy error handling (catch the exception, handle default returns if required)
- If argument not set, get a usable default value back (like passing 1 in the page example as default value).
- Maintainable code
- This function might well be the first step towards building your own
Requestclass.
But all in all, I'd suggest you look into existing paginator tools. There are plenty of them, some of them are really quite good, like the KnpLabs paginatorBundle.
Code Snippets
$a = range('a', 'z');
for ($a=range('a','z'),$i=0,$j=count($a),$s='',$k=mt_rand()%j;$i<$j;++$i, unset($a[$k]))
{
while (!isset($a[$k])) $k = mt_rand()%j;
$s .= $a[mt_rand()%$j];
}
//compared to
$chars = range('a', 'z');//array of all chars to shuffle
$len = count($chars);//get the length, avoid calling count too many times
$string = '';//the string containing the chars in random order
for ($i=0;$i<$len;++$i)
{
do
{//keep generating a random key, until we find one that exists
$k = mt_rand()%$len;
} while(!isset($chars[$k]));
$string .= $chars[$k];//add char to string
unset($chars[$k]);//unset the char
}$_GET['page'] = '01';//<-- reassignment/**
* @param array $params
* @param string $key
* @param array $filter
* @param mixed $default = null
* @return mixed
* @throws \InvalidArgumentException
*/
function getValue(array $params, $key, array $filter, $default = null)
{
if (!$params || !isset($params[$key])
return $default;//no GET params, or key is not set, return default
$value = $params[$key];//use the raw value
//validate
$validated = filter_var($value, $filter['type'], $filter['options']);
if ($validated === false)
{//validation failed
throw new \InvalidArgumentException(
sprintf(
'%s is an invalid value',
$value
)
);
}
return $validated;
}
//usage:
$page = getValue(
$_GET,
'page',
array(
'type' => FILTER_VALIDATE_INT,
'options' => array(
'min_range' => 1,//1 is valid, if user was on page 2, and requests page 1 again
'max_range' => 10
),
1//default value
);$email = getValue(
$_POST,
'email',
array(
'type' => FILTER_VALIDATE_EMAIL,
'options' => null
);
if ($email === null)
//handle $_POST['email'] not set errorContext
StackExchange Code Review Q#59979, answer score: 2
Revisions (0)
No revisions yet.