Java - Design advice for simple game

Question

Hey,

I'm implementing a simple "Play your cards right" (otherwise known as higher/lower) game. In case you've not come across it before the rules are really simple. A single suit of cards (e.g. hearts) are used. One card is drawn at a time and the objective is to correctly guess if the face value of the next card will be higher or lower than the face value of the previously drawn card.

The logic for the game isn't particularly complex, I'm not worried about that. I've come up with a design, but I am not entirely happy with it. There are a few areas where I'm sure that it could be improved, and that's what I would like your advice on. Here's an interface for the class (comments for additional understanding, not real comments):

public interface PlayYourCardsRight {

/**
 * Get the number of cards remaining with higher face values than the previously
 * drawn card
 * @return
 */

public abstract int getNumberCardsHigher();

/**
 * Get the number of cards remaining with lower face values than the previously
 * drawn card
 * @return
 */

public abstract int getNumberCardsLower();

/**
 * Get all cards that have already been drawn in the order they were drawn in
 * 
 */

public abstract List<Card> getPlayedCards();

/**
 * Simple prediction algorithm  - if there are more cards left in the deck with
 * lower face values than the previous card, then predict 'Lower', if there 
 * are more cards left in the deck with higher face values then predict
 * 'Higher', if there are equal numbers of higher/lower cards pick 'higher' or     'lower'
 * at random
 *
 * Prediction is an Enum (Higher/Lower/None)
 *
 */

public abstract Prediction getPrediction();

/*
 * Draw the next card at random
 */

public abstract void nextRound();

/**
 * Specifiy what the next card should be
 * 
 * @param card
 */

public abstract void nextRound(Card card);

}

As you can see it is all fairly self explanatory and simple. Here are my issues:

I do not want the constructor to automatically draw a card. This means that initially there is no "previously drawn card". I have a NO PREDICTION value in the Prediction enum but, as there is no "previously drawn card" the getNumberCardsHigher() and getNumberCardsLower() methods can't return sane values (they cannot also return sane values when all the cards from the deck have been drawn).

Obviously, I could simply throw an exception, but that seems like overkill - especially as then all calls to the methods then have to be wrapped in try/catches. I'm also unhappy with returning a negative value, since that could easily lead to some errors if someone forgets/can't be bothered to check for them.

All suggestions welcome!

Answer 1

I would argue that both methods should return card.count when there is no previous card that has been drawn. There is the same number of lower and higher cards left, and for both there is card count more higher/lower cards than nothing. Your algorithm would then work and return a NO_PREDICTION.

Answer 2

I would personally recommend having an initial up card before the player does anything, as it doesn't make sense to have the player do anything before the first card is up, but I think "I do not want the constructor to automatically draw a card" meant you don't want to do that. If you don't want to do that I would make the functions throw exceptions, and have the code that calls them (the prediction function) special case the beginning of the game to return "no prediction" instead of even trying to call them. The end of the game is not a special case; both functions should return 0, as there are no cards higher or lower than the up card remaining in the deck

Also, there's no need to declare every function abstract in an interface, it's automatic and required

Answer 3

Personally I don't think throwing an unchecked exception in the case of argument checking is overkill at all--this is assuming that your code is asserting an invalid state (You should not call those methods with the object in that state, EVER).

I usually use an IllegalArgumentException to show that an argument has been passed in that does not fit the contract of the method call, and an IllegalStateException to show that the object is not in the state to handle the method call at this time.

Since they are both unchecked exceptions, you don't have to try/catch them, just let them bubble up, they do what exceptions are great at--they give you a stack trace and tell you exactly where your bug is including whoever called it incorrectly.

I usually use some kind of string by the way, in your case it might be:

throw new IllegalStateException("You cannot call this method until a card has been drawn");

Logically it just doesn't make sense to ask if the card is higher or lower than a card that doesn't exist.

Now, if your method actually THROWS that exception, then you have to go ahead and fix your code so that it does not call that method until after it has drawn a card--so you have to figure out how to draw your first card regardless.

Note: Exceptions are just for error detection, avoid using them for flow control. This means that you should not try to catch the exception and use it to draw a card then call again! Instead you should program in such a way that guarantees a card will be drawn before the first time that the methods are called.

Answer 4

I do not want the constructor to automatically draw a card. This means that initially there is no "previously drawn card". I have a NO PREDICTION value in the Prediction enum but, as there is no "previously drawn card" the getNumberCardsHigher() and getNumberCardsLower() methods can't return sane values (they cannot also return sane values when all the cards from the deck have been drawn).

I think the API confusion arises from the fact that your PlayYourCardsRight interface is trying to model two separate things: the game engine/rules and the deck of cards. I would move the state of the deck of cards and the remaining-card counting methods to a Deck class. I would change the API to getNumberCards[Higher/Lower](Card) and let the game engine specify which card you want to compare against rather than expecting the deck to remember which card was drawn last, which I would see as an element of the game's state, not the deck's.

And I highly recommend writing some JUnit tests. TDD helps to produce a cohesive, decoupled API.