Do you consider foreach((array)$foo as $bar) a code smell?

Question

Do you consider this a code smell?

foreach((array)$foo as $bar)
{
    $bar->doStuff();
}

Should i use that instead?

if (isset($foo) && is_array($foo))
{
    foreach($foo as $bar)
    {
        $bar->doStuff();
    }
}

Any other good practices to cover not set variables and assert an array?

Answer 1

Personally, I would never do the first method and always opt for the second.

Answer 2

If you are testing if variables are set, you can initialize them:

if (! $foo or !is_array($foo))
    $foo = array();

foreach($foo as $bar)
{
    $bar->doStuff();
}

Answer 3

They're both code smells. The second one is just evading all the error messages, kind of like turning off the fire alarm before you set your kitchen on fire. Both of those tell you that you have no idea what's in the variable $foo or if it's even been defined in the code above. You need to go back up through the code and find out exactly what's going on with $foo.

If it was my code, $foo would probably be always defined either as an array, or else false to indicate the array isn't needed:

if(do_we_need_an_array())
  $foo = function_returning_an_array();
else
  $foo = false;

[...snip...]

if($foo)
  foreach($foo as $f) { ... }

Answer 4

If $foo should always be an array, then the second form would be much better if you did some kind of handling for the error case, e.g.:

if (isset($foo) && is_array($foo))
{
    foreach($foo as $bar)
    {
        $bar->doStuff();
    }
} 
else
{
    // This should not happen, exit angrily.
    exit("Oh crap, foo isn't an array!");
}

Of course you don't have to just exit the application, but do whatever is appropriate in that case, maybe logging or some alternate logic.

Answer 5

(array)$foo != if (isset($foo) && is_array($foo))

The (array) cast can be useful for casting objects to arrays or scalars to arrays so you can create consistent interfaces to variables that may contain single values or arrays.

(array)$foo == array($foo)

As defined in the PHP Manual for Array Types.

So if you need to always use an array then the first code snippet you presented would be the answer. However type casting rules still apply so you may not get what you want, so look to the manual for more info. Otherwise the second option would prevent accessing unset variables that are not arrays.

As far as a code smell, I would say that checking for unset variables can certainly be avoided, however always knowing that a variable is going to have an array is more often than not, going to creep up. So I would aim to keep code wrapped in is_array($foo) if-then statements to a minimum.

Answer 6

I usually do this to make sure a foreach can handle both scalars and collections:

<?php

foreach (makeSureTraversable($scalarOrCollection) as $val)
{
    // Do something.
}

function
makeSureTraversable($anything)
{
    if (is_array($anything) || ($anything instanceof Traversable))
    {
     return $anything;
    }
    else
    {
     return array($anything);
    }
}

This way I also handle classes that implement Traversable (from the SPL), which means allowing them to be used in foreaches.

Answer 7

if (!isset($foo) && !is_array($foo)) {
    throw new InvalidArgumentException('Wrong array passed'); 
    // Or do something to recover lost array
}
foreach($foo as $bar) {
    $bar->doStuff();
}

Answer 8

There's quite a few times that you'd like to write a function to take one or more values for a parameter:

function getNamesById($id) { }

In this case, it would make sense that if this function was called with an array of ids, it should probably return an array of names. Similarly, to save the calling code from having to wrap the input in an array and then unwrap the output, if you just pass a scalar, then a scalar should be returned. Consider the likely contents of the function designed to handle both scalar and array parameters:

function getNamesById($id) {
    $returnAnArray = is_array($id);
    $output = array();
    foreach ((array)$id as $theId) {
        // perform some logic
        $output[] = someFunction($theId);
    }
    return $returnAnArray ? $output : $output[0];
}

You can see that in this case, casting to an array definitely makes things a lot easier for everyone. As they say, be liberal in what you accept... As long as it is documented that it is expected that a variable could be either, then I see no problem. PHP is a duck-typed language, which has both benefits and drawbacks, but this is one of the benefits, so enjoy it!