2
\$\begingroup\$

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.

\$\endgroup\$
7
  • \$\begingroup\$ Ever heard of empty()? \$\endgroup\$
    – Christian
    May 3 '16 at 19:38
  • \$\begingroup\$ @Christian yes, I've used != '' instead. What would it change? \$\endgroup\$
    – Aziz
    May 3 '16 at 19:47
  • 1
    \$\begingroup\$ @Christian in this case empty may give unexpected results with ie. "0" as a value. It is a function that has to be used with care... \$\endgroup\$
    – Pevara
    May 3 '16 at 20:27
  • \$\begingroup\$ @Pevara that is true. I don't want "0" to return empty/NULL. \$\endgroup\$
    – Aziz
    May 3 '16 at 20:30
  • 1
    \$\begingroup\$ Point taken. But beware of the == and != operators, type juggling as php calls it is not (usually) a good thing.. \$\endgroup\$
    – Christian
    May 3 '16 at 21:46
1
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ This is great. Your code is missing a ) at if (is_null($array[$key]) {. Also what if I want to add $allowEmpty to array_get()? I don't want url?foo to return true. \$\endgroup\$
    – Aziz
    May 3 '16 at 21:53
  • \$\begingroup\$ @aziz fixed the bug and added an update in an attempt to answer your question \$\endgroup\$
    – Pevara
    May 3 '16 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 return NULL ($default) when it's empty like url?foo but instead returns "" empty string \$\endgroup\$
    – Aziz
    May 3 '16 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 of if($foo). Thanks for your help! \$\endgroup\$
    – Aziz
    May 3 '16 at 22:36

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.