Simulations of the card game Crazy Eights

Question

The following code simulates the card game Crazy Eights. Some of the methods seem a little clunky or superfluous (although I've yet to come up with a better solution). I'm specifically referring to the convert method used in EightsCard which is called as a helper method in a number of EightsPlayer methods (play, searchForMatch and drawForMatch).

I'm really interested in way that this code could be refactored. After spending some time reading up on abstract classes and interfaces I'm starting to think that the classes in the 'defaults' folder would be better off as interfaces with default methods.

What I want is a good set of default classes and/or interfaces which I can use as a starting point for virtually any classic playing card game.

Card:

public class Card {

  public static final String[] RANKS = {
      null, "Ace", "2", "3", "4", "5", "6", "7",
      "8", "9", "10", "Jack", "Queen", "King"};
  public static final String[] SUITS = {
      "Clubs", "Diamonds", "Hearts", "Spades"};

  private final int rank;
  private final int suit;

  public Card(int rank, int suit) {
    this.rank = rank;
    this.suit = suit;
  }

  public int getRank() {
    return rank;
  }

  public int getSuit() {
    return suit;
  }  

  public String toString() {
    return RANKS[rank] + " of " + SUITS[suit]; 
  }

  public boolean equals(Card that) {
    return rank == that.rank
        && suit == that.suit;
  }

  /**
   * Compares Card values when Aces are Low and the suit value dominates.
   * @param that Card to be compared
   * @return -1 if lower value, 1 otherwise.
   */
  public int compareValueALSD(Card that) {
    if (suit < that.suit) {
      return -1;
    }
    if (suit > that.suit) {
      return 1;
    }
    if (rank < that.rank) {
      return -1;
    }
    if (rank > that.rank) {
      return 1;
    }
    return 0;
  }

  /**
   * Compares Card values when Aces are High and the suit value dominates.
   * @param that Card to be compared
   * @return -1 if lower value, 1 otherwise.
   */
  public int compareValueAHSD(Card that) {
    if (suit < that.suit) {
      return -1;
    }
    if (suit > that.suit) {
      return 1;
    }
    if (this.rank == 1 && that.rank != 1) {
      return 1;
    }
    if (rank < that.rank) {
      return -1;
    }
    if (rank > that.rank) {
      return 1;
    }
    return 0;
  }

  /**
   * Prints a deck of cards.     
   */
  public static void printDeck(Card[] cards) {
    for (int i = 0; i < cards.length; i++) {
      System.out.println(cards[i]);
    }
  }
}

CardCollection:

import java.util.ArrayList;
import java.util.Random;

/**
 * A collection of playing cards.
 */
public class CardCollection {

  private String label;
  private ArrayList<Card> cards;

  /**
   * Constructs an empty collection.
   */
  public CardCollection(String label) {
    this.label = label;
    this.cards = new ArrayList<Card>();
  }

  /**
   * Returns the label of the card collection.
   */
  public String getLabel() {
    return label;
  }

  /**
   * Returns the card with the given index.
   */
  public Card getCard(int i) {
    return cards.get(i);
  }

  /**
   * Adds the given card to the collection.
   */
  public void addCard(Card card) {
    cards.add(card);
  }

  /**
   * Removes and returns the card with the given index.
   */
  public Card popCard(int i) {
    return cards.remove(i);
  }

  /**
   * Removes and returns the last card.
   */
  public Card popCard() {
    int i = size() - 1;
    return popCard(i);
  }

  /**
   * Returns the number of cards.
   */
  public int size() {
    return cards.size();
  }

  /**
   * True if the collection is empty, false otherwise.
   */
  public boolean empty() {
    return cards.size() == 0;
  }

  /**
   * Moves n cards from this collection to the given collection.
   */
  public void deal(CardCollection that, int n) {
    for (int i = 0; i < n; i++) {
      Card card = popCard();
      that.addCard(card);
    }
  }

  /**
   * Moves all remaining cards to the given collection.
   */
  public void dealAll(CardCollection that) {
    int n = size();
    deal(that, n);
  }


  /**
   * Returns the last card.
   */
  public Card last() {
    int i = size() - 1;
    return cards.get(i);
  }

  /**
   * Swaps the cards at indexes i and j.
   */
  public void swapCards(int i, int j) {
    Card temp = cards.get(i);
    cards.set(i, cards.get(j));
    cards.set(j, temp);
  }

  /**
   * Randomly permute the cards.
   */
  public void shuffle() {
    Random random = new Random();
    for (int i = size() - 1; i > 0; i--) {
      int j = random.nextInt(i);
      swapCards(i, j);
    }
  }

  /**
   * Returns a string representation of the card collection.
   */
  public String toString() {
    return label + ": " + cards.toString();
  }

  /**
   * Prints the label and cards.
   */
  public void display() {
    System.out.println(label + ": ");
    for (Card card: cards) {
      System.out.println(card);
    }
    System.out.println();
  }

}

Deck:

/**
 * A deck of playing cards.
 */
public class Deck extends CardCollection {

  /**
   * Constructs a standard deck of 52 cards.
   */
  public Deck(String label) {
    super(label);

    for (int suit = 0; suit <= 3; suit++) {
      for (int rank = 1; rank <= 13; rank++) {
        addCard(new Card(rank, suit));
      }
    }
  }
}

Player:

public class Player {

  private String name;
  private CardCollection hand;

  /**
   * Constructs a player with an empty hand.
   */
  public Player(String name) {
    this.name = name;
    this.hand = new CardCollection(name);
  }

  /**
   * Gets the player's name.
   */
  public String getName() {
    return name;
  }

  /**
   * Gets the player's hand.
   */
  public CardCollection getHand() {
    return hand;
  }


  /**
   * Displays the player's hand.
   */
  public void display() {
    hand.display();
  }

Eights:

import java.util.ArrayList;
import java.util.Scanner;

/**
 * Simulates a game of Crazy Eights. See https://en.wikipedia.org/wiki/Crazy_Eights for basic play
 * and scoring rules.
 */
public class Eights {

  private CardCollection drawPile;
  private CardCollection discardPile;
  private Scanner in;
  private ArrayList<EightsPlayer> players;

  /**
   * Initializes the state of the game.
   */
  public Eights() {
    Deck deck = new Deck("Deck");
    deck.shuffle();

    // create an ArrayList of players
    this.players = new ArrayList<EightsPlayer>();

    // turn one card face up
    discardPile = new CardCollection("Discards");
    deck.deal(discardPile, 1);

    // put the rest of the deck face down
    drawPile = new CardCollection("Draw pile");
    deck.dealAll(drawPile);

    // create the scanner we'll use to wait for the user
    in = new Scanner(System.in);
  }


  /**
   * Adds the given player to the collection.
   */
  public void createPlayer(EightsPlayer player) {
    players.add(player);
  }

  /**
   * Prompts user to create a player name and uses input to create it.
   * @return The player's name
   */
  public String playerName() {
    System.out.println("Enter player name: ");
    String name = in.next();
    while (name == "") {
      name = in.nextLine();
    }
    return name;
  }

  /**
   * Gives player within collection a name and a hand with 5 cards.
   */
  public void equipPlayer() {
    EightsPlayer player = new EightsPlayer(playerName());
    createPlayer(player);
    drawPile.deal(player.getHand(), 5);
  }

  /**
   * Returns a player from the array at a given index.
   */
  public EightsPlayer getPlayer(int i) {
    return players.get(i);
  }

  /**
   * Returns index of a given player.
   */
  public int getPlayerIndex(EightsPlayer player) {
    return players.indexOf(player);
  }


  /**
   * Returns true if either hand is empty.
   */
  public boolean isDone() {
    for (int x = 0; x < players.size(); x++) {
      if (getPlayer(x).getHand().empty()) {
        return true;
      }
    }
    return false;
  }

  /**
   * Moves cards from the discard pile to the draw pile and shuffles.
   */
  public void reshuffle() {
    // save the top card
    Card convert = discardPile.popCard();
    EightsCard prev = new EightsCard(0, 0);
    prev = prev.convert(convert);


    // move the rest of the cards
    discardPile.dealAll(drawPile);

    // put the top card back
    discardPile.addCard(prev);

    // shuffle the draw pile
    drawPile.shuffle();
  }

  /**
   * Returns a card from the draw pile.
   */
  public EightsCard draw() {
    if (drawPile.empty()) {
      reshuffle();
    }
    Card convert = drawPile.popCard();
    EightsCard prev = new EightsCard(0, 0);
    return prev.convert(convert);
  }

  /**
   * Switches players.
   */
  public EightsPlayer nextPlayer(EightsPlayer current) {

    if (getPlayerIndex(current) < players.size() - 1) {
      return getPlayer(getPlayerIndex(current) + 1);
    }
    if (getPlayerIndex(current) == players.size() - 1) {
      return getPlayer(getPlayerIndex(current) + 1 - players.size());
    }
    return null;

  }

  /**
   * Displays the state of the game.
   */
  public void displayState() {
    for (int x = 0; x < players.size(); x++) {
      getPlayer(x).display();
    }
    discardPile.display();
    System.out.print("Draw pile: ");
    System.out.println(drawPile.size() + " cards");
  }

  /**
   * Waits for the user to press enter.
   */
  public void waitForUser() {
    in.nextLine();
  }

  /**
   * One player takes a turn.
   */
  public void takeTurn(EightsPlayer player) {
    Card convert = discardPile.last();
    EightsCard prev = new EightsCard(0, 0);
    prev = prev.convert(convert);    
    EightsCard next = player.play(this, prev);
    discardPile.addCard(next);

    System.out.println(player.getName() + " plays " + next);
    System.out.println();
  }

  /**
   * Plays the game.
   */
  public void playGame() {

    System.out.println("It's time to play Crazy Eights!");
    System.out.println();
    System.out.println("Enter number of players: ");

    int nop = in.nextInt();
    while (nop > 4 || nop < 2) {
      System.err.println("Number of players must be between 2 and 4!");
      System.out.println();
      System.out.println("Re-enter number of players: ");
      nop = in.nextInt();
    }

    for (int i = 0; i < nop; i++) {
      equipPlayer();
    }
    EightsPlayer player = getPlayer(0);

    // keep playing until there's a winner
    while (!isDone()) {
      displayState();
      waitForUser();
      takeTurn(player);
      player = nextPlayer(player);
    }
    // display the final score
    for (int x = 0; x < players.size(); x++) {
      getPlayer(x).displayScore();
    }
  }

  /**
   * Creates the game and runs it.
   */
  public static void main(String[] args) {

    Eights game = new Eights();
    game.playGame();

  }
}

EightsCard:

public class EightsCard extends Card {

  public EightsCard(int rank, int suit) {
    super(rank, suit);

  }

  /**
   * Checks whether cards match.
   */
  public boolean cardMatches(EightsCard that) {
    if (getSuit() == that.getSuit()) {
      return true;
    }
    if (getRank() == that.getRank()) {
      return true;
    }
    if (getRank() == 8) {
      return true;
    }
    return false;
  }

  /**
   * Gives the card score for a given card.
   * @return card score
   */
  public int scoreCard() {

    int rank = getRank();
    if (rank == 8) {
      return -20;
    } else if (rank > 10) {
      return -10;
    } else {
      return -rank;
    }
  }

  /**
   * Converts a Card into an EightsCard
   * @param card The card to be converted
   * @return The converted EightsCard
   */
  public EightsCard convert(Card card) {
    if (card != null) {
    EightsCard result = new EightsCard(card.getRank(), card.getSuit());
    return result;
    } 
    return null;
  }
}

EightsPlayer:

public class EightsPlayer extends Player {

  private CardCollection eightsHand;

  public EightsPlayer(String name) {
    super(name);
    eightsHand = new CardCollection(name);
  }

  public CardCollection getHand() {
    return eightsHand;
  }


  /**
   * Removes and returns a legal card from the player's hand.
   */
  public EightsCard play(Eights eights, EightsCard prev) {
    EightsCard ecard = new EightsCard(0, 0);
    ecard = ecard.convert(searchForMatch(prev));

    if (ecard == null) {

      ecard = drawForMatch(eights, prev);
      return ecard;
    }
    return ecard;
  }

  /**
   * Searches the player's hand for a matching card.
   */
  public Card searchForMatch(EightsCard prev) {
    for (int i = 0; i < eightsHand.size(); i++) {
      Card card = eightsHand.getCard(i);
      EightsCard ecard = new EightsCard(0,0);
          ecard = ecard.convert(card);
      if (ecard.cardMatches(prev)) {
        return eightsHand.popCard(i);
      }
    }
    return null;
  }

  /**
   * Draws cards until a match is found.
   */
  public EightsCard drawForMatch(Eights eights, EightsCard prev) {
    while (true) {
      EightsCard card = eights.draw();
      System.out.println(getName() + " draws " + card);
      if (card.cardMatches(prev)) {
        return card;
      }
      eightsHand.addCard(card);
    }
  }

  /**
   * Calculates the player's score (penalty points).
   */
  public int score() {
    int sum = 0;
    for (int i = 0; i < eightsHand.size(); i++) {
      Card card = eightsHand.getCard(i);
      int rank = card.getRank();
      if (rank == 8) {
        sum -= 20;
      } else if (rank > 10) {
        sum -= 10;
      } else {
        sum -= rank;
      }
    }
    return sum;
  }

  /**
   * Displays the player's name and score.
   */
  public void displayScore() {
    System.out.println(getName() + " has " + score() + " points");
  }

  /**
   * Displays the player's hand.
   */
  public void display() {
    getHand().display();
  }

}

Here is another version of the game which uses an extra class EightsCardCollection but avoids the clunky convert method:

You can also see the UML class diagrams associated with these two classes over on Stack Overflow.

Answer 1

An important property of code that is going to change in the future is testability; how expensive will it be to validate a later change?

One technique for keeping the costs low is to separate the code that needs to talk to the world outside the program (the imperative shell) from the code that doesn't (the imperative shell). Gary Bernhardt gives a good introduction to this idea in his talk Boundaries.

Input, output, time, random, environment... these are all examples of things that interact with the boundary.

Key takeaway: the functional core should have no dependencies on the imperative shell. Instead, the core describes the capabilities that it needs, and the shell provides them.

public static void printDeck(Card[] cards) {
  for (int i = 0; i < cards.length; i++) {
    System.out.println(cards[i]);
  }
}

This is core code that depends on the imperative shell -- the output capability. So the dependency arrow is pointing the wrong direction. Compare this with

public static void printDeck(Card[] cards, PrintStream out) {
  for (int i = 0; i < cards.length; i++) {
    out.println(cards[i]);
  }
}

I can easily isolate a test of this version, simply by providing a PrintStream that is local to the test.

Yes, there is going to be a binding somewhere to System.out; we typically arrange those bindings in the composition root -- Mark Seemann's blog has a lot of great material on this topic; read all of it.

In the case of a stand alone app like this one, that binding will usually happen in main

public static void main(String[] args) {

  Eights game = new Eights(System.out);
  game.playGame();
}

Or even

public static void main(String[] args) {
  EightsUI ui = new ConsoleEightsUI(System.in, System.out)
  Eights game = new Eights(ui);
  game.playGame();
}

Here's another example of the same idea, based on Random

public void shuffle() {
  Random random = new Random();
  for (int i = size() - 1; i > 0; i--) {
    int j = random.nextInt(i);
    swapCards(i, j);
  }
}

Bonus hint: new is a code smell -- seeing it coupled to logic is an immediate hint that something is going wrong. Misko Hevery wrote a lot about this.

So there are two problems here - one is that we are creating a capability that should be passed as an argument...

public void shuffle(Random random) {
  for (int i = size() - 1; i > 0; i--) {
    int j = random.nextInt(i);
    swapCards(i, j);
  }
}

By itself, this change makes the behavior deterministic - in the sense that if you know what state the random number generator is in, you can predict the result of the shuffle.

public static void main(String[] args) {
  Random random = new Random();
  EightsUI ui = new ConsoleEightsUI(System.in, System.out)
  Eights game = new Eights(ui, random);
  game.playGame();
}

The other problem is that we're being too specific -- shuffle doesn't actually care whether the stream of numbers is deterministic or not, it just cares that the value returned is within the defined bounds.

In Java 8, you might see the code written as...

interface BoundedInt {
    int boundedInt(int excludedUpperBound);
}

public void shuffle(BoundedInt bounded) {
  for (int i = size() - 1; i > 0; i--) {
    int j = bounded.boundedInt(i);
    swapCards(i, j);
  }
}

public static void main(String[] args) {
  Random random = new Random();
  BoundedInt boundedInt = random::nextInt;

  EightsUI ui = new ConsoleEightsUI(System.in, System.out)
  Eights game = new Eights(ui, boundedInt);
  game.playGame();
}

Because we've decoupled the capability we need from a specific provider, we can more precisely control what numbers are used during testing, without having to reverse engineer a pseudo random seed that gives us what we want.

BoundedInt zero = x -> 0;
BoundedInt upperBound = x -> x-1;

What's really happening here is that we are making a decision about how the required capability is implemented, and designing the code so that the visibility of that decision is limited. This approach was described by Parnas in 1972; see also Adrian Colyer's summary of that paper.

The code as written uses an implicit interface to communicate with the player. Make it explicit.

public String playerName() {
  System.out.println("Enter player name: ");
  String name = in.next();
  while (name == "") {
    name = in.nextLine();
  }
  return name;
}

There are a couple issues here. One is that println is the wrong abstraction level. The motivation isn't to write bytes, it's to prompt the user

interface UI {
    void promptUserForName();
}

Another way of thinking of this: we are sending messages to the user, and receiving messages back.

  String name = in.next();
  while (name == "") {
    name = in.nextLine();
  }
  return name;

This block confuses two different ideas that you may want to tease apart -- there's the capability of reading data provided by the user, and there's also the state machine that decides whether or not we need to try again. Watch Cory Benfield's Building Protocol Libraries The Right Way; the key points here

• recognizing that this interaction is a protocol
• understanding that the tool for implementing protocols is a state machine
  • an explicit state machine, please
• separating the I/O from the state machine (which is doing the bookkeeping).

That's the big picture. Some smaller things of note

public int compareValueALSD(Card that) 
public int compareValueAHSD(Card that)

These look like they are trying to become Comparator<Card>.

public boolean equals(Card that) {
  return rank == that.rank
      && suit == that.suit;
}

Respect the Object.equals contract -- see Effective Java, Item 10. Also, remember to override Object.hashCode. Alternatively, if what you want is a domain specific semantics, don't use the spelling equals. See, for instance, Value Objects like a Pro

final class Card {
    private Rank rank;
    private Suit suit;

    public Card(Rank rank, Suit suit) {
        this.rank = rank;
        this.suit = suit;
    }

    public boolean sameAs(Card anotherCard) {
        return rank.sameAs(anotherCard.rank) &&
                suit.sameAs(anotherCard.suit);
    }
}

There's a lot to be said for using domain specific types to isolate the decisions you have made about the in memory representations. Using primitive types like ints, Strings, and so on is a code smell called Primitive Obsession. Unfortunately, Java doesn't have a great toolkit for introducing new types; there are some libraries that can help if you are willing to introduce new dependencies.

public EightsCard convert(Card card) {
  if (card != null) {
  EightsCard result = new EightsCard(card.getRank(), card.getSuit());
  return result;
  } 
  return null;
}

If you really need to be managing nulls, I would consider teasing these apart into two different ideas

public static EightsCard convert(Card card) {
  if (null == card) {
    // This could reasonably be an AssertionError if you think
    // this _can't_ ever happen
    throw new IllegalArgumentException();
  }
  return new EightsCard(card.getRank(), card.getSuit());
}

public static Optional<EightsCard> convert(Optional<Card> card) {
    return card.map(EightsCard::convert);
}

Answer 2

Card

It took a while for me to understand why the null was there in RANKS, it's so that you don't have to do any off-by-one fixes when accessing the values. I'm not a big fan of that null though and would prefer a method called String getRankText() that would handle the off-by-one.

I would prefer SUITS as an enum { CLUBS, DIAMONDS, HEARTS, SPADES }

I don't think the compareValueALSD and compareValueAHSD belongs in the Card class. I would keep the comparing logic outside this class, as there is not one clear way of how to do the comparison.

The method printDeck also does not belong in the Card class.

---

CardCollection

Most of the class makes sense, but the shuffle method should take a Random object as input and avoid re-usage. There's also Collections.shuffle that you can call instead of doing the shuffling yourself. (I have not verified whether or not you have implemented Fisher-Yates shuffle correctly).

The display method also does not really belong in this class. A display method is part of your user interface, but this class is a model class. You could possibly have a method like public String getFullRepresentation() that could return a String for what to print, but I don't think that really belongs there either.

---

Deck

This entire class can be replaced by having one method:

public static CardCollection createDeck(String label) {
    CardCollection result = new CardCollection(label);
    for (int suit = 0; suit <= 3; suit++) {
        for (int rank = 1; rank <= 13; rank++) {
            result.addCard(new Card(rank, suit));
        }
    }
    return result;
}

This method can be in a class called DeckFactory, or in the class CardCollection or whereever you want essentially.

---

Player

Everything fine except for the display() method which I don't think is even needed, as any code wanting to print the hand can call getHand()

---

Eights

Now it's getting more interesting. I don't know your rules for this game but I'm used to it not allowing some cards as the first card in the discard pile. If an illegal card goes there you might want to put it back, reshuffle and put a new card there (and repeat while it is an illegal card).

This class currently contains both user interactions (System.out and System.in) and also quite a bit of game logic. I would recommend to separate these aspects into different classes. One class for handling game logic, and one for handling input/output.

Your nextPlayer logic is currently dealing with EightsPlayer instead of dealing with a int currentPlayerIndex. If you would use an index instead you could switch player by doing this.currentPlayerIndex = (this.currentPlayerIndex + 1) % this.players.size() and then use return this.players.get(this.currentPlayerIndex);

---

EightsCard

I consider the convert method here a code smell. It indicates that there is some design that you can improve.

The main reason for this class seems to contain two things: 1. Logic for checking if a card matches another card by suit or rank. 2. Give a score to the card.

Put the logic for checking card matches and scoring a card in another class instead. There are some different levels to do this at:

• Easiest: Put as static methods in Eights so that makes static boolean cardMatches(Card first, Card second) and static int cardScore(Card card).
• Tougher: Extract an interface for each of these two functionalities instead and inject them where they are needed. interface Scoring extends ToIntFunction<Card> {} and interface Matching extends BiPredicate<Card, Card> {}, then use Java 8 method references for the cardMatches method and the cardScore method to have an instance of these and then pass them to where they are needed - i.e. other methods that need to call the methods for matching cards and scoring cards.

---

EightsPlayer

This class contains a copy of the scoring logic previously found in EightsCard.

This class both extends Player and also have its own eightsHand which leads to duplicate data - which one should we use?

This class contains both business logic - drawForMatch - and acts as an AI in the play method. The logic for handling the situation if a player cannot make a move should be in the Eights class. Only call the play method if the player really can make a move. You could have a look at an implementation of Rock Paper Scissors Lizard Spock to get an idea.

---

Summary

I would recommend that the EightsPlayer and EightsCard classes should be removed through refactoring using the suggestions above.