2
\$\begingroup\$

I have the below function, it is designed so I can call:

$object->temp('one', 1);    //add the key `one` to the temp array with value 1
$object->temp('two', 2);    //add the key `two` to the temp array with value 2
$object->temp('three', 3);  //add the key `three` to the temp array with value 3

$object->temp('three');       //returns 3
$object->temp('three', null); //removes the `three` key
$object->temp();              //returns array('one' => 1, 'two' => 2)

and it works well, my only gripe is that I don't like using the $value = 'read_temp in the definition, it feels like a hack to me.

So, is there anything wrong with that, and how can this function be optimized?

public function temp($key = null, $value = 'read_temp') {
    if($key === null)
        return $this->temp;

    if ($value === null){
        unset($this->temp[$key]);
    }

    elseif ($value == 'read_temp' && isset($this->temp[$key])) {
        $value = $this->temp[$key];
    } else {
        $this->temp[$key] = $value;
    }

    return $value;
}

I could split this into two setTemp getTemp (or three clearTemp to be pedantic) functions, but I would like to be able to have just one function call to do everything.

\$\endgroup\$
2
  • \$\begingroup\$ What is you actual reasoning to not split it into different functions? It would be much more readable to future coders. Depending on your object you could even use overloading php.net/manual/en/language.oop5.overloading.php \$\endgroup\$ – René Oct 6 '12 at 9:16
  • \$\begingroup\$ more or less my reason is that, temp is a temporary storage on that instance of the object, it is a tiny, almost "bootstrap" part of the actual class, so I want to keep it as just one small interface instead of 2/3. \$\endgroup\$ – Hailwood Oct 6 '12 at 9:41
4
\$\begingroup\$

Here's one way to do it

public function temp($key = null , $value = null) {
    $args = func_get_args();
    if( count($args) === 2 ) {
        $this->temp[$key] = $value;
    } elseif( count($args) === 1 ) {
        return @$this->temp[$key];
    } else {
        return $this->temp;
    }
}

func_get_args() will return an array of the arguments that were actually passed - i.e. excluding default values. So if no arguments were passed, return the array; if there's 1 argument (the key), return the corresponding value; and if there are 2 arguments, set the key/value pair.

I'm using the warning-suppression @ instead of explicitly checking isset(). Again, the end result is the same; if you check with isset() and don't (explicitly) return anything for missing keys, the function still returns NULL. If you simply return $this->temp[$key]with no checking, but suppress the "undefined index" warning that might occur, you also get NULL.

I've also skipped the unset() call, since calling $obj->tmp("key", null) does pretty much the same thing. If you request a value that hasn't been set, you get NULL. If you set that value to NULL, you get NULL. If you were to unset the value, you'd also get NULL.
But if you want to keep the unset, you can just check $value === null in the first branch:

if( count($args) === 2 ) {
    if( $value === null ) {
        unset($this->temp[$key]);
    } else {
        $this->temp[$key] = $value;
    }
} // ...

If you skip the unset() part, it'll obviously keep the array keys around, so you won't know for sure if a key whose value is NULL was "deleted", or if it was explicitly set to NULL on purpose.
On the other hand, if you unset() when the passed value is NULL, you won't be able to set a value to NULL on purpose.

The simplest solution is to not use unset in the temp() function, but instead add an extra unset_temp_value($key) function that does the unset. Then you can use that to explicitly remove all traces of a key/value pair, while still being able to store NULLs "on purpose" using temp($key, null).


As mseancole points out below, a switch statement could be used too

public function temp($key = null, $value = null) {
  $args = func_get_args();
  switch( count($args) ) {
    case 2:
      return $this->temp[$key] = $value;
    case 1:
      return @$this->temp[$key];
    default:
      return $this->temp;
  }
}

Only difference is that this returns the value that's set in the first case (just because it's more compact then breaking)

And just for fun, here's the same thing again in Unreadable Mode™

public function temp($key = null, $value = null) {
  $args = count(func_get_args());
  return ($args==2?($this->temp[$key]=$value):($args==1?@$this->temp[$key]:$this->tmp));
}
\$\endgroup\$
9
  • \$\begingroup\$ Cheers, just what I was looking for, but; I loathe the silence operator! so much infact that I have this set on my dev server; ini_set('scream.enabled', true); so I shall be using isset ;) \$\endgroup\$ – Hailwood Oct 6 '12 at 13:35
  • \$\begingroup\$ @Hailwood Heh, fair enough :) \$\endgroup\$ – Flambino Oct 6 '12 at 13:38
  • \$\begingroup\$ @Hailwood: Just a couple of things to add to Flambino's post. 1 Why are your statements inconsistent? You are using braceless and braced code interchangeably. I honestly don't think braceless should be used at all, being as how PHP does not completely support support it, but you should at the very least be consistent. 2 A switch statement might be more appropriate in Flambino's example. \$\endgroup\$ – mseancole Oct 9 '12 at 21:25
  • \$\begingroup\$ @mseancole Good point regarding switch, I'll add it to my answer \$\endgroup\$ – Flambino Oct 9 '12 at 22:16
  • \$\begingroup\$ (Warning: annoying literalness incoming.) @mseancole I understand what you mean, and agree with your sentiments, but "PHP does not completely support [braceless syntax]" is not true. PHP supports it perfectly. Its behavior is well defined, yatta yatta. Ease of misuse and lack of support are not the same thing. \$\endgroup\$ – Corbin Oct 9 '12 at 23:00

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.