Basic playing card generation and shuffle

Question

This is my first attempt at Java so I appreciate any criticism or pointers. The program should generate a full set of given decks and then shuffle them.

import java.util.Scanner;
import java.util.Arrays;
import java.util.Random;

public class shuffle {
    //Make it easier to find array items
    static int value = 0;
    static int suit = 1;
    static int remaining = 2;

    public static void main(String Args[]) {
            int[][] deck;

            String clabel;
            //number of decks to generate
            int deckcount = 1;
            int decksize = deckcount * 52;

            deck = new int[decksize][2];
            //loop through deck building until deck reaches card count
            for (int c = 0; c < decksize;) {
                for (int s = 0; s < 4; s++) {
                    for (int v = 1; v <= 13; v++) {
                        deck[c][value] = v;
                        deck[c][suit] = s;
                        System.out.println(getCard(deck[c]));
                        c++;
                    }
                }
                //test parameters for debugging
                //System.out.println(""+c);
                //pressAnyKeyToContinue();
            }

            System.out.println("\nSHUFFLING\n");

            int order[];
            order = new int[decksize];
            order = shuffle(decksize);

            for(int i = 0;i < decksize;i++) {
                System.out.println(getCard(deck[order[i]]));
            }
        }
    //method to get cards correct label
    public static String getCard(int[] card) {
            String c_label, s_name = "", v_name = "";
            //get suit name
            switch (card[suit]) {
                case 0:
                    s_name = "Clubs";
                    break;
                case 1:
                    s_name = "Diamonds";
                    break;
                case 2:
                    s_name = "Hearts";
                    break;
                case 3:
                    s_name = "Spades";
                    break;
            }
            //get card label
            if (card[value] > 10 || card[value] == 1) {
                switch (card[value]) {
                    case 1:
                        v_name = "ACE";
                        break;
                    case 11:
                        v_name = "Jack";
                        break;
                    case 12:
                        v_name = "Queen";
                        break;
                    case 13:
                        v_name = "King";
                        break;
                }
            } else {
                v_name = "" + card[value];
            }
            c_label = v_name + " of " + s_name;

            return c_label;

        }
        //shuffle deck
    public static int[] shuffle(int size) {
        Random rgen = new Random(); // Random number generator
        int[] order;
        order = new int[size];

        for(int i = 0; i < order.length; i++) {
            order[i] = i;   
        }


        for (int i = 0; i < order.length; i++) {
            int randomPosition = rgen.nextInt(order.length);
            int temp = order[i];
            order[i] = order[randomPosition];
            order[randomPosition] = temp;
        }

        return order;
    }
    private static void pressAnyKeyToContinue() {
        System.out.println("Press any key to continue...");
        try {
            System.in.read();
        } catch (Exception e) {}
    }
}

Answer 1

You should create a immutable Card class that hold the value and suit and use an enum for the suit values:

public class Card{
    public static enum Suit{
        HEARTS, CLUBS, SPADES, DIAMONDS;
    }
    public final int value;
    public final Suit suit;
    public Card(int value, Suit suit){
        this.value = value;
        this.suit = suit;
    }

}

You can put the toString there instead of the getCard. Speaking of conversion to String; Integer.toString is a much cleaner way of converting a number to a string.

Your shuffle isn't a true shuffle, instead use Collections.shuffle.

Collections.shuffle(Arrays.asList(deck));

You can also pass your own Random to it.

Answer 2

First off I want to say that your decision to make an array that represents the ordering permutation is a really good idea. Many people would have passed in the array to be shuffled and had it mutated. Returning a permutation and applying that to an ordered array is a conceptually much nicer way to go.

---

Your shuffle algorithm has an interesting and common bug. Your shuffle algorithm is "generate the vector {0, 1, 2, 3, 4, ..., 51}. For each position 0 through 51, swap the current contents with a randomly-generated position".

Let's do a little math. Suppose we have only three cards, to make the math easier. How many decks are there? Six:

1. 0, 1, 2
2. 0, 2, 1
3. 1, 0, 2
4. 1, 2, 0
5. 2, 0, 1
6. 2, 1, 0

Each deck should be as likely as any other.

How many paths through the algorithm are there? Twenty-seven, each equally likely:

