Card arithmetic game

Question

There was a task on Stack Overflow which quite obviously was too broad (it didn't have a solution provided or anything!) which was the following:

Give an object oriented design for a game that has the following requirements:

1. It is a game of cards.
2. Cards can have four symbols - Hearts, Diamonds, Clubs, Leaves
3. Cards have numbers from A to 10. These are the values of the cards.
4. There is an operation associated with each symbol. They are Hearts - Sum, Diamonds - Subtract, Clubs - Multiply, Leaves - Divide.
5. Each Player would be dealt 3 cards.

6. The score of the player would be calculated by carrying out the particular operation on the card.

   E.g. if a player has 1 of hearts, 2 of Diamonds, 4 of hearts, the score would be calculated as +1 -2 +4, which is equal to 3.

7. Precedence of the operators would be given as input.

Give a design for the same.

Technically, I've just realized as I was writing this that I'm not providing the precedence of the operators as input and just evaluating them in order of the cards, but please excuse me for that.

Otherwise, I've implemented a possible solution to this given task:

public interface Operator {
 double evaluate(double carrier, double newValue);
}
public enum Operations implements Operator {
 ADDITION {
 @Override
 public double evaluate(double carrier, double newValue) {
 return carrier + newValue;
 }
 },
 SUBTRACTION {
 @Override
 public double evaluate(double carrier, double newValue) {
 return carrier - newValue;
 }
 },
 MULTIPLICATION {
 @Override
 public double evaluate(double carrier, double newValue) {
 return carrier * newValue;
 }
 },
 DIVISION {
 @Override
 public double evaluate(double carrier, double newValue) {
 return carrier / newValue;
 }
 }
}
public enum CardTypes {
 HEART(Operations.ADDITION),
 DIAMOND(Operations.SUBTRACTION),
 CLUB(Operations.MULTIPLICATION),
 LEAVES(Operations.DIVISION);
 private Operator operation;
 private CardTypes(Operator operation) {
 this.operation = operation;
 }
 public Operator getOperation() {
 return operation;
 }
}
public enum CardValues {
 _A(1.0),
 _2(2.0),
 _3(3.0),
 _4(4.0),
 _5(5.0),
 _6(6.0),
 _7(7.0),
 _8(8.0),
 _9(9.0),
 _10(10.0);
 private double value;
 private CardValues(double value) {
 this.value = value;
 }
 public double getValue() {
 return value;
 }
}
public class Card {
 private CardValues cardValue;
 private CardTypes cardType;
 public Card() {
 }
 public Card(CardValues cardValue, CardTypes cardType) {
 this.cardType = cardType;
 this.cardValue = cardValue;
 }
 public double evaluateCard(double currentScore) {
 Operator operator = cardType.getOperation();
 double value = cardValue.getValue();
 return operator.evaluate(currentScore, value);
 }
}
public class Deck {
 public static class EmptyDeckException extends Exception {
 public EmptyDeckException() {
 super("There are no cards left in the deck.");
 }
 }
 private List<Card> cards;
 public Deck() {
 this.cards = new ArrayList<>();
 for(CardTypes cardTypes : CardTypes.values()) {
 for(CardValues cardValues : CardValues.values()) {
 Card card = new Card(cardValues, cardTypes);
 cards.add(card);
 }
 }
 }
 public Card getCardFromDeck(Random random) throws EmptyDeckException {
 int currentDeckSize = cards.size();
 if(currentDeckSize == 0) {
 throw new EmptyDeckException();
 } else {
 return cards.remove(random.nextInt(currentDeckSize));
 }
 }
}
public class Player {
 private List<Card> cards;
 public Player() {
 this.cards = new ArrayList<>();
 }
 public void addCard(Card card) {
 this.cards.add(card);
 }
 public void removeCard(Card card) {
 this.cards.remove(card);
 }
 public double evaluateCards() {
 double score = 0;
 for(Card card : cards) {
 score = card.evaluateCard(score);
 }
 return score;
 }
}
public class Game {
 private List<Player> players;
 private Deck deck;
 private static final int CARD_COUNT = 3;
 private Random random;
 public Game() {
 this(new ArrayList<>());
 }
 public Game(List<Player> players) {
 this.players = players;
 this.deck = new Deck();
 this.random = new Random();
 }
 public void addPlayer(Player player) {
 this.players.add(player);
 }
 public void play() {
 try {
 for (Player player : players) {
 for(int i = 0; i < CARD_COUNT; i++) {
 player.addCard(deck.getCardFromDeck(random));
 }
 }
 } catch(Deck.EmptyDeckException e) {
 System.out.println(e.getMessage());
 }
 double[] results = new double[players.size()];
 for(int i = 0, n = players.size(); i < n; i++) {
 Player player = players.get(i);
 double result = player.evaluateCards();
 results[i] = result;
 System.out.println("Player " + (i+1) + " had a score of " + result);
 }
 System.out.println("");
 double max = results[0];
 int playerIndex = 0;
 for(int i = 1; i < results.length; i++) {
 if(max < results[i]) {
 max = results[i];
 playerIndex = i;
 }
 }
 int playerId = playerIndex + 1;
 System.out.println("Player " + playerId + " won with a score of " + max);
 }
}
public class Main {
 public void execute() {
 Game game = new Game();
 game.addPlayer(new Player());
 game.addPlayer(new Player());
 game.addPlayer(new Player());
 game.addPlayer(new Player());
 game.play();
 }
 public static void main(String[] args) {
 Main main = new Main();
 main.execute();
 }
}

And the result:

Player 1 had a score of 0.0
Player 2 had a score of 6.0
Player 3 had a score of 2.0
Player 4 had a score of -2.0
Player 2 won with a score of 6.0

I'd like to ask if there are any glaring flaws and ways to improve the code.

(I used double because there's division as a possible operator, and I figured it'd make no sense to have int and integer division in the evaluation of a card game.)

• Simon Forsberg: What would happen if the cards were just multiple and divide? Like *3 /2 /4 ?
• h.j.k.: @SimonAndréForsberg since the starting score is simply double score = 0; ... score = card.evaluateCard(score);, I think that just becomes 0... This challenge will be more fun if players get to decide their card order to maximize their points, just saying :D.
• Zhuinden: @h.j.k. Hmm, I haven't thought of that, but you could actually make a game that's actually fun out of that. :D

Answer 1

1. Operator

   Tip: If Java 8 is available for you, you should instead implement DoubleBinaryOperator. This will be useful if you're dealing with DoubleStream.

2. CardValues

   This is a personal preferences thing, but I'm not a fan of _ prefix in variable names, since this is not exactly a recommended Java naming convention. I understand your usage here is to let you conveniently name your enum values as numbers, so it's really up to you whether to stick with it, or go with ACE, ONE, TWO, ... etc.

   You also don't really need a value field for it, since you can always return (double)ordinal() + 1 (or ordinal() + 1.0, shorter due to the implicit casting of 1.0) from getValue().

   One more thing, enum types' names are usually singular, but since you have consistently used the plural form, I think this is fine too. Generally, consistency over convention, over chaos.

3. Card

   Since a Card must have both a value and type, you should remove your no-args constructor.

4. Deck

   You can probably inline new Card(...) below:

   // Card card = new Card(cardValues, cardTypes);
   // cards.add(card);
   cards.add(new Card(cardValues, cardTypes));
   

   You can also consider removing the temporary variable currentDeckSize inside getCardFromDeck(), since the repeated method call is likely to be optimal enough.

   There's also another approach you may want to consider, which is to pre-shuffle your Deck first (using Fisher-Yates perhaps, or one of the two available Collections.shuffle() method?), and then simply take from either the head or tail of your Deck. This would be more optimal in cases where you use an implementation that lets you perform an item removal without having to resize/shift its internal state, but then this is a minor point to consider for a simple 40-element Deck.

5. Game

   To avoid the double for-loop, you can already start checking for the highest score in the first loop:

   double maxScore = Double.MIN_VALUE;
   int winningId = -1;
   for(int i = 0, n = players.size(); i < n; i++) {
    double result = players.get(i).evaluateCards();
    if (result > maxScore) {
    maxScore = result;
    winningId = i;
    }
    System.out.println("Player " + (i + 1) + " had a score of " + result);
   }
   System.out.println("\nPlayer " + (winningId + 1) + " won with a score of " + maxScore);
   

• Zhuinden: Thanks for the answer! :D you're right, shuffling the collection could have been a cleaner way of handle getting a new card - because then I can ditch the Random dependency. I didn't think of that for some reason! I was debating the no-args constructor, for example if you use a Json parser or something it tends to require one, that's why I added it - but I admit it'd break if it's not parametrized, so that does seem like a terrible idea. I'll keep that in mind. What I'm not happy about is the play() method in general, it feels a bit too hackytacky. I should refactor it, any tips?
• h.j.k.: Which part of play() is still 'hackytacky' in your opinion, after avoiding the double for-loop? Assigning cards to players? For some games, cards dealt on a round-basis rather than player-bases, so an alternative is also to loop by CARD_COUNT as the outer loop first, and the players in the inner loop.
• h.j.k.: @Zhuinden well it is doing three things, so I suppose you can break it down to three methods...
• Zhuinden: Reasonable. There's one thing I'm wondering about though... which is the following - is it really the Card that should know how it is evaluated? Is it really the CardTypes that should get as a parameter what operator is bound to it? I'm thinking that if I'd want to abstract it, then a Card should know only its value and its type, but the classes responsible for the binding between types and operators should be its own class CardTypeOperatorBinding (or a map, really) and the evaluation should happen in external class as well, CardEvaluator - so that data would be separate from logic.
• h.j.k.: @Zhuinden sure, because you can also think of your game as a simplified Calculator where an operator and an operand is bundled together as a 'step', of sorts. By that train of thought, it wouldn't be far off to suggest some kind of a calculator/evaluator that reduces the Cards to a single value, just like evaluating a mathematical expression.