PHP Random List

Question

Gentlemen!

So what I have here is a function who's main purpose is to check whether an attribute exists and if so add it to another list for a maximum of 3 entries. The first item in the list should always be checked while the rest of the list should be inserted into the new list randomly.

ex: Old List: 0, 1, 2, 3, 4 New List: 0, 3, 1

Code follows.

private function getList(&$oldList, $type = 'atrb1')
{
    $list = array();

    if(isset($oldList->item[0]->attribute))
    {
        if($oldList->item[0]->attribute->type == $type)
        {
            $list[] = 0;
        }
    }

    $keys = array_keys($oldList->items);
    shuffle($keys);

    for($i = 0; $i < count($keys); $i++)
    {
        if(isset($oldList->item[$keys[$i]]->attribute) && $keys[$i] != 0 )
        {
            if($oldList->item[$keys[$i]]->attribute->type == $type)
            {
                if(count($list) == 3)
                {
                    break;
                }
                else
                {
                    $list[count($list)] = $keys[$i];
                }                
            }
        }
    }

    return $list;
}

I appreciate the review, I have some more but they take time to sanitize :)

Answer 1

This method seems... odd... Why would you need to do this? Anyways, unimportant... Like I mentioned in the other review, the referenced parameters are unnecessary when you could more easily use, and put better use to, properties.

Also, as I mentioned in that other review, there is room for adding more methods here. I didn't give an example in the last review, but I'll give a hint here. That first if statement would make a lovely new addition. Try to think about how that could be reused, and is used currently. My examples below and in the previous review should help you to figure this out.

Don't call functions in the parameter list for for or while loops. They are called on each iteration of the loop, meaning your program will become less efficient. Perform the count before the loop and set it as a variable, then pass that variable into the loop instead.

$count = count( $keys );
for( $i = 0; $i < $count; $i++ ) {

As I mentioned in the last review, don't violate DRY, or the Arrow anti pattern.

$item = $oldList->item[ $keys[ $i ] ];
if( ! isset( $item->attribute ) || $keys[ $i ] == 0 ) {
    continue;
}

To help with the Arrow anti pattern, you should add the following statement before the one mentioned above the to ensure no unnecessary processing is done.

if( count( $list ) == 3 ) {
    break;
}

Of course, the above would become unnecessary if you limited your loop or array to the proper size initially. Limiting the loop might be impossible in this context, but I mention it for completeness. The following is about to get a little complicated and may not even be "better" (I didn't test or profile it). So, you can follow along if you want, but you don't need to use it, I'm just demonstrating. BTW: this requires a relatively new version of PHP (5.3 I believe).

//remove all unwanted keys
$keys = array_filter( $keys, function( $val ) use( $keys, $oldList, $type ) {
    //remove zero as you appear not to want it
    if( $val == 0 ) {
        return FALSE;
    }

    //remove elements without the proper attribute
    $item = $oldList->item[ key( $keys ) ];
    return isset( $item->attribute ) && $item->attribute->type == $type;
} );

shuffle( $keys );

//get max size, adjusted for initial addition
$max = 3 - count( $list );

//get correctly sized portion
$keys = array_slice( $keys, 0, $max );

//loop unnecessary, merge arrays
$list = array_merge( $list, $keys );

Again, as I don't have 5.3 on this machine, the above example is untested and has not been profiled. Mostly I did this because I enjoy the mind stretch. If you understand what is going on here, find it easier to read, can reproduce it on your own, and it is more efficient, go ahead and use it; if not, you might want to stick to the loops for now. I hope this helps!