Is my method working effectively?

Asked 6/8, 2012 at 21:1 Answered 6/8, 2012 at 21:27

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?

Beachlamar answered 6/8, 2012 at 21:1 Comment(5)

Can you utilize the Collections.shuffle method with the Cards in your Deck? - docs.oracle.com/javase/7/docs/api/java/util/… – Psychrometer 6/8, 2012 at 21:4

One violation is "uses a single Random object". You are using one per iteration of your loop. – Vitalism 6/8, 2012 at 21:4

I've added the homework tag based on the "actual question". – Thunder 6/8, 2012 at 21:4

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? – Porosity 6/8, 2012 at 21:4

the point of the exercise is to be able to do what .shuffle does using Random I think – Beachlamar 6/8, 2012 at 21:5

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

Reindeer answered 6/8, 2012 at 21:5 Comment(11)

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. – Shelving 6/8, 2012 at 21:6

Correct me if I'm wrong, but wouldn't this change make it a Knuth Shuffle? – Stonyhearted 6/8, 2012 at 21:6

@Carl, that shouldn't matter technically speaking. – Reindeer 6/8, 2012 at 21:7

@Reindeer It seems the whole point of this assignement is not to use Collections#shuffle! – Charming 6/8, 2012 at 21:7

@aioobe, Do you mean from a Java perspective or in general? I guess I'm not very familiar with how Java seeds the generator, and perhaps it's even a singleton? – Shelving 6/8, 2012 at 21:8

@Carl, new Random does it's best to set a seed which has not been previously used. – Reindeer 6/8, 2012 at 21:9

Like this?: public void shuffle() { ArrayList<Card> shuffled = new ArrayList<Card>(myDeck.size()); Random rn = new Random(); for( int x = myDeck.size(); x > 0 ; x--) { int index1 = rn.nextInt(myDeck.size()); Card c = myDeck.remove(index1); myDeck.add(c); } myDeck = shuffled; } – Beachlamar 6/8, 2012 at 21:16

@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. – Charming 6/8, 2012 at 21:17

ok I changed the for loop to (int x = 500 ; x > 0 ; x-- ) but when I run the code nothing shows up in the terminal?? what happened – Beachlamar 6/8, 2012 at 21:18

52 iterations should be enoungh to give all 52! possible permutations equal probability. – Reindeer 6/8, 2012 at 21:23

Yes but nothing at all is showing up in the terminal when i execute the main class. Is my mainclass test wrong? I wrote public static void main(String[] args ) { Deck d = new Deck(); d.newDeck(); d.shuffle(); d.printDeck(); } it should work but nothing is showing up its a blank screen – Beachlamar 6/8, 2012 at 21:24

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.

Pilpul answered 6/8, 2012 at 21:6 Comment(3)

Doing 520 iterations would A) not be very efficient B) not yield a result where all permutations are equally probable. – Reindeer 6/8, 2012 at 21:50

@Reindeer 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. – Pilpul 6/8, 2012 at 22:58

Not really. 52 iterations is enough. See my answer. – Reindeer 7/8, 2012 at 8:35

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

Charming answered 6/8, 2012 at 21:5 Comment(3)

I agree, 52 times is definitely not enough. 1+ – Foxhound 6/8, 2012 at 21:6

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. – Reindeer 6/8, 2012 at 21:44

@Reindeer 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. – Charming 6/8, 2012 at 21:54

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

Beyrouth answered 6/8, 2012 at 21:27 Comment(2)

why not change the for to a while? – Reindeer 6/8, 2012 at 21:48

@Reindeer that's definitely an option! – Beyrouth 6/8, 2012 at 22:25

Recommended topics

Hot tags