Acey Ducey game

Question

I'm a beginner in Java and want to improve and learn.

This is a game called "Acey Ducey":

The dealer shows you two cards. You decide whether you want the dealer to pick a third card or not. If the third card is between the two cards, then you win, otherwise you lose.

GitHub

This is the main file:

import model.*;
import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;

public class Main {
    public static void main(String[] args) {
        // load
        Player player = new Player();
        List<Card> deck = new ArrayList<>();
        for (CardColor color : CardColor.values()) {
            for (CardNumber number : CardNumber.values()) {
                Card card = new Card(number, color);
                deck.add(card);
            }
        }

        while(true) {
            System.out.println("---------------------");
            System.out.println("remaining cards " + deck.size());
            if (deck.size() < 2 ) {
                System.out.println("Game over. Not enough cards");
                break;
            }
            if (player.getMoney() <= 0) {
                System.out.println("Game over. Not enough money");
                break;
            }
            // pick two
            Card firstPick = Dealer.pickRandomCardFrom(deck);
            Card secondPick = Dealer.pickRandomCardFrom(deck);
            Card thirdPick = new Card();
            //show two cards
            System.out.println("First card ");
            System.out.println(firstPick.getCardNumber() + " of " + firstPick.getCardColor());
            System.out.println("----");
            System.out.println("Second card ");
            System.out.println(secondPick.getCardNumber() + " of " + secondPick.getCardColor());

            System.out.println("Do you want to set?");
            String userInput = getUserInput();
            if (userInput.equals("y")) {
                System.out.println("How much money do you want to bet?");
                System.out.println("You got that amount of money " + player.getMoney());
                int playerBet = Integer.parseInt(getUserInput());
                try {
                    player.withdrawMoney(playerBet);
                } catch (Exception e) {
                    System.out.println("You don't have enough money");
                    e.printStackTrace();
                    break;
                }
                thirdPick = Dealer.pickRandomCardFrom(deck);
                int compareThis = thirdPick.getCardNumberValue();
                int maxValue = Math.max(firstPick.getCardNumberValue(), secondPick.getCardNumberValue());
                int minValue = Math.min(firstPick.getCardNumberValue(), secondPick.getCardNumberValue());
                System.out.println("Result:");
                System.out.println(thirdPick.getCardNumber() + " of " + thirdPick.getCardColor());

                if (compareThis >= minValue && compareThis <= maxValue) {
                    System.out.println("You won!");
                    try{
                        player.depositMoney(2 * playerBet);
                    } catch (Exception e) {
                        System.out.println("Invalid amount of money");
                        e.printStackTrace();
                    }
                } else {
                    System.out.println("though luck!");
                }
            }
        }

    }

    private static String getUserInput() {
        Scanner scanner = new Scanner(System.in);
        return scanner.nextLine();
    }


}

In the folder "model" I got these classes:

Card

package model;

public class Card {
    private CardNumber number;
    private CardColor color;

    public Card() {};
    public Card(CardNumber number, CardColor color) {
        this.number = number;
        this.color = color;
    }

    public CardNumber getNumber() {
        return number;
    }

    public String getCardNumber() {
        return number.getCardNumber();
    }

    public int getCardNumberValue() {
        return number.getCardValue();
    }

    public void setNumber(CardNumber number) {
        this.number = number;
    }

    public CardColor getColor() {
        return color;
    }

    public String getCardColor() {
        return color.getCardColor();
    }

    public void setColor(CardColor color) {
        this.color = color;
    }


}

Card Color (=card suite)

package model;

public enum CardColor {
    HEART("Heart"),
    SPADE("Spade"),
    CLUB("Club"),
    DIAMOND("Diamond");
    private String color;
    private CardColor(String color) {
        this.color = color;
    }
    public String getCardColor() {
        return color;
    }
}

Card Number

package model;

