2
\$\begingroup\$

How to improve this kind of redundancy?

$nid = isset( $_GET['nid'] ) ? $_GET['nid'] : 0;

I want to avoid to repeat $_GET['nid']

Example of useful use case:

$nid = isset( $node->field_mymodule_extended_nid['und'][0]['target_id'] ) ? $node->field_mymodule_extended_nid['und'][0]['target_id'] : 0;
\$\endgroup\$
  • 1
    \$\begingroup\$ Couldn't you just create a helper function for that? \$\endgroup\$ – svick Sep 13 '13 at 9:12
  • \$\begingroup\$ yes but it would be so handy directly \$\endgroup\$ – pico34 Sep 13 '13 at 11:38
3
\$\begingroup\$

By reading the manual:

Since PHP 5.3, it is possible to leave out the middle part of the ternary operator. Expression expr1 ?: expr3 returns expr1 if expr1 evaluates to TRUE, and expr3 otherwise.

So in short:

$nid = $_GET['nid'] ?: 0;

Would work, but PHP will trigger a notice if $_GET['nid'] is not set. The logic || operator won't help, either. The closest you can get is:

(($nid = $_GET['nid']) || ($nid = o));

But that's not exactly shorter, nor does it solve the Notice issue.
The "best" as in shortest, moste un-maintainable and horrible looking code I can think of is this:

$nid = 0;//default
foreach($_GET as $name => $val)
{
    $$name = $val;
}
var_dump($nid);

This turns all keys that were present in the $_GET array into var names. If you simply declare the vars you're after (names must be identical to keys, though) and assign them a default value, this loop takes care of the rest.
To map certain keys to another variable name, you could use another array:

$nid = 0;
$map = array('new_id' => 'nid');
foreach($_GET as $k => $v)
{
    if (isset($map[$k]))
    {
        $k = $map[$k];
    }
    $$k = $v;
}

But honestly, maintaining code like this is going to be a nightmare. Though request objects add a lot of overhead for a task as simple as this, their get methods make it all worth while:

public function get($name, $default = null)
{
    if (!isset($_GET[$name]))
    {
        return $default;
    }
    return $_GET[$name];
}

This is easy to read/understand and easy to use:

$nid = $getInstance->get('nid', 0);

Assignes the GET param, or 0 to $nid... which makes your code tidier, easier to maintain and a lot easier to understand. Based on the use-case you posted, I take it you're not using the ternary on just request variables. Well, in that case: use objects with getters/setters that define a default return value (or allow for one to be specified @call-time). If needs must, implement the ArrayAccess or some Traversable interface, so you can use it as an array, too.
Granted, it'll take some time/effort to do this, but once you've done that, any new code you write will be cleaner (no ternaries), debugging will be a doddle and type-hinting will become your best friend. check this answer for some examples and considerations on getters and setters, along with this answer on the importance of pre-declaring properties (and thus implementing getters/setters).

If you're still up for it after that, refer to this answer to see why and how you should avoid object overloading (in PHP, at least) as much as possible.

\$\endgroup\$
  • \$\begingroup\$ @pico34: Made quite a few edits, and please, don't be ungreatful... your comment isn't goign to make me more keen on getting back to your question, as it is rather blunt. You can point out oversights/omissions without exclamation marks or "come on"'s. Just like I can (and indeed did) answer your question politely, instead of opening with "Mais putain, lisez les documents sur PHP.net:" \$\endgroup\$ – Elias Van Ootegem Sep 13 '13 at 14:36
  • \$\begingroup\$ @pico34: Not sure if I understand you last comment, but the drupal variable_get function is just another prime example of terrible code (and there's quite a bit of bad code in drupal). It uses a global $config variable, for starters... don't, just create your own object, it'll be far more performant \$\endgroup\$ – Elias Van Ootegem Sep 15 '13 at 16:30
2
\$\begingroup\$

This snippet of code is from Kohanaphp framework.

I have found this the easiest and most flexible way to do what you want. It's not a lot less code then what you have, but at least you are not repeating the array key part.

The name of the function get() isn't my preferred naming as I feel it is confused with $_GET, but it is part of the framework, so I have left as is. My preference would be to call it val() (a bit of jquery influence there)

It is in it's own class here, but that is not essential.

class Arr {
    public static function get($array, $key, $default = NULL)
    {
        return isset($array[$key]) ? $array[$key] : $default;
    }
}

// Use like this
$nid = Arr::get($_GET, 'nid', 0);

// or assume null by default
$nid = Arr::get($_GET, 'nid');

// or check for a post value
$nid = Arr::get($_POST, 'nid');

// or as per your example
// although multidimensional arrays would get an error notice if ['und'][0] doesn't exist
$nid = Arr::get($node->field_mymodule_extended_nid['und'][0], 'target_id', 0);
\$\endgroup\$
  • \$\begingroup\$ Jesus... this snippet shows just how much the static keyword is being abused... the function here is just a global in drag. That's not what statics are for. If you need a global function, use a global function. If you need an object, use an object... I know you didn't write it, but IMO, it's bad code \$\endgroup\$ – Elias Van Ootegem Sep 15 '13 at 16:28

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.