Basic OOP Poker – Deck, Cards and Hands

Posted on

Problem

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++) {
}
}
}

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

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();

}
}
``````

Solution

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!

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.
}
``````

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() {
for (int i = 0; i < SUITS.length; i++) {
for (int j = 0; j < RANKS.length; j++) {
}
}
}

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) {
}

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

• 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`.

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

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;
}
}
``````