public enum CardNumber {
    ACE("Ace", 1),
    TWO("Two", 2),
    THREE("Three", 3),
    FOUR("Four", 4),
    FIVE("Five", 5),
    SIX("Six", 6),
    SEVEN("Seven", 7),
    EIGHT("Eight", 8),
    NINE("Nine", 9),
    TEN("Ten", 10),
    JACK("Jack", 11),
    QUEEN("Queen", 12),
    KING("King", 13);
    private String number;
    private int value;
    private CardNumber(String number, int value) {
        this.number = number;
        this.value = value;
    }
    public String getCardNumber(){return number;}
    public int getCardValue() {return value;}
}

Dealer

package model;

import java.util.List;

public class Dealer {
    public static Card pickRandomCardFrom(List<Card> deck) {
        int randomNumber = (int)Math.floor(Math.random() * deck.size());
        Card pickedCard = deck.get(randomNumber);
        deck.remove(randomNumber);
        return pickedCard;
    }
}

Player

package model;

public class Player {
    private int money = 100;

    public int getMoney() {
        return money;
    }

    public void setMoney(int money) throws Exception {
        if (money > 0) {
            this.money = money;
        } else {
            throw new Exception();
        }

    }

    public void withdrawMoney(int amount) throws Exception {
        if (amount <= this.money) {
            this.money -= amount;
            return;
        }
        throw new Exception();
    }
    public void depositMoney(int amount) throws Exception {
        if (amount > 0) {
            this.money += amount;
            return;
        }
        throw new Exception();
    }
}

Answer 1

First of all, it's very well written and I can easily understand what's going on.

I also want to point out to another post which is similar to yours with a lot of good code, answers and comments: Object Oriented Design of Card Deck

However ...

What 'hurts' the most is the very long main method. A lot of things in the main method can be moved to separate types/methods, for instance, the loading of the Deck: Consider writing something like List<Card> deck = getCardDeck(). It's still quite clear what you should happen, but it's written / summarized in one line.

As others have mentioned, it's better to move the 'game itself' to its separate type. Why? Assume you want to provide a GUI for your game - what do you have to change? Yeah, a lot, because a lot of the code is not reusable. I think, with that in mind, you will have a total different approach of writing the game, its classes and methods and the control flows. And by how your posted code looks, and how you are already "thinking in objects", I think you can do that without me pointing out every single tiny thing I see - so I won't go into detail about what part of code you should refactor to where and why - which would really take a lot of time.

Now, some smaller thingies:

