3
\$\begingroup\$

I humbly ask that you review my simple (albeit a little over-done) program. The intention is to create a deck of cards, shuffle them, and draw the first five off the deck as a poker hand.

Can you offer some feedback, particularly if something can be done more efficiently? I intentionally left out much of the error-correction that can go into this just to cement the object-based design.

CardEnum.java

public enum CardEnum {
    ACE("A"), TWO("2"), THREE("3"), FOUR("4"),
    FIVE("5"), SIX("6"), SEVEN("7"), EIGHT("8"),
    NINE("9"), TEN("10"), JACK("J"), QUEEN("Q"), KING("K");

    private String name;

    CardEnum (String name) {
        this.name = name;
    }

    String getSymbol() {
        return this.name;
    }
}

SuitEnum.java

public enum SuitEnum {
    HEART("H"), DIAMOND("D"), CLUB("C"), SPADE("S");

    private String name;

    SuitEnum (String name) {
        this.name = name;
    }

    String getSymbol() {
        return this.name;
    }
}

Card.java

public class Card {
    private CardEnum card;
    private SuitEnum suit;

    private Card() {}

    public Card(CardEnum card, SuitEnum suit) {
        this.card = card;
        this.suit = suit;
    }

    void changeCard(CardEnum card, SuitEnum suit) {
        this.card = card;
        this.suit = suit;
    }

    String getName() {
        return card.getSymbol() + suit.getSymbol();
    }

    CardEnum getCard() { return card; }

    SuitEnum getSuit() { return suit; }
}

Deck.java

import java.util.*;

public class Deck {
    private final int newSize = 52;
    private List<Card> deck = new ArrayList<>(newSize);

    public Deck() {
        for (CardEnum c : CardEnum.values()) {
            for (SuitEnum s: SuitEnum.values()) {
                Card newCard = new Card(c, s);
                this.deck.add(newCard);
            }
        }

        if (deck.size() != newSize)
            throw new IndexOutOfBoundsException("Deck does not contain 52 cards.");
    }

    int size() { return this.deck.size(); }

    String draw() {
        Iterator<Card> itrCard = deck.iterator();

        if (!itrCard.hasNext()) {
            throw new IndexOutOfBoundsException("Deck is empty!");
        }

        Card next = itrCard.next();
        String name = next.getName();
        this.deck.remove(next);
        return name;
    }

    void shuffle() {
        Collections.shuffle(this.deck);
    }
}

And lastly, basicpoker.java

public class Basicpoker {

    public static void main(String[] args) {

        Deck deck = new Deck();

        System.out.println("The deck contains " + deck.size() + " cards.");
        deck.shuffle();
        System.out.println("The deck is now shuffled.");

        System.out.println("The first five cards drawn are:");
        for (int i = 0; i<5; i++) {
            System.out.print(deck.draw() + " ");
        }  
    }        
}
\$\endgroup\$
  • 1
    \$\begingroup\$ CardEnum sounds like the wrong name for that, if you're using it in a class called Card. I think I'd prefer Face or similar. And drop the Enum - it's noise. \$\endgroup\$ – Clockwork-Muse Apr 5 '12 at 17:40
  • \$\begingroup\$ That's a good suggestion. I was only using it to distinguish the fact that it's a special enum class. \$\endgroup\$ – user11607 Apr 5 '12 at 23:08
  • 1
    \$\begingroup\$ I would change getSymbol and getName to @Override toString and change the underlying List of cards in the Deck to a LinkedList so you can take advatange of the removeLast function when you draw. \$\endgroup\$ – Eva Jul 7 '12 at 9:34
3
\$\begingroup\$
  1. Using name coupled with getSymbol is confusing - the underlying property name should be the same.
  2. Do not omit public/private/default modifiers.
  3. Get rid of this method:

    // A card should be immutable.
    // This method exposes the guts for no good reason.
    // Gives you a chance to create an invalid deck.
    void changeCard(CardEnum card, SuitEnum suit) {
        this.card = card;
        this.suit = suit;
    }
    
  4. Depending on how frequently the cards are reused and accessed, you might want to compute this value only once, and then keep returning it.

    // This could be computed only once.
    String getName() {
        return card.getSymbol() + suit.getSymbol();
    }
    
  5. I like more verbose variable names and some things ought to take one line whereas others ought not. I would write:

    public Deck() {
        for (CardEnum card : CardEnum.values()) {
            for (SuitEnum suit: SuitEnum.values()) {
                this.deck.add(new Card(card, suit)); // I find this readable.
            }
        }
    
        if (deck.size() != newSize) {
            throw new IndexOutOfBoundsException("Deck does not contain 52 cards.");
        } 
        // If statements without braces can be dangerous when you change the code later.
        // I personally do not like them here or elsewhere.
    }
    
  6. You might want to make the deck reusable by not throwing cards away but storing them in a dirty pile(say an array). Because a deck will always contain 52 cards, I would use two arrays of 52 elements each - one for clean cards and one for the dirty ones. Then you can make these arrays behave like queues when you need them to by keeping track of exactly one number: the number of cards remaining in the clean pile. You can fetch the cards from 51st to 0th. That way you do not even need to erase the contents of the clean pile - by decreasing the number you hide one more card from the clean deck. This should make your program a bit smaller and faster, but maybe this is premature optimization.

  7. You could add an bool isEmpty() and bool isFull() method to the Deck class.

  8. You could add automatic re-shuffling by passing a bool flag to the constructor. It should default to false via constructor chaining. This can be useful in the case when you have many players and the game requires multiple decks, but this does not apply to poker. I am not sure what kind of system you are trying to build.

  9. You only have about two pages of Java code. You need to expand on this. It looks hygienic precisely because it is not doing all that much.

Looks good otherwise. Keep at it.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your cogent advice. This was very helpful. :) From your advice in #3, is it a general practice to code somewhat "defensively" for things like the underlying guts of an application? #6 is a good piece of advice, I never thought of keeping 2 decks. \$\endgroup\$ – user11607 Apr 3 '12 at 21:22
  • 4
    \$\begingroup\$ I work in a code base that weighs about a gigabyte of just textual code. When the software gets complex, you want to make every effort to minimize the potential for bugs. In your case it does not really matter, but all large software starts out small. Immutability is not just a neat trick - it is a philosophy. It is part of functional programming. If things do not change (are immutable), then the passage of time almost does not matter to you. Immutability is big selling point for Clojure. Add mut. obj to tree and change it - cant find it. javapractices.com/topic/TopicAction.do?Id=29 \$\endgroup\$ – Leonid Apr 3 '12 at 22:28

Your Answer

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