Is my method working effectively?

Question

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);
        myDeck.add(c);
     }
  }

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?

Answer 1

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.)

Answer 2

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.

Answer 3

The question gives a hint:

To shuffle the deck effectively, this operation should be repeated many times (say, 500 times).

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--).

Answer 4

Change the loop to:

  ArrayList<Integer> myDeck = new ArrayList<Integer>();
   for(int i=0; i< 52; i++){
       myDeck.add(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