Fisher-Yates Shuffle

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;

}());

• 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. Sep 21 '18 at 23:08
• @Errorsatz I agree - was just trying to shush the linter :) Sep 21 '18 at 23:19

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

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;

}());