Sign up ×

I'm writing a code for a deck of cards which shuffles the deck of cards. I tested the code but I don't really know if it's actually doing what it's supposed to be doing correctly? What do you think?

This is the code for the shuffle method:

public void shuffle()
    for( int x = myDeck.size(); x > 0 ; x--) 
        Random rn = new Random();
        int index1 = rn.nextInt(52);
        Card c = myDeck.remove(index1);

My output seems shuffled in its numbers but not by the name of the card like spades hearts etc,

For example this is my output when I test the code :

Deuce of spades
Seven of spades
Eight of spades
Ace of spades
Three of hearts
Five of hearts
Six of hearts
Seven of hearts
Nine of hearts
Ten of hearts
Queen of hearts
King of hearts
Ace of hearts
Seven of diamonds
Eight of diamonds
Jack of diamonds
King of diamonds
Three of clubs
Seven of clubs
Nine of clubs
Jack of clubs
Queen of clubs
King of clubs
Ace of clubs
Queen of spades
Deuce of clubs
Three of spades
Nine of diamonds
Four of spades
Four of clubs
Deuce of hearts
Jack of spades
Ten of clubs
Six of diamonds
Jack of hearts
Six of clubs
Four of diamonds
Five of diamonds
Ace of diamonds
Four of hearts
Nine of spades
Ten of spades
Five of spades
Three of diamonds
Six of spades
Five of clubs
Deuce of diamonds
Eight of hearts
King of spades
Ten of diamonds
Eight of clubs
Queen of diamonds

Like there's always repeated names. is it wrong since the point of shuffling is to mix it up?

This is the actual question: When playing cards, it is, of course, important to shuffle the deck, that is, to arrange things so that the cards will be dealt in a random order. There are several ways to achieve this. One strategy involves repeatedly picking a card at random out of the deck and moving it to the end. The following code uses the Random class (which you met on page 8 of the “ArrayLists” section of the online course) to perform one such “pick and move to the end” operation:

Random rn = new Random();
int index1 = rn.nextInt( 52 );
Card c = myDeck.remove( index1 );
myDeck.add( c );

To shuffle the deck effectively, this operation should be repeated many times (say, 500 times). Create a new instance method, shuffle, for the Deck class that uses a single Random object and a for loop to shuffle myDeck. After suitably modifying the main method, use it to test your new code.

So my main question is: am I doing this wrong?

share|improve this question
Can you utilize the Collections.shuffle method with the Cards in your Deck? -… –  Dan W Aug 6 '12 at 21:04
One violation is "uses a single Random object". You are using one per iteration of your loop. –  jeff Aug 6 '12 at 21:04
I've added the homework tag based on the "actual question". –  pb2q Aug 6 '12 at 21:04
If you have to use the method specified, the blurb you posted with it says you have to call that method 500 times or so to actually shuffle your deck. Are you actually shuffling that many times, or are you simply calling it once? –  Sean Cogan Aug 6 '12 at 21:04
the point of the exercise is to be able to do what .shuffle does using Random I think –  Panthy Aug 6 '12 at 21:05

4 Answers 4

up vote 16 down vote accepted

Just change rn.nextInt(52); to rn.nextInt(x) and you have a proper Fisher-Yates shuffle. No need to do more than 52 iterations.

Why this works:

  • In the first iteration (when x is 52) you'll select a random card from the full deck and move it last.

  • In the second iteration (when x is 51) you'll select a random card from the remaining cards and move it last.

    ...and so on.

  • After 52 iterations, the first card selected, will have ended up in the first index. Since this card was selected randomly from the full deck, each card is equally probable.

  • Same applies for second index, third index, ...

  • It follows that each possible permutation of the deck is equally probable.

(In production code, just use Collections.shuffle in these situations.)

share|improve this answer
To give a little more, the problem most likely is that you're reinitializing your random number generator each time, which is not how they're usually meant to be used. Declare it once, and the numbers you pull in each iteration will be more random. –  Carl Aug 6 '12 at 21:06
Correct me if I'm wrong, but wouldn't this change make it a Knuth Shuffle? –  Dennis Meng Aug 6 '12 at 21:06
@Carl, that shouldn't matter technically speaking. –  aioobe Aug 6 '12 at 21:07
@aioobe It seems the whole point of this assignement is not to use Collections#shuffle! –  assylias Aug 6 '12 at 21:07
@Rohan Your loop needs to run more than 52 times: for (int x = 0; x < 500; x++) would work better and is what you are being asked. –  assylias Aug 6 '12 at 21:17

The best way to do this would be to use the built in Collections.shuffle() method, which will shuffle your ArrayList for you in a random way (or near enough to random.)

The problem with your logic at the moment is that it's picking out a random card from the deck, and putting it on the end - and doing that 52 times. Now you've got a good change that you'll end up doing this to a number of cards multiple times, and some none at all - hence the problem you're getting with lots of cards that appear to have not been randomised.

You seem to have the logic that you need to do this operation for the number of cards in the deck, which is flawed; you need to perform it many more times.

You've got two main logical solutions, you could firstly do this many more times - say 10 times more than you're doing at present, or you could re-engineer your code to use a built in (or more effective) shuffling algorithm.

share|improve this answer
Doing 520 iterations would A) not be very efficient B) not yield a result where all permutations are equally probable. –  aioobe Aug 6 '12 at 21:50
@aioobe I agree it's not ideal, hence my mention of Collections.shuffle() - but if the OP has to use that method, then using more iterations will be better than not. –  berry120 Aug 6 '12 at 22:58
Not really. 52 iterations is enough. See my answer. –  aioobe Aug 7 '12 at 8:35

The question gives a hint:


Whereas your loop only runs 52 times (myDeck.size()). So you remove a card and replace it randomly 52 times only. That does not seem to be enough.

ps: it is more usual to write for(int i = 0; i < max; i++) than for (int i = max; i >0 i--).

share|improve this answer
I agree, 52 times is definitely not enough. 1+ –  Hovercraft Full Of Eels Aug 6 '12 at 21:06
52 times is enough if you do it right. (Have a look at the Fisher-Yates algorithm for instance.) If you do it wrong, it doesn't really matter how many iterations you do, you'll always have a bias towards certain permutations. –  aioobe Aug 6 '12 at 21:44
@aioobe I don't think I agree - It would be enough if the cards were removed from the deck and added to a new deck - but since they are put back into the deck (as proposed in the assignement), the fact that the random function is (roughly) uniform is not enough as one card can be shuffled more than once. –  assylias Aug 6 '12 at 21:54
@aioobe Your latest edit adresses that issue. –  assylias Aug 6 '12 at 22:08
Simplified my answer though... –  aioobe Aug 6 '12 at 22:14

Change the loop to:

  ArrayList<Integer> myDeck = new ArrayList<Integer>();
   for(int i=0; i< 52; i++){
   Random rn = new Random();    
   for( int x = 52; !myDeck.isEmpty() ; x--) {                
        int index1 = rn.nextInt(myDeck.size());
        //Card c = (Card)myDeck.remove(index1);  -> this comment here should be removed
        System.out.print(index1 + ", ");

This way you won't repeatedly select the same card, plus you'll always pick a number (cell) < myDeck.size() which is constantly changing when you remove cards, AND bail-out when there are no cards in the deck

share|improve this answer
why not change the for to a while? –  aioobe Aug 6 '12 at 21:48
@aioobe that's definitely an option! –  alfasin Aug 6 '12 at 22:25

Your Answer