1. Swap 0 with 0, swap 1 with 0, swap 2 with 0
2. Swap 0 with 1, swap 1 with 0, swap 2 with 0
3. Swap 0 with 2, swap 1 with 0, swap 2 with 0
4. Swap 0 with 0, swap 1 with 1, swap 2 with 0
5. Swap 0 with 1, swap 1 with 1, swap 2 with 0
6. Swap 0 with 2, swap 1 with 1, swap 2 with 0
7. Swap 0 with 0, swap 1 with 2, swap 2 with 0
8. Swap 0 with 1, swap 1 with 2, swap 2 with 0
9. Swap 0 with 2, swap 1 with 2, swap 2 with 0

... I won't list all of them.

Now we have 6 possible outcomes and 27 paths through the algorithm, so plainly some of the outcomes must be produced more often than the others. There is no way to divide 27 paths into 6 sets such that each set has the same number of elements.

The same is true with your shuffle; there are \$52!\$ possible decks and \$52^{52}\$ paths through the code, and those don't divide evenly into each other.

What you've done is the most common incorrect implementation of the Knuth Shuffle. Now go do some research and figure out how to fix your bug; it's an easy fix.

Once you've done that, you've still got a problem. There are 52! possible decks but only \$2^{32}\$ possible seeds to Random, so you are still not sampling the space of all possible decks fairly. If you care about this, you should probably use a crypto-strength randomness generator.

You might be interested in my articles on these subjects.

• http://ericlippert.com/2013/04/15/producing-permutations-part-one/
• http://ericlippert.com/2014/10/13/producing-combinations-part-one/

---

Other problems with your code:

int[][] deck;

What the heck? A deck of cards is an array of arrays of integers? That's craziness. I want to see

Deck deck = new Deck();

Programming computers is the art of creating meaningful abstractions. A deck is not an array of arrays of integers. A deck is a deck. Make a class that represents the concept of deck that abstracts away this mechanism.

---

String clabel;

We pronounce that "klabel" of course.

Seriously, typing out the remaining letters of a word will not cause you to die younger.

Later...

  String c_label, s_name = "", v_name = "";

Oh, it's c_label now when it was clabel before. Now would be a good time to get into good habits. I don't care what your naming strategy is, so long as it is (1) comprehensible, and (2) consistent. Your strategy is neither. My preference as a C# programmer is

cardLabel

but

card_label 

is idiomatic in Java. Better entirely though would be to make a Card type that has a label getter.

---

        order = new int[decksize];
        order = shuffle(decksize);

If you saw

x = 1;
x = 2;

you would immediately note that the first line is pointless. But your code is equally pointless for exactly the same reason.

Something made you think that you needed to initialize order to a new array immediately before throwing it away, just like the contents of x are thrown away. What incorrect process of thought led you to this conclusion? Can you examine that process?

These are not rhetorical questions. I analyze these sorts of programming errors for a living and I am very interested in the beginner-mind psychology that leads to them. Any insights would be appreciated.

---

WHY IS "ACE" IN ALL CAPS? It seems weird.

Answer 3

Some notes:

The shuffle method creates an array, so there is no need to create one before calling it:

int order[];
//order = new int[decksize]; - not needed, would be replaced
order = shuffle(decksize);

For the shuffle, use the Fisher-Yates algorithm:

for (int i = order.length - 1; i > 0; i--) {
  int randomPosition = rgen.nextInt(i + 1);
  int temp = order[i];
  order[i] = order[randomPosition];
  order[randomPosition] = temp;
}

Answer 4

In my opinion, I find your loops to build the decks a little convoluted. I think a more standard for loop for the number of decks would make it easier to understand:

var cardIndex = 0;
for (int c = 0; c < deckcount; c++) {
    for (int s = 0; s < 4; s++) {
        for (int v = 1; v <= 13; v++) {
            deck[cardIndex][value] = v;
            deck[cardIndex][suit] = s;
            System.out.println(getCard(deck[cardIndex]));
            cardIndex++;
        }
    }
}

This way uses an extra variable but, when I read it, I'm not searching through the loop to find where you increment c. You could also calculate cardIndex based on c, s, and v, but I think having an extra variable to increment is more straightforward.

As a side note, I find it humorous that you use c++ in java :)