5
\$\begingroup\$

The PHP manual states that the regular shuffle() function isn't cryptographically secure, but that random_int() is. I thought, then, that I'd make something to leverage the latter for the former purpose. (PHP 8.2 includes Random\Engine, which includes a shuffler, but I wasn't aware of that when I started thinking about this, and my web server doesn't have 8.2 support yet anyway.)

After a search, I found out about the Fisher-Yates shuffle, and attempted to implement a PHP version based on the pseudocode provided in the Wikipedia article:

function shuffle_rint($a){
// To shuffle an array $a of $n elements (indices 0..$n-1)
$n = count($a);
for ($i = $n - 1; $i >= 1; $i--)
{
    $j = random_int(0,$i);
    $ai = $a[$i];
    $a[$i] = $a[$j];
    $a[$j] = $ai;
}
return $a;
}

Are there any glaring defects here? hrtime seems to suggest that it's a little slower than the regular shuffle, but I expect that is an unavoidable cost of using the secure numbers.

\$\endgroup\$
0

2 Answers 2

4
\$\begingroup\$

Swaps from one index to same index could be avoided

You asked:

Are there any glaring defects here?

As J_H mentioned in their answer

You swap even when $i == $j

The code could check for this condition and in that case avoid swapping values since it would be a waste of operations. The difference in speed would likely be negligible.

Follow common conventions for readability

While there is no de-facto rules about readability conventions and it is up to individuals/teams to decide their conventions, idiomatic PHP code often follows the PHP Standards Recommendations - e.g. PSR-12: Extended Coding Style. The code presented follows many of the recommendations though not all.

Perhaps it was a copy and paste issue but the lines inside the function block should be indented one level. This way anyone reading the function can easily see the code inside the function.

Other things I notice that do not follow the conventions of PSR-12 are:

    $j = random_int(0,$i);

There is no space after the comma separating the arguments in the call to random_int().

for ($i = $n - 1; $i >= 1; $i--)
{

Per PSR-12 Section 5.4 for:

The closing parenthesis and opening brace MUST be placed together on their own line with one space between them.

Idiomatic PHP developers might simply write the middle condition as $i since 0 is considered false

for ($i = $n - 1; $i; $i--) {

Variables can be swapped without a temporary variable

As was mentioned in this review Variables can be swapped without the use of a temporary variable using array destructuring assignment in PHP 7.1+.

Instead of:

$ai = $a[$i];
$a[$i] = $a[$j];
$a[$j] = $ai;

It could be as simple as:

[$a[$i], $a[$j]] = [$a[$j], $a[$i]];
\$\endgroup\$
4
  • \$\begingroup\$ Do not keep calling random_int until the indices are different. You will get a derangement rather than a permutation — the shuffle won’t be as random as it could be. \$\endgroup\$
    – Charles
    Feb 25, 2023 at 16:08
  • \$\begingroup\$ (It’s ok if you want to avoid the unnecessary swap in that case, but it’s probably not any faster.) \$\endgroup\$
    – Charles
    Feb 25, 2023 at 16:12
  • \$\begingroup\$ Good call - I’ve updated my answer \$\endgroup\$ Feb 25, 2023 at 19:20
  • \$\begingroup\$ You got my +1, then! \$\endgroup\$
    – Charles
    Feb 26, 2023 at 19:22
4
\$\begingroup\$

The stated goal is to leverage random_int() to create a cryptographically secure shuffle function, but I don't see any hint of that in the code artifact. Not in the identifiers and not in comments or DocBlock. We don't see any comparisons with behavior of the old shuffle().

Given a deck of three cards, it's not hard to produce 3! permutations. What your function neglects to do is offer the caller advice on what fraction of permutations it is able to produce when given a larger deck of, say, ~ two thousand cards.


The "of $n elements (indices 0..$n-1)" part of the comment is maybe a little weird, as it is implied by $a, the input arg that the caller cares about. Consider trimming the comment, and elevating it to a /** DocBlock */


There's an unconditional swap at the end of the sequence. You swap even when $i == $j. And that's OK.


This submission contains no unit tests. Sigh!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.