1
\$\begingroup\$

This is a cut-and-dried Fisher-Yates implementation. As not typically a JS developer, I've tried to incorporate as many best practices as I can. I would appreciate any input as to make it conform to more best practices and continue to be maintainable.

(function fisherYates () {

    "use strict";

    var shuffle = function shuffle (array) {

        var beginningIndex = 0,
            currentIndex = array.length,
            indexDecrement = 1,
            randomIndex = 0,
            temporaryValue = 0;

        // While elements remain to be shuffled...
        while (currentIndex !== beginningIndex) {

            // Pick an element from the remaining elements...
            randomIndex = Math.floor(Math.random() * currentIndex);
            currentIndex -= indexDecrement;

            // Swap it with the current element.
            temporaryValue = array[currentIndex];
            array[currentIndex] = array[randomIndex];
            array[randomIndex] = temporaryValue;

        }

        return array;

    };

    return shuffle;

}());
\$\endgroup\$
  • 1
    \$\begingroup\$ Minor point - having indexDecrement as a variable makes it look like it could change. IMO, "currentIndex--" is clear enough on its own, but if you did want it named it should be const. \$\endgroup\$ – Errorsatz Sep 21 '18 at 23:08
  • 1
    \$\begingroup\$ @Errorsatz I agree - was just trying to shush the linter :) \$\endgroup\$ – Jesse C. Slicer Sep 21 '18 at 23:19
3
\$\begingroup\$
  • The docs on "use strict" are vague but I'm not entirely sure putting it inside a function makes it apply to the function definition.

  • var shuffle = function shuffle (array) { seems unnecessarily redundant. I would just use function shuffle (array) {

  • Use const and let instead of var (unless you're trying to support older browsers)

  • It doesn't make sense to initialize randomIndex or temporaryValue.

  • I would write while (currentIndex > beginningIndex) { especially if the indexDecrement could be something other than 1. Actually I would prefer a for loop though its arguable given the way your loop is structured.

  • I would use destructuring assignment to swap values:

    [ array[currentIndex], array[randomIndex] ] = [ array[randomIndex], array[currentIndex] ];
    
  • If you use const or let you can move the definition of randomIndex and temporaryValue inside the loop block.

  • You return the shuffle function but your IIFE doesn't assign it anywhere. Apart from the fact that this seems unnecessarily complex I wonder if this even works?

\$\endgroup\$
0
\$\begingroup\$

I have incorporated most of Marc Rohloff's points (with the notable exception of the last -- I am currently letting Will It Shuffle? work with the result of the IIFE), while minimizing hacking off the linter and have come up with this:

/* eslint-env es6 */
/* eslint no-magic-numbers: ["error", { "ignore": [1] }] */
/* eslint one-var: ["error", "consecutive"] */
(function fisherYates () {

    const shuffle = function shuffle (array) {

        let currentIndex = array.length;

        // While there remain elements to shuffle...
        while (currentIndex) {

            // Pick a remaining element...
            const randomIndex = Math.floor(Math.random() * currentIndex);

            currentIndex -= 1;

            // And swap it with the current element.
            [
                array[currentIndex],
                array[randomIndex]
            ] = [
                array[randomIndex],
                array[currentIndex]
            ];

        }

        return array;

    };

    return shuffle;

}());
\$\endgroup\$

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.