3
\$\begingroup\$

I want to check if there is page variable (in URL) and if it is, if it's correct - it cannot be number 1 because this is default page and it should be valid int from 2 to max number of pages (not octal and not hexadecimal).

<?php

$_GET['page'] = '01'; // this line is only for testing

$selPage = 1;
$totalPages = 10;

if (isset($_GET['page'])) {
    $selPage = filter_var($_GET['page'], FILTER_VALIDATE_INT, ['options' => ['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?

\$\endgroup\$
2
\$\begingroup\$

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:

$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';//<-- reassignment

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:

/**
 * @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 error

At 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, $_COOKIE and $_SESSION keys
  • 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 Request class.

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.

\$\endgroup\$
  • \$\begingroup\$ $_GET['page'] = '01'; was obviously for testing the code. In real code of course this line wouldn't be present. I should have added it in the question (already added comment). And of course I know that this code could be moved to function. However my main question is if this code is optimal or it could be shorter. \$\endgroup\$ – Marcin Nabiałek Aug 14 '14 at 11:28
  • \$\begingroup\$ @MarcinNabiałek: I've updated my answer. Shorter code should never be your primary objective. Write bug free, and bug-proof code is far more important. Next, focus on maintainability and portability. Only if all of those criteria have been fulfilled, you can look into optimizing/shortening your code, provided it does not harm readability, maintainability and error-proofness, of course \$\endgroup\$ – Elias Van Ootegem Aug 14 '14 at 11:36
  • \$\begingroup\$ +1 for you for your help. Of course I agree with you that shorter doesn't mean better. I simply want someone to look at this code and tell me - it could be done better because this code doesn't handle some case or has bug or "no", it's the best one that achieves the goal. The problem with many libraries is that they don't care about urls at all. For example if you put page=1 in url, they would display 1st page and if you put page=-100 they will also display 1st page. I don't want to have duplicated urls so validation of exact values is important for me. \$\endgroup\$ – Marcin Nabiałek Aug 14 '14 at 11:59
  • \$\begingroup\$ @MarcinNabiałek: I understand your point about existing bundles/modules, but that's the trade-off: you don't have to spend time writing a paginator, but instead, you use one that wasn't tailor-made for you. Of course, you're free to fork any one of the open-source paginators, and edit it to be more strict, or use them in such a way that you can validate the arguments before passing them on (that's what I do with the KnpPaginatorBundle, for example) \$\endgroup\$ – Elias Van Ootegem Aug 14 '14 at 12:23
0
\$\begingroup\$

Well I would reduce the code by casting the $_GET['page'] as int:

<?php

$_GET['page'] = '01'; // this line is only for testing

$selPage = 1;
$totalPages = 10;

if (isset($_GET['page'])) {
    $selPage = (int)$_GET['page'];
    if ($selPage === 0) {
        exit('redirection'); // this line for test only - in fact make 301 redirection here to correct url
    }
}
echo $selPage;
\$\endgroup\$
  • \$\begingroup\$ Sorry, but it won't work at all. What about 02 or -2? Both those after casting will work and they are not valid int as described in my question. \$\endgroup\$ – Marcin Nabiałek Aug 14 '14 at 17:48

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.