patternphpMinor
Setting the page number with a default
Viewed 0 times
numberthewithsettingdefaultpage
Problem
Consider this code segment:
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:
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
That said, you should be doing something like the following:
or
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:
Now we safely default to
Of course, we still have another bug: what happens if
And finally we'd extract that to a method:
Easy to use in the future as well.
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.