Kings Drinking Game

This Review Request evolves into this Review Request now with a custom UI and other changes.

I wrote a program to simulate the drinking card game Kings. This is my third Java Project. I didn't quite understand how to create the classes Card and Deck. I was struggling to create them so I went ahead and used someones code who had made them in a tutorial. But I think I get it now.

I plan on adding a main menu in the PSVM under a while-true loop with a similar structure to the one in playGame().

I would like a review of my code and some pointers on making it more efficient, cleaner, easier to read, or any other general pointers you have.

The explanation of the game is in the code comment box here:

/**
 * @author              :KyleMHB
 * Project Number       :0003
 * Project Name         :Kings
 * IDE                  :NETBEANS
 * Goal of Project      - 
 * Kings is a rule based drinking game using cards for 4+ players.
 * The Rules are read in from a rules.txt so that one can easily change the rules.
 * How the game works:
 * Players shuffle a deck of cards, place a glass between them and circle the
 * cards around the base of the glass.
 * The players then take turns picking cards, each card has its own rule associated to it.
 * Most importantly, there are 4 Kings, each time a King is picked, 
 * the player who picked it can pour as much of his/her drink into the glass between
 * them as they wish.
 * The game ends when the fourth and final King is picked.
 * The player to pick the final King must down the glass in the center of table.
 */

Card Class:

```
public static class Card {
private int rank, suit;

private String[] suits = { "Hearts", "Spades", "Diamonds", "Clubs" };
private String[] ranks = { "Ace", "2", "3", "4", "5", "6", "7", "8",
"9", "10", "Jack", "Queen", "King" };

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

Your code has exceptionally tight coupling between the business code (the actual implementation of the game) and the user interface code. This means I can't easily reuse your code e.g. to play it on the command line instead of the GUI you have implemented.

You can achieve this seperation by defining an interface for all UI operations. In your main, you can then decide which UI implementation you want to use.

interface UserInterface {
  public int     numberOfPlayers();
  public boolean playerWantsToDrawCard(int player);  // if false: skip
  public void    showCard(Card card, int player, boolean last);
}

Your existing methods would then be refactored into a GraphicalUserInterface implements UserInterface. Note that this is a different abstraction than yours – the implementation you have shown abstracts over what is shown, but hardcodes how this is done. I'd rather do this the other way round.

With that new interface, your playGame would be changed to:

private static void playGame(UserInterface ui) {
    int players = ui.numberOfPlayers();
    int kings = 0
    Deck deck = new Deck();

    while (!deck.isEmpty()) {
        for (int player = 0; player < players; player++) {
            // players do not have to draw a card
            if (!ui.playerWantsToDrawCard(player)) {
                continue;
            }

            Card drawnCard = deck.draw();

            if (drawnCard.getRank() == Rank.KING) {
                kings++;
            }
            ui.showCard(drawnCard, player, kings == 4 || deck.isEmpty());

            // exit if we've seen all kings
            if (kings == 4) {
               return;
            }
        }
    }
}

While this assumes some further changes (e.g. to the Deck API), the main point is that the business logic doesn't have to deal with UI directly. Other important changes are:

• I use a for loop to iterate through all players instead of obfuscating this through another while loop.

• I declare my variables in the tightest scope possible. The drawnCard is not needed outside the inner loop, so I declare it there. As a rule of thumb, you shouldn't declare variables without initializing them directly.

One thing that isn't immediately obvious, but I let the ui.showCard method figure out wether this card was a king, and wether this was the last king. The only hint I have to provide is whether this was the last card in the game.

One thing I haven't explained yet is Rank.KING. In your original code, you have hardcoded 12, which doesn't explain anything to a reader of the code. Currently, you specify the suit and rank of each card using integers. This is arguably wrong, and you should be using an enum:

enum Rank {
  ACE   ("Ace"),
  TWO   ("2"),
  THREE ("3"),
  FOUR  ("4"),
  FIVE  ("5"),
  SIX   ("6"),
  SEVEN ("7"),
  EIGHT ("8"),
  NINE  ("9"),
  TEN   ("10"),
  JACK  ("Jack"),
  QUEEN ("Queen"),
  KING  ("King");

  String name;

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

Ditto for Suit. Now our Card looks like

class Card {
  private Rank rank;
  private Suit suit;

  public String toString() {
    return rank.toString() + " of " + suit.toString();
  }

  ...
}

with the remaining methods having updated their types. Using enums allows for more type safety, and automatic additions like comparability and a iterable view of all possible values. This allows us to construct the deck like

cards = new ArrayList();
for (Suit suit : Suit.values()) {
  for (Rank rank : Rank.values()) {
    cards.add(new Card(suit, rank));
  }
}
Collections.shuffle(cards, new Random());

Btw, shuffling once is “random” enough, and additional shuffling does not increase randomness. If you need a true cryptography-grade entropy source, the builtin pseudo-random number generator which new Random() gives you is not sufficient. But for the purpose of this game, its usage is quite allright.

StackExchange Code Review Q#32281, answer score: 5