Basic OOP Poker - Deck, Cards and Hands

Question

I decided it would be interesting to simulate a game of Poker. Baby steps at the moment, and eventually I'll attempt to turn it into a GUI. The code I have so far is very basic such as populating a deck, shuffling the deck, and distributing two cards to a player. Trying to understand how inheritance works, the proper use of Java generics, and grasping the concept of how powerful/fun OOP can be. The Collections Framework is also something I'm beginning to experiment with.

I would really appreciate anything that will help me better understand Java.

import java.util.*;


class Deck {
    // create possible card combinations
    public final String[] SUITS = { "H", "D", "C", "S" };
    public final String[] RANKS = { "A", "K", "Q", "J", "10", "9", "8", "7", "6", "5", "4", "3", "2" };

    // maximum number of cards
    public final int deckLength = SUITS.length * RANKS.length;
    public List<String> fullDeck = new ArrayList<>();

    public Deck() {
        for(int i = 0; i < SUITS.length; i++) {
            for(int j = 0; j < RANKS.length; j++) {
                fullDeck.add(RANKS[j] + SUITS[i]);
            }
        }
   }

    public Deck(List<String> fullDeck) {
        this.fullDeck = fullDeck;
        for(int i = 0; i < SUITS.length; i++) {
            for(int j = 0; j < RANKS.length; j++) {
                fullDeck.add(RANKS[j] + SUITS[i]);
            }
        }
    }

    public List<String> shuffle(List<String> fullDeck) {
        this.fullDeck = fullDeck;
        Collections.shuffle(fullDeck);
        return fullDeck;
    }

    // this was mainly used for testing purposes
    // to ensure shuffling was indeed taking place
    public void showDeck(List<String> fullDeck) {
        this.fullDeck = fullDeck;
        for(int i = 0; i < deckLength; i++) {
            System.out.printf("%s ",fullDeck.get(i));
        }
    }
}

class Hands extends Deck {

    public String[] hand = new String[2];
    Random random = new Random();

    // select 2 cards to distribute to player
    public String[] getHand(List<String> fullDeck) {
        super.fullDeck = fullDeck;
        for(int i = 0; i < this.hand.length; i++) {
            this.hand[i] = fullDeck.get(random.nextInt(super.deckLength));
        }
        return this.hand;
    }

    // show player hand
    public void showHand() {
        for(int i = 0; i < this.hand.length; i++) {
            System.out.printf("%s ", this.hand[i]);

        }
    }
}

public class Cards {
    public static void main(String[] args) {

        List<String> cards = new ArrayList<>();
        Deck deck = new Deck(cards);
        deck.shuffle(cards);

        Hands hands = new Hands();
        hands.getHand(cards);
        hands.showHand();

    }
}

Answer 1

Welcome to Code Review, and welcome to Java! Overall your code is clean and easy to understand, so thank you for that.

Enums

Storing these values in a string array is not very extensible:

public final String[] SUITS = { "H", "D", "C", "S" };
public final String[] RANKS = { "A", "K", "Q", "J", "10", "9", "8", "7", "6", "5", "4", "3", "2" }

The first thing to note is that a List provides more options and flexibility than a raw array. However, I think it would be best to store these values as an Enum:

public enum SUITS {
    H, D, C, S
}

This will allow you to use these values in a switch statement, and provides other benefits. Most of all, it is more readable and more extensible.

Variable Names

I would strongly recommend against single letter variable names outside of simple constructs such as for (int i = 0; i < something; i++);. Instead of H, D, C, and S, I would just spell them out as HEARTS, DIAMONDS, etc. There is no disadvantage to using the extra characters to make the code more readable. In the rest of your code you do not make this mistake, so that is good.

Improper OOP

I do not think that Hand should inherit from Deck:

class Hands extends Deck {

I would advise that you think about inheritance as an "is a" relationship. A hand is not a deck, and should not be treated like one. For example, it is unlikely that you will need to "shuffle" a hand. To me, this is the same as having Hand or Deck inherit from a Card. It is not going to end up being helpful to the organization or understanding of the code.

I don't understand why Cards is a class:

public class Cards {

A singular Card should be the class here. Then, the Hand and Deck can have a "has a" relationship with cards, and use Card objects. The way you have this set up here:

    List<String> cards = new ArrayList<>();
    Deck deck = new Deck(cards);
    deck.shuffle(cards);

    Hands hands = new Hands();
    hands.getHand(cards);
    hands.showHand();

really does not make a whole lot of sense. Why would a Cards object have Decks or Hands as objects? It should definitely be the other way around.

I look forward to seeing a revised version of your code in another question!

Answer 2

I'm just going to focus on the Deck class.

Your use of fullDeck in the Deck class is bizarre:

• When would you ever use the Deck(List<String> fullDeck) constructor? What kind of parameter should one pass in? What if I try this… shouldn't I get a deck of Pokémon cards? Unfortunately, that doesn't work.

  List<String> pokemon = new LinkedList<>(Arrays.asList(new String[] {
      "Bulbasaur", "Ivysaur", "Venusaur", "Charmander", ...
  }));
  Deck d = new Deck(pokemon);
  

• Even if that did work, it is risky to let others meddle with the innards of your class. Suppose that I subsequently did

  pokemon.removeFirst();
  

  Then the deck would also have one of its cards removed, unbeknownst to the code in your Deck class.

• shuffle() should take no parameters and return nothing. There is no reason why your deck needs to be told again what cards it should hold.

• The same goes for showDeck() — showing should be a "read-only" operation, and shouldn't result in the deck "adopting" a new list of cards.

• fullDeck is a list of cards, right? So call the field cards instead.

Next, for proper object-oriented design, you should consider what kinds of operations should be supported by a Deck. For example, you should be able to draw a card from the top of the pile, and return a card to the bottom of a pile. So, you would introduce methods like

public String drawCard() throws NoSuchElementException {
    if (this.cards.isEmpty()) {
        throw new NoSuchElementException();
    }
    return this.cards.remove(0);
}

public void returnCard(String card) {
    // We trust that the card is valid, and not a Pokémon card.
    this.cards.add(card);
}

Rather than having showDeck() print its output, I suggest implementing toString().

public String toString() {
    // String.join() was introduced in Java 8.  In older versions,
    // you would use a StringBuilder.
    return String.join(" ", this.cards);
}

Suggested implementation

import java.util.*;

class Deck {
    // create possible card combinations
    private static final String[] SUITS = {
        "H", "D", "C", "S"
    };
    private static final String[] RANKS = {
        "A", "K", "Q", "J", "10", "9", "8", "7", "6", "5", "4", "3", "2"
    };

    private List<String> cards;

    public Deck() {
        this.cards = new LinkedList<>();
        for (int i = 0; i < SUITS.length; i++) {
            for (int j = 0; j < RANKS.length; j++) {
                this.cards.add(RANKS[j] + SUITS[i]);
            }
        }
    }

    public void shuffle() {
        Collections.shuffle(this.cards);
    }

    public String drawCard() throws NoSuchElementException {
        if (this.cards.isEmpty()) {
            throw new NoSuchElementException();
        }
        return this.cards.remove(0);
    }

    public void returnCard(String card) {
        this.cards.add(card);
    }

    public void toString() {
        return String.join(" ", this.cards);
    }
}

Answer 3

• It's generally preferred to import individual libraries (without .*). Since you just use ArrayList and List from util, have them separate.

• All fields must be private as they're not supposed to be part of the interface.

• Do you really want this to only work for Poker? If you'd like to have some reusability (using the same Deck for other games), then you can allow the game to decide which cards to use (not every game uses the standard 52 cards).

• Since fullDeck holds all the cards in a Deck, you won't need deckLength.

Answer 4

You already got some great advise about your Deck class, so I'll focus on your Hands class.

First of all naming:

• The class represents the hand of a single person, right? So it should be named Hand instead.
• getHand does return a hand, but it's primary function is to set a hand, or in other words, to give cards to this hand (or rather to let the hand take cards).

As for the structure:

Sometimes, it helps to think about the real-life situation: Is it the responsibility of the hand or of the player to get cards from the deck? Generally, it's not, and the Hand class shouldn't be responsible for it either.

Your Hand class is also quite static. It is hardcoded how many cards a hand can have, so you cannot for example play Omaha. The way cards are added to a hand right now, you also cannot play a game where cards are added at different points in time (for example Stud).

A more reusable and practical Hand class might look like this:

public class Hand {

