HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Display four random entries from an array

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
randomfromarrayfourdisplayentries

Problem

This code will take the array shown and display 4 random entries in the format shown (or near enough).

Are there any parts of this that can be optimised? I'm not happy about the if statement.

= 3){
      break;
    }
    $i++;
  }
  return $random; 
} 

$arr = array();
$arr[] = array('img' => "http://placehold.it/150x150", 'link' => '#1', 'text1' => "ONE 1", 'text2' => 'ONE 2');
$arr[] = array('img' => "http://placehold.it/200x150", 'link' => '#2', 'text1' => "TWO 1", 'text2' => 'TWO 2');
$arr[] = array('img' => "http://placehold.it/250x150", 'link' => '#3', 'text1' => "THREE 1", 'text2' => 'THREE 2');
$arr[] = array('img' => "http://placehold.it/300x150", 'link' => '#4', 'text1' => "FOUR 1", 'text2' => 'FOUR 2');
$arr[] = array('img' => "http://placehold.it/350x150", 'link' => '#5', 'text1' => "FIVE 1", 'text2' => 'FIVE 2');
$arr[] = array('img' => "http://placehold.it/400x150", 'link' => '#6', 'text1' => "SIX 1", 'text2' => 'SIX 2');
$arr[] = array('img' => "http://placehold.it/450x150", 'link' => '#7', 'text1' => "SEVEN 1", 'text2' => 'SEVEN 2');

foreach(shuffle_assoc($arr) as $key => $value)
{
    echo '';
    echo ''.$value["link"].'';
    echo ''.$value["text1"].'';
    echo ''.$value["text2"].'';
    echo '';
}
?>

Solution

I think the function is not named properly. You are NOT shuffling an associative array. You are shuffling a numerically indexed array (whose values happen to be associative arrays).

As such, I don't see a great need for this function. You could use array_rand() to pick random keys from the array.

Now, assuming you did want to keep such a function, for example to actually get array of values not just keys. I would consider a few things:

  • Consider throwing an exception or logging error if function is passed a non-array value. This can help make sure you are invoking this function properly in your code vs. just returning the passed value unchanged.



  • Consider passing "limit" value to the function vs. hard-coding for 4 return elements.



This might yield something like:

function array_rand_values($arr, $limit = 1) {
    if(!is_array($arr)) {
        throw new InvalidArgumentException('Array expected for first argument.');
    }
    if(!is_int($limit) || $limit < 1) {
        throw new InvalidArgumentException(
            'Positive integer value expected for second argument.'
        );
    }
    if(count($arr) <= $limit) {
       shuffle($arr);
       return $arr;
    }
    $keys = array_rand($arr, $limit);
    $values = array();
    for ($i = 0; $i < $limit; $i++) {
        $values[] = $arr[$keys[$i]];
    }
    return $values;
}


Note that this function does not preserve keys if passed an associative array. This should probably be the logical behavior in such a case, as "shuffling" an associative array typically makes zero sense. This would however still pick X random values from an associative array.

If one needed to pick X random key/value pairs from an associative array, I would suggest a separate function, so that intent of caller is clear. That function may be implemented in very similar manner to the one above, with single line of code change.

function array_rand_values_assoc($arr, $limit = 1) {
    // same code as above until assignment in loop

    for ($i = 0; $i < $limit; $i++) {
        $values[$keys[$i]] = $arr[$keys[$i]];
    }
    return $values;
}

Code Snippets

function array_rand_values($arr, $limit = 1) {
    if(!is_array($arr)) {
        throw new InvalidArgumentException('Array expected for first argument.');
    }
    if(!is_int($limit) || $limit < 1) {
        throw new InvalidArgumentException(
            'Positive integer value expected for second argument.'
        );
    }
    if(count($arr) <= $limit) {
       shuffle($arr);
       return $arr;
    }
    $keys = array_rand($arr, $limit);
    $values = array();
    for ($i = 0; $i < $limit; $i++) {
        $values[] = $arr[$keys[$i]];
    }
    return $values;
}
function array_rand_values_assoc($arr, $limit = 1) {
    // same code as above until assignment in loop

    for ($i = 0; $i < $limit; $i++) {
        $values[$keys[$i]] = $arr[$keys[$i]];
    }
    return $values;
}

Context

StackExchange Code Review Q#153494, answer score: 2

Revisions (0)

No revisions yet.