6
\$\begingroup\$

My Deck Class:

(In case it isn't clear, a Card is just an object with two enums, one for suit and one for value).

I'm particularly proud of the readability of this little bit if I do say so myself.

public class Deck {

    private static Random random = new Random();

    private List<Card> cards = new ArrayList<Card>();

    Deck () {
        // Create one of each card to populate deck with.
        for (Card.Suit suit : Card.Suit.values()) {
            for (Card.Value value : Card.Value.values()) {
                Card card = new Card(value, suit);
                cards.add(card);
            }
        }
        // Shuffle multiple times to ensure randomness...
        this.shuffle();
        this.shuffle();
        this.shuffle();
    }

    public void shuffle() {
        List<Card> temp = new ArrayList<Card>();
        while(!cards.isEmpty()) {
            int randInt = random.nextInt(cards.size());
            temp.add(cards.remove(randInt));
        }
        cards = temp;
    }

    public void addCard(Card card) {
        cards.add(card);
    }

    public Card getNextCard() {
        return cards.remove(0);
    }

}

EDIT:

I revised the drawCard() method. What do you guys think?

public Card drawCard() {
    if(cards.isEmpty()) {
        System.err.println("Can't draw from an empty deck!");
        System.exit(-1);
    }
    return cards.remove(cards.size() - 1);
}
\$\endgroup\$
5
\$\begingroup\$

I would agree that your implementation is quite readable. Indentation and white space are consistent. The variables and operations are named clearly and concisely, with the exception of 'getNextCard' as noted by '200_success'. This allows us to focus much more at the low level nuts and bolts of your code.

    private static Random random = new Random();

Use caution in assigning an instance of the Random class as a static member as concurrent access to its operations can cause contention and consequent poor performance. A pattern I like to follow to ensure random instances are seeded uniquely is to have a package level static random member for seeding instances. This still risks contention, but limits the surface area for it to occur in. For simple gaming purposes the 48-bit seed of Random will suffice, but using java.security.SecureRandom will provide a better source of entropy:

    private static final SecureRandom SEEDER = new SecureRandom();
    private static final int ENTROPY = 256; // 2048-bit is good enough for banking
    private SecureRandom random = new SecureRandom(SEEDER.generateSeed(ENTROPY));

-

    this.shuffle();
    this.shuffle();
    this.shuffle();

If your shuffle method and random source are fair, than one shuffle is actually better than more -- you're skipping over entropy and risk introducing bias. If either is not fair shuffling more will not make them fair.

public void shuffle() {
    List<Card> temp = new ArrayList<Card>();
    while(!cards.isEmpty()) {
        int randInt = random.nextInt(cards.size());
        temp.add(cards.remove(randInt));
    }
    cards = temp;
}

While I would argue with mjolka that your implementation of shuffle is a non-in-place implementation of the Fisher-Yates shuffle, I would prefer to be more concise and use the base library utility method:

public void shuffle() {
    java.util.Collections.shuffle(cards, random);
}

I would join both mjolka and 200_success in recommending to guard your getNextCard implementation and throw an appropriate error as well as exposing an external status check method such as size() or isEmpty(). Also I agree the card should be popped from the end of the list when using an ArrayList as the backing collection.

Finally, you have a very well formed class with clear and concise public members. Be sure to add just enough javadoc comments to allow a the class to be consumed without needing to open the code.

\$\endgroup\$
3
\$\begingroup\$

When you have a good idea of the maximum size that an ArrayList will attain, it's a good idea to specify the capacity so that the list doesn't have to guess and reallocate. (It's better to overestimate than underestimate, so I'd leave room for two jokers just in case.)

private List<Card> cards = new ArrayList<Card>(54);

A public class should have a public constructor, else it won't be usable to code in other packages.

The constructor can call Deck.addCard() instead of cards.add(), and therefore it should.

Your class doesn't offer any way for users to see how many cards are in the deck. Therefore, the only recourse is this:

try {
    deck.getNextCard();
} catch (IndexOutOfBoundsException deckWasEmpty) {
    …
}

… which is ugly because IndexOutOfBoundsException leaks the fact that the deck uses some kind of array-like storage, and it should be possible for the caller to avoid an exception in the first place. I suggest that you offer a public int size() method. Also, getNextCard() on an empty deck should either return null or throw a NoSuchElementException.

A method named get…() has a connotation that it inspects the state of the object without changing it. That's not the case with your getNextCard(). I'd rename it to drawCard().

\$\endgroup\$
2
\$\begingroup\$

A more efficient way of shuffling is the Fisher-Yates shuffle aka the Knuth shuffle.

An implementation might look like this:

public void shuffle() {
  final int n = this.cards.size();
  for (int i = n - 1; i > 0; i--) {
    int j = random.nextInt(i + 1);
    swap(cards, i, j);
  }
}

private static <T> void swap(List<T> items, int i, int j) {
  T swap = items.get(i);
  items.set(i, items.get(j));
  items.set(j, swap);
}

You can find another implementation on Robert Sedgewick's Algorithms website.

[Edit: @psaxton has a much better suggestion to use java.util.Collections.shuffle.]

Shuffling multiple times will not be of any use, given a good shuffling algorithm.


In getNextCard, consider removing from the end of the list instead of the beginning, as it will be more efficient (elements do not need to be moved around).

Also consider throwing an appropriate exception if there are no more cards in the deck.


You might want to check the parameter card in addCard to check that the card is not null, is not already in the deck, won't make the deck have more than 52 cards, etc.

\$\endgroup\$
  • \$\begingroup\$ IllegalStateException for drawing from an empty deck is not quite appropriate, in my opinion. \$\endgroup\$ – 200_success Feb 7 '15 at 8:10
  • \$\begingroup\$ @200_success you're probably right, I'm not that familiar with Java. My first thought was NoSuchElementException but the documentation says 'Thrown by the nextElement method of an Enumeration to indicate that there are no more elements in the enumeration.' So I took the idea from this answer. I'll update the post. \$\endgroup\$ – mjolka Feb 7 '15 at 8:14
  • \$\begingroup\$ My solution was just to crash the program if the user attempts to draw from an empty deck, as I can't imagine a scenario that is likely to occur where that would happen, absent of other code issues. \$\endgroup\$ – Bassinator Feb 7 '15 at 18:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.