I'm trying to follow the DRY programming philosophy and I am tired of running isset()
and an if not empty
statements for each time I want to check a $_GET[]
variable.
I am working on a page that receives up to 4-5 GET
parameters in the URL, so I wrote the following function to reduce repetitive code:
function getset( $get, $default, $empty=FALSE ) {
// checks if a GET parameter is set
if ( isset($_GET[$get]) ) {
// optionally allows empty values
if ($empty) { return $_GET[$get]; }
// check if value is not empty
else {
if ( $_GET[$get] != '' ) { return $_GET[$get]; }
// otherwise return default value
else { return $default; }
}
}
// if param not set then return default value
else { return $default; }
}
// set the variables
$foo = getset('foo', 'abc');
$bar = getset('bar', 'xyz', TRUE)
// localhost/?foo => returns 'abc'
// localhost/?foo=x => returns 'x'
// localhost/?bar => returns TRUE / ''
I am wondering if my function is proper, efficient and safe compared to the alternative:
// default values
$foo = 'abc';
$bar = '';
// set $foo from $_GET value
if ( isset($_GET['foo']) && $_GET['foo'] != '' ) {
$foo = $_GET['foo'];
}
// set $bar from $_GET value
if ( isset($_GET['bar']) ) {
$bar = $_GET['bar'];
}
I am open to suggestions for improving/refining my code.
1 Answer 1
If your aim is to go DRY, I would consider taking it one step further. You are writing a function for a common use case indeed, but one could wonder if this is only used for $_GET variables. What about $_POST or $_SESSION or some none magic array your application uses...
I'm used to working with Laravel, and this framework offers a very basic support method called array_get
(docs here). I use it so often, it should really be part of core php.
Allow me to rewrite your function to this API. (Note that I'll leave .
syntax for nested arrays out, though it has proven useful to me it would take us to far.)
function array_get($array, $key, $default = null) {
if (! array_key_exists($key, $array)) {
return $default;
}
if (is_null($array[$key])) {
return $default;
}
return $array[$key];
}
// usage
$get = array_get($_GET, 'myKey', 'default');
$post = array_get($_POST, 'myKey', 'default');
$session = array_get($_SESION, 'myKey', 'default');
$value = array_get($myArray, 'myKey', 'default');
I'm not sure about you $empty
parameter. It doesn't feel entirely right to me to have it in there, but I may be wrong.
If you decide to stick with your function, try to clean it up a bit.
- I feel the comments are obsolete when you are performing such trivial, perfectly readable tasks.
- All the
else
statements are obsolete, they just clutter the code. - Nesting control structures is rarely a good idea. It makes the code harder to read. You'll be surprised how rarely you have to nest control structures if you take this rule into account. If you have to, you are probably doing something else wrong (single responsibility? extract another function perhaps)
- Nice job on the early returns!
Have a look at this:
function getset( $get, $default, $allowEmpty=FALSE ) {
if (! isset($_GET[$get]) ) {
return $default;
}
if ($allowEmpty) {
return $_GET[$get];
}
if ( $_GET[$get] === '' ) {
return $default;
}
return $_GET[$get];
}
update:
Concerning your question about the $allowEmpty, I think you are approaching the problem from the wrong angle. Either you want a variable to have a value, or you want to default a missing variable.
// default
$foo = array_get($_GET, 'foo', 'default');
// required
$bar = array_get($_GET, 'bar');
if (is_null($bar)) {
// required var missing
}
I feel cramming that $allowEmpty
into the function violates he SRP somewhat. You are not just getting a value from an array anymore, you are validating it as well.
Do feel free to add it however, it is your code after all. It should be as simple as adding a fourth parameter to the array_get
function and checking it similar to the way I provided in the refactored getset
function.
-
\$\begingroup\$ This is great. Your code is missing a
)
atif (is_null($array[$key]) {
. Also what if I want to add$allowEmpty
toarray_get()
? I don't wanturl?foo
to returntrue
. \$\endgroup\$Aziz– Aziz2016年05月03日 21:53:31 +00:00Commented May 3, 2016 at 21:53 -
\$\begingroup\$ @aziz fixed the bug and added an update in an attempt to answer your question \$\endgroup\$Pevara– Pevara2016年05月03日 22:07:22 +00:00Commented May 3, 2016 at 22:07
-
\$\begingroup\$ I think I got a bit confused there, what I really meant to say is that
$foo = array_get($_GET, 'foo', NULL);
should returnNULL
($default
) when it's empty likeurl?foo
but instead returns""
empty string \$\endgroup\$Aziz– Aziz2016年05月03日 22:13:18 +00:00Commented May 3, 2016 at 22:13 -
\$\begingroup\$ I suppose I may have overcomplicated this. I'll just use
if ($foo === "")
in case I wanted to allow an empty string instead ofif($foo)
. Thanks for your help! \$\endgroup\$Aziz– Aziz2016年05月03日 22:36:49 +00:00Commented May 3, 2016 at 22:36
empty()
? \$\endgroup\$!= ''
instead. What would it change? \$\endgroup\$empty
may give unexpected results with ie."0"
as a value. It is a function that has to be used with care... \$\endgroup\$"0"
to returnempty
/NULL
. \$\endgroup\$