• // load: I do always mention this in nearly every answer I post here: Try not to write any comments at all. Make the code speak for itself, make it sexy (that's what we call it, when it's really well written). Actually it does speak for it self, it creates a new player, then creates a card deck, no need to explain that any further.
• The declaration of the three Cards firstPick, secondPick, thirdPick: I would name it something like firstSelectedCard, to make it clear, that this variable is a Card. Why? So it will be obvious, when I read it later in the code. Also, you declare thirdPick up there. Try to put it as close as possible to the point where you need it. In your case, you can declare it directly when you call Dealer.pickRandomCardFrom. Why? When you come across the pickRandomCardFrom-line, you will ask yourself "Why I am assigning a new value? What happend to it before?". Also, that might sound very stupid: You have to scroll up. Even if it's a "small thing to do", the process of scrolling up, "answering your question about a specific line of the code", scrolling down again and searching for the line you were before can break your concentration. And the more of those "focus breaks" in a piece of code, the harder to understand. If you put it closer to where it's actually needed, you will save the scrolling.
• int compareThis = thirdPick.getCardNumberValue(); Again: Move the declaration down.
• compareThis: That is really badly named. A variable should be named after what it represents, not what you should have to do with it.
• Card type: I'd make it immutable, meaning: Remove the default constructor and remove the setters. I don't think it's needed.
• Dealer.pickRandomCardFrom: The remove method of List actually does return the removed object, so you can directly write return deck.remove(randomNumber).
• Player exception handling. Usually you do the so called "guards" - a precondition, which must be satisfied, before the method is or can be executed - at the top of the method and then perform the actual method. Why? Well, it's usually more of a problem when you have more than one guard, which leads to quite unsexy nested if else blocks. So, it's a good habit to do it always.
• I really don't like to use return for anything else than returning a return value. Also: In setMoney you throw the Exception within the else block, in withdrawMoney, you return in the if block, and have no else block, at least be consistent.
• You throw an exception without an message, I would have at least expected something like "Not enough money". In general it's questionable if you should throw a Exception there, because an Exception shouldn't be used for controlling the flow of a routine. I would have provided more methods, for example for depositMoney, I would have provided something like a canDepositMoney method. And then call the actual depositMoney method, at the same time leave the guards, but throw a RuntimeException (if that happens, something's totally wrong, anyway).

Hope that helps ...

Answer 2

I'll just go over some aspect of your code, the one that I noticed first.

Bare exceptions

You use bare Exceptions for different types of errors. Better is to create specialized exceptions or re-user exsisting ones, and add useful information.

For example:

 public void setMoney(int money) throws IllegalArgumentException {
        if (money > 0) {
            this.money = money;
        } else {
            throw new IllegalArgumentException("'money' needs to be positive");
        }

    }

Printing stackstraces

You also show the user the stacktrace if that happens. I would not show the user this.

Possible misuse of methods

• withdrawMoney allows for negative amounts.
• pickRandomCardFrom fails if the Deck is empty (you currently have a check in the main() method, but it is safer to also check here)

Answer 3

Why isn't Deck an object?

It seems the Dealer functionality replaces that object. I would rather have a Deck object with functionalities like restore(), shuffle(), draw(), hasCards() etc.

The added value is that this Object would take care of many constraints like not drawing when empty whereas with a simple list you have the add, set & remove methods exposed.

Tip: When I see a static method doing stuff on its unique parameter, I instantly know it should have been encapsulated in said parameter.

Something like this:

public class Deck{

    private final List<Card> deck = new ArrayList<Card>();
    private final List<Card> initialContents;

    public Deck(List<Card> initialContents){
        this.initialContents = initialContents;
        deck.addAll(initialContents);
    }

    public Card draw() throws EmptyDeckException(){
        if(deck.isEmpty()){
            throw new EmptyDeckException("Cannot draw a card from an empty deck!");
        }
        deck.remove(0);
        return pickedCard;
    }

    public boolean hasCards(){
        return !deck.isEmpty();
    }

    public void shuffle() {
        Collections.shuffle(deck);
    }

    public void restore() {
        deck.clear();
        deck.addAll(initialContents);
    }
}

You'll notice I'm using a custom exception as @RobAu pointed out. You'll have to handle it in the relevant parts of the program.

I recommend not relying on the exception but rather checking with hasCards() if the operation is legal. The exception should only be thrown if you thought you could draw, but can't for unforeseen reasons (also called exceptions!).

---

All the game code is in the main() method

Simply looking at a method signature/class should tell you what it does.

What does a public static void main(String[] argc) do? What does a Main class do? You can't tell.

The Java convention says a main() method starts a Java program. Your function does so much more. You should extract these business actions to appropriate methods/objects.

How about a AceyDucey class ?

Your main() should look like:

public static void main(String[] argc){
    new AceyDucey().begin();
}

Then you can break down the game flow :

public class AceyDucey() {

    public void begin(){
        while(player.hasMoney()) {
             playTurn();
        }
    }

    public void playTurn() {
        ...
    }
}

this makes much more sense even to the untrained eye.

---

Reuse your Objects

Calling Math.random() many times isn't very good, try instancing a static final Random object once instead (look around on SO for valid reasons). In the above example, I simply used Collection.shuffle() because that's what a dealer physically does.

In getUserInput(), you're creating a Scanner every time. There is no need for that, just make one once, and reuse it. You couldn't do it nicely with that Main class with only static methods. But if you make an AceyDucey class, you can make the Scanner a class attribute, like so:

public static void main(String[] argc){
    new AceyDucey(new Scanner(System.in)).begin();
}