javascript - Stuck using array.splice() to shuffle a deck array

Question

I'm trying to randomly shuffle a deck array of Card objects into a newDeck array of Card objects using array.splice(). I imagine my problem is either something to do with variable scope, or a misapprehension about array.splice().

var deck = [new Card(), new Card(), new Card(), new Card(), new Card(), new Card()];
var newDeck = [];
var shuffle = function(){
    var i = "";
    for (i = 0; i < deck.length; i++){      
        newDeck[i] = deck.splice(Math.floor(Math.random() * deck.length, 1));
}    
};

shuffle();

Is there a better way to shuffle a deck?

score 2 · Accepted Answer

Yes, there is a misapprehension about array.splice(). Read the docs: It will return an array of the removed elements, in your case with one card: [<object Card>]. Also, with deck.splice(Math.floor(Math.random() * deck.length, 1)), the 1 (which i guess should be how many cards to remove) is an argument to Math.floor, not splice - you will remove all elements after the index. So, you seem to want:

function shuffle(deck) {
    var newDeck = [];
    for (var i = 0; i < deck.length; i++)    
        newDeck[i] = deck.splice(Math.floor(Math.random() * deck.length), 1)[0];
    return newDeck;
}    
shuffle([new Card(), new Card(), new Card(), new Card(), new Card(), new Card()]);

You have asked for other ways to shuffle: You could use the very common

deck.sort(function(a, b){ return 0.5-Math.random(); })

or the algorithm from underscore.js:

function shuffle(deck) {
    var newDeck = [];
    for (var i = 0; i < deck.length; i++) {    
        var rand = Math.floor(Math.random() * (i + 1));  
        newDeck[i] = newDeck[rand];
        newDeck[rand] = deck[i];
    }
    return newDeck;
}

score 0 · Accepted Answer

Reverse your for cycle:

for (i = deck.length - 1; i >= 0; i--)
    newDeck.push(deck.splice(Math.floor(Math.random() * deck.length))[0]);

The problem you're having is that deck.length decreases every time the cycle executes deck.splice. Alternatively, you can save deck.length in a separate variable before running the script:

var length = deck.length;
for (i = 0; i < length; i++){      
    newDeck[i] = deck.splice(Math.floor(Math.random() * deck.length))[0];

Inside the cycle, though, you have to use deck.length.

Note: Math.floor takes just one argument. What was that ", 1" supposed to be? And initializing i = "" is pointless, since that value is unused. Just put var i;.

Edit: fixed a missing point about the return value of Array.splice.

score 0 · Accepted Answer

You could as well use the Array#shuffle method from underscore.js in case you do not want to reinvent the wheel in that case ;)

javascript - Stuck using array.splice() to shuffle a deck array

3 回答 3

Related

Reference