HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Setting the page number with a default

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
numberthewithsettingdefaultpage

Problem

Consider this code segment:

if (empty($_GET)) {
    $page_number = 1;
} else {
    $page_number = $_GET["page_number"];
}


I wonder if there is a better, more succinct way to express that idea. I thought about using a ternary operator, but the improvement over the original is not substantial:

$page_number = empty($_GET) ? 1 : $_GET["page_number"];

Solution

You have a surprise bug: what happens when I do page.php?Name=EBrown? Well $_GET is not empty but your page_number sure isn't set.

That said, you should be doing something like the following:

$page_number = 1;

if (isset($_GET["page_number"})) {
    $page_number = $_GET["page_number"];
}


or

$page_number = $_GET["page_number"];

if (!isset($page_number)) {
    $page_number = 1;
}


The first defaults and then checks for the parameter, the second checks if it's set then defaults. Take your pick.

But those still don't handle invalid (non-numeric) input, so we can do that as well:

$page_number = $_GET["page_number"];

if (isset($page_number) === FALSE || is_numeric($page_number) === FALSE) {
    $page_number = 1;
}


Now we safely default to 1 when page_number is not set, or when it's not numeric.

Of course, we still have another bug: what happens if page_number is 1.12? That's probably unintended. So, we'll check for that as well:

$page_number = $_GET["page_number"];

if (isset($page_number) === FALSE || strpos($page_number, '.') !== FALSE || is_numeric($page_number) === FALSE) {
    $page_number = 1;
}


And finally we'd extract that to a method:

function defaultInt($intValue, $default) {
    if (isset($intValue) === FALSE
        || strpos($intValue, '.') !== FALSE
        || is_numeric($intValue) === FALSE) {
        return $default;
    }

    return $intValue;
}

$page_number = defaultInt($_GET["page_number"], 1);


Easy to use in the future as well.

Code Snippets

$page_number = 1;

if (isset($_GET["page_number"})) {
    $page_number = $_GET["page_number"];
}
$page_number = $_GET["page_number"];

if (!isset($page_number)) {
    $page_number = 1;
}
$page_number = $_GET["page_number"];

if (isset($page_number) === FALSE || is_numeric($page_number) === FALSE) {
    $page_number = 1;
}
$page_number = $_GET["page_number"];

if (isset($page_number) === FALSE || strpos($page_number, '.') !== FALSE || is_numeric($page_number) === FALSE) {
    $page_number = 1;
}
function defaultInt($intValue, $default) {
    if (isset($intValue) === FALSE
        || strpos($intValue, '.') !== FALSE
        || is_numeric($intValue) === FALSE) {
        return $default;
    }

    return $intValue;
}

$page_number = defaultInt($_GET["page_number"], 1);

Context

StackExchange Code Review Q#156265, answer score: 6

Revisions (0)

No revisions yet.