Fisher-Yates Shuffle

Question

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;

}());

Answer 1

• 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?

Answer 2

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;

}());