Fisher-Yates shuffle in JavaScript

Question

This is my first time making a function just from reading about it on Wikipedia, so I'm sure there's room for a lot of improvement.

function fisherYates(str) {
 let result = '';
 for (let i = 0, len = str.length; i < len; i++) {
 let rand = (Math.random()*str.length)|0;
 result += str[rand];
 str = str.slice(0, rand) + str.slice(rand+1);
 }
 return result;
}

• Sᴀᴍ Onᴇᴌᴀ: I know it is 10 years old and pertains to arrays instead of strings, but you mind find this post interesting

Answer 1

Mostly just style

Why add the variable len inside the for loop. It is a constant so put it outside the loop as one.

The variable result is in function scope. Use var rather than let.

The variable rand does not change inside the loop so it should be a const.

The expression to get the random number does not need the (...) as operator precedence ensures that the | is applied correctly.

Add some spaces between operators for readability. eg (Math.random()*str.length)|0; is better as (Math.random() * str.length) | 0

One issue

The only problem is that you have one to many iterations. The last random value will always be to select from a 1 character string, so it is not needed. You need only iterate the string length minus one.

The rewrite

How I would rewrite your code. I personally don't like putting the let inside the for loop, so I put the declaration in the scope above. This is because for loops can have a lot of noise (clutter) and that reduces it a bit. Also the function is so short and simple variable names can be shortened without problem, I would make rand just the r.

function fisherYates(str) {
 var i, result = '';
 const len = str.length - 1;
 for (i = 0; i < len; i ++) {
 const r = Math.random() * str.length | 0;
 result += str[r];
 str = str.slice(0, r) + str.slice(r + 1);
 } 
 return result + str;
}

• Tamoghna Chowdhury: I'd go the other way and change rand to randomIndex, but each to his own, I guess.
• Tamoghna Chowdhury: I'd also declare each variable in the smallest lexical scope, using let.
• Blindman67: @TamoghnaChowdhury Block scope has its place but without a clear need its should not be used. You use scope to protect a variable's name, but good function design (no rambling 100+ line functions) eliminates that as an issue (no risk in a 8 line function of using the same variable name by mistake). So i go on the side of efficiency, each scope declaration requires a heap assignment and additional context stack work. In nested loops this can contribute considerable overhead for no good reason but that of "protecting against incompetent programmers"
• Tamoghna Chowdhury: I didn't know that let was that inefficient in JavaScript... Anyway, your comment should IMO be a part of your answer. Especially given that almost every article about ES6+ online is preaching about how var is outdated and let should be used everywhere.
• Blindman67: @TamoghnaChowdhury That is standard in the language since day one (1995). Implementation method is not part of the language spec, V8 uses a "context" object which carries the scope chain. The scope is mutable so I guess it is similar to Python (though I am not familiar with python's inner workings)