Is my method effective?

Question

I am writing code for a deck of cards that shuffle a deck of cards. I tested the code, but I really don’t know if it really does what it should do right? 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 result seems to be shuffled in its numbers, but not by the name of the card, like hearts with peaks, etc.,

For example, this is my conclusion when I check 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 

As always duplicate names. this is wrong, since the shuffle point is to mix it?

This is a topical issue: when playing cards, of course, it is important to shuffle the deck, that is, arrange things so that the cards are dealt in random order. There are several ways to achieve this. One strategy involves repeatedly selecting a card randomly from 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 select and move to the end operation:

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

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

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

Answer 1

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

Why does it work:

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

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

  ... etc.

• After 52 iterations, the first selected map will go to the first index. Since this card was randomly selected from the full deck, each card is equally likely.

• The same goes for the second index, the third index, ...

• It follows that every possible permutation of the deck is equally likely.

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

Answer 2

The best way to do this is to use the built-in Collections.shuffle() method, which will randomly move your ArrayList for you (or close enough to random.)

The problem with your logic at the moment is that it selects a random card from the deck and puts it at the end - and does it 52 times. Now you have some good changes that you end up doing this with several cards several times, and some do not - hence the problem that you get with a lot of cards that do not appear to have been randomized.

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

You have two main logical decisions: first you can do it many times - say, 10 times more than now, or you can rebuild your code to use the built-in (or more efficient) one.

Answer 3

The question gives a hint:

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

While your loop only runs 52 times ( myDeck.size() ). Thus, you delete the card and arbitrarily replace it only 52 times. This does not seem to be enough.

ps: usually 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 + ", "); } } 

Thus, you will not re-select the same card, plus you will always choose the number (cell) myDeck.size (), which constantly changes when you delete cards, and is issued when there are no cards in the deck