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++) {
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();
}
}
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
Deckclass. -
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. -
fullDeckis a list of cards, right? So call the fieldcardsinstead.
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);
}
}
-
It’s generally preferred to
importindividual libraries (without.*). Since you just useArrayListandListfromutil, have them separate. -
All fields must be
privateas 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
Deckfor other games), then you can allow the game to decide which cards to use (not every game uses the standard 52 cards). -
Since
fullDeckholds all the cards in aDeck, you won’t needdeckLength.
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
Handinstead. getHanddoes 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.
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;
}
}