    private List<Card> cards; // note the Card class, it's a lot more practical than a string
                              // eg you could add a "hidden" field, easily compare suits of cards, etc.

    public void addCards(List<Card> cardstoAdd) {
        cards.add(cardstoAdd);
    }

    public List<Card> getCards() {
        return cards;
    }

    public SomeEnumOrClass calculateHandValue() {
        // calculate the best possible hand
    }

    @Override
    public void toString() {
        // hand to string for debug purposes
    }
}

class Player {
    Hand hand;
    ...
}

class Dealer {

    public void deal(List<Player> player, int cardCountPerPlayer) {
        // give each player cards
    }

    public void giveCards(Player player, int cardCount) {
        // give one player cards
    }
    ...
}

class Table {
    private Dealer dealer;
    private List<Player> player;
    ...
}

You could also create a CardCollection class (containing logic for adding, getting, and showing cards), which the Hand class extends. That way, you could also add a TableCards class, which also extends CardCollection.

Answer 5

I would suggest to give Rank enum cardinality. This can help you with calculating game situations. Something like this:

public enum Rank {
    TWO(2),
    THREE(3),
    FOUR(4),
    FIVE(5),
    SIX(6),
    SEVEN(7),
    EIGHT(8),
    NINE(9),
    TEN(10),
    JACK(11),
    QUEEN(12),
    KING(13),
    ACE(14);

    private int cardinality;

    private Rank(final int cardinality) {
        this.cardinality = cardinality;
    }

    public int getCardinality() {
        return cardinality;
    }
}

Answer 6

Is a Card object supposed to represent a kind of card (e.g. "spade jack" or "diamond six") or is it supposed to represent an entity which can only be in one place at any given time? For example, if you shuffled together two decks, and the top two cards of the shuffled-together deck are both "club queen", should those be represented by the same Card object, or should one card object represent "club queen which is at position 0 in a particular deck" and the other one "club queen which is at position 1 in that same deck"?

I would suggest that you define a concept of a "card container" [used for hands, decks, etc.] and then define a "card" to be an object which has a suit and rank, and always sits in some sort of card container, but can be moved from one container to another. Each container should have a means of identifying the cards which occupy it, and each card should have a means of identifying the one container which contains it. Moving a card from one container to another should cause it to be removed from the former and added to the latter.

Answer 7

There are two things I want to point out that has been shown in other answers, but not pointed out why it should change like this.

In this code:

public String[] getHand(List<String> fullDeck) {
    super.fullDeck = fullDeck;
    for(int i = 0; i < this.hand.length; i++) {
        this.hand[i] = fullDeck.get(random.nextInt(super.deckLength));
    }
    return this.hand;
}

This is not how you should give two cards to a player. With this code, there is a probability that the player will be given the same two cards. That should not happen. This is why the answer by 200_success is very important, where the cards is first shuffled and then a card given with:

return this.cards.remove(0);

---

public final String[] SUITS = { "H", "D", "C", "S" };
public final String[] RANKS = { "A", "K", "Q", "J", "10", "9", "8", "7", "6", "5", "4", "3", "2" };

Now let's say that I have a Deck, simply by Deck deck = new Deck();

Then I would be able to do: deck.RANKS[2] = "Haha"; and violà, I have changed your 'constants'. An array is mutable, so it is possible to change the values of it. And as your variable is public, it is possible to access it from other classes.

This is why those variables should absolutely be private. And as they are meant to be used as constants, they should also be static.

private static final String[] SUITS = { "H", "D", "C", "S" };
private static final String[] RANKS = { "A", "K", "Q", "J", "10", "9", "8", "7", "6", "5", "4", "3", "2" };

This however still makes them mutable, which is why an enum is a good approach. It is not possible to add a member to an enum at runtime.

Another approach could be to use

private static final List<String> SUITS = Collections.unmodifialbleList(Arrays.asList("H", "D", "C", "S"));

As they have private visibility though, it is okay to use them as private static final String[], just make sure that your code doesn't change any index of them.