4
\$\begingroup\$

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.

<?php
function shuffle_assoc($list) { 
  if (!is_array($list)) return $list; 

  $keys = array_keys($list); 
  shuffle($keys); 
  $random = array(); 
  $i = 0;
  foreach ($keys as $key) { 
    $random[$key] = $list[$key];
    if ($i >= 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 '<p><img src="'.$value["img"].'"></p>';
    echo '<p>'.$value["link"].'</p>';
    echo '<p>'.$value["text1"].'</p>';
    echo '<p>'.$value["text2"].'</p>';
    echo '<hr>';
}
?>
\$\endgroup\$
2
\$\begingroup\$

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;
}
\$\endgroup\$
2
\$\begingroup\$

If you are just shuffling and truncating the array, I don't see much point in generating an array of keys, calling a loop with incrementing and a conditional break. In your function, I recommend just three lines maximum:

  • Check that $list is an array, if not, return an empty array not the non-array value. Returning an empty array means foreach() will not iterate it and not choke on. A warning message of your choice when $list is not an array would be a good touch, of course.
  • Shuffle $list.
  • Truncate $list using array_slice() to its first four elements and return it.

Code: (Demo)

function shuffle_assoc($list){ 
  if(!is_array($list)) return []; 
  shuffle($list); 
  return array_slice($list,0,4);
} 

$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 $value){
    echo "<p><img src=\"{$value['img']}\"></p>";
    echo "<p>{$value['link']}</p>";
    echo "<p>{$value['text1']}</p>";
    echo "<p>{$value['text2']}</p>";
    echo '<hr>';
}

p.s. My preference about outputting variables with strings is to use double quotes and curly braces around variables instead of toggling between strings and variables using concatenation. If you are going use concatenation to output your data, commas are faster than dots.


p.p.s. If you would like to compact your code (at a cost to maintainability), you could eliminate the custom function call and implode the random selections to display.

Code: (Demo)

foreach (array_rand($arr, 3) as $index) {
    $row = $arr[$index];
    $row['img'] = "<img src=\"{$row['img']}\">";
    echo '<p>' , implode('</p><p>', $row) , '</p><hr>';
}
\$\endgroup\$
0
\$\begingroup\$

Feedback

As was pointed out in other answers, the (outer) array isn't really associative- it is numeric, and the sub-arrays are associative. And I agree with the point in mickmackusa's answer - just use shuffle() along with array_slice() to simplify that function.

The foreach within shuffle_assoc() iterates over the keys, which will be numerically-indexed and thus it is pointless to create $i and increment it manually. It can simply use the associative syntax (i.e. foreach ($keys as $i => $key)).

Suggestion

Simplifying array generation

Perhaps the code used to add values to $arr is synthesized for this post, but it could be simplified, using a foreach or even array_map, combined with range() to iterate 7 times, plus NumberFormatter::format() to generate the spelled out numbers:

$nf = new NumberFormatter('en', NumberFormatter::SPELLOUT);
$arr = array_map(function($index) use($nf) {
    $word = strtoupper($nf->format($index));
    $dim = 100 + 50 * $index;
    return array('img' => "http://placehold.it/{$dim}x150", 'link' => '#'.$index, 'text1' => $word . " 1", 'text2' => $word . ' 2');
}, range(1,7));

See a demonstration of the updated code in this playground example

\$\endgroup\$
  • \$\begingroup\$ Yeah - you weren't going for the unsung badge, were you? :) Fiddlesticks! I should have read through your answer more thoroughly. I removed the redundant advice. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ May 31 '18 at 22:48

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.