15
\$\begingroup\$

edit: After receiving the great feedback, I have tried to make my code better. The new version can be seen here: Simple Blackjack game in console (update)

This is one of my first finished projects and I like to be sure that I am doing things right. Especially the part of modelling the classes and who has to do what. Also, I am not so sure about model/view/controller separation. Right now I have the Person class handling some of the view (the printCards() method) and the Player class handling some of the control (the wantToHit() method) and I'm not sure if this is alright or if this could be improved.

Blackjack.java

package nl.rickhurkens.blackjack;

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

public class Blackjack {
    private static Blackjack game;
    private ArrayList<Person> players;
    private Deck deck;

    public Blackjack() {
        initializeGame();
        dealCards();
        takeTurns();
        declareWinner();
        playAgain();
    }

    public static void main(String[] args) {
        game = new Blackjack();
    }

    private void initializeGame() {
        deck = new Deck();
        players = new ArrayList<Person>();
        players.add(new Player());
        players.add(new Dealer());

        System.out.println("Welcome to a new Blackjack game!");
    }

    private void dealCards() {
        for (Person player : players) {
            deck.dealInitialCards(player);
            player.printCards(player.getName().equals("You"));
        }
    }

    private void takeTurns() {
        for (Person player: players) {
            boolean endOfTurn = false;
            while(!endOfTurn) {
                player.printCards(true);
                boolean hit = player.wantToHit();
                if (hit) {
                    deck.dealCard(player);
                    System.out.println(player.getName() + " drew a card.\n");
                    if (player.getTotal() > 21) {
                        endOfTurn = true;
                        System.out.println(player.getName() + " died.\n");
                    }
                } else {
                    endOfTurn = true;
                    System.out.println(player.getName() + " stayed.\n");
                }
            }
        }
    }

    private void declareWinner() {
        byte highest = -1;
        byte topPlayer = -1;
        for (byte i = 0; i < players.size(); i++) {
            String name = players.get(i).getName();
            byte total = players.get(i).getTotal();

            System.out.println(name + (name.equals("You") ? " have " : " has ") + "a total of " + total + ".");

            if (total > highest && total <= 21) {
                highest = total;
                topPlayer = i;
            }
            if (total == highest && name.equals("The dealer")) {
                topPlayer = i;
            }
        }
        if (topPlayer == -1) {
            System.out.println("Everyone is dead. Nobody wins.");
        } else {
            System.out.println(players.get(topPlayer).getName() + " wins!");
        }
    }

    private void playAgain() {
        System.out.print("\nPlay again? \"y\" / \"n\": ");
        Scanner keyboard = new Scanner(System.in);
        while(true) {
            String input = keyboard.next();
            if (input.equals("y")) {
                game = new Blackjack();
            } else if (input.equals("n")) {
                System.exit(0);
            } else {
                System.out.print("Please type \"y\" or \"n\": ");
            }
        }
    }
}

Person.java

package nl.rickhurkens.blackjack;

import java.util.ArrayList;
import java.util.List;

public abstract class Person {
    private List<Card> cards;
    protected String name;
    protected byte total;

    public Person() {
        cards = new ArrayList<Card>();
        total = 0;
    }

    public Card receiveCard(Card card) {
        cards.add(card);
        total += card.getValue();
        return card;
    }

    public abstract boolean wantToHit();

    public byte getTotal() {
        return total;
    }

    public String getName() {
        return name;
    }

    public void printCards(boolean showAll) {
        System.out.print("\n" + name + (name == "You" ? " have a" : " has a") + (cards.get(0).getValue() == 8 || cards.get(0).getValue() == 11 ? "n " : " ") + cards.get(0).toString());
        for (int i = 1; i < cards.size(); i++) {
            if (showAll) {
                System.out.print(" and a" + (cards.get(i).getValue() == 8 || cards.get(i).getValue() == 11 ? "n " : " ") + cards.get(i).toString());
            } else {
                System.out.println(" and a hidden card.");
            }
        }
        if (showAll) {
            System.out.println(".\n" + name + (name == "You" ? "r" : "'s") + " total is " + total + ".");
        }
    }
}

Player.java

package nl.rickhurkens.blackjack;

import java.util.Scanner;

public class Player extends Person {
    private Scanner keyboard;

    public Player() {
        super();
        name = "You";
        keyboard = new Scanner(System.in);
    }

    public boolean wantToHit() {
        System.out.print("\nWould you like to \"hit\" or \"stay\": ");
        while (true) {
            String input = keyboard.next();
            if (input.equals("hit")) {
                return true;
            } else if (input.equals("stay")) {
                return false;
            } else {
                System.out.print("Type \"hit\" or \"stay\": ");
            }
        }
    }
}

Dealer.java

package nl.rickhurkens.blackjack;

import java.util.concurrent.ThreadLocalRandom;

public class Dealer extends Person {

    public Dealer() {
        super();
        name = "The dealer";
    }

    public boolean wantToHit() {
        if (total < 16) return true;
        if (total > 16) return false;
        if (ThreadLocalRandom.current().nextInt(0,2) == 0) return true;
        return false;
    }
}

Deck.java

package nl.rickhurkens.blackjack;

import java.util.concurrent.ThreadLocalRandom;

public class Deck {

    public Card drawNextCard() {
        int value = ThreadLocalRandom.current().nextInt(2,12);
        return new Card(value);
    }

    public void dealCard(Person person) {
        person.receiveCard(drawNextCard());
    }

    public void dealInitialCards(Person person) {
        dealCard(person);
        dealCard(person);
    }
}

Card.java

package nl.rickhurkens.blackjack;

public class Card {
    private int value;

    public Card(int value) {
        this.value = value;
    }

    public String toString() {
        return "" + value;
    }

    public int getValue() {
        return value;
    }
}
\$\endgroup\$
  • \$\begingroup\$ If player and dealer bust the dealer wins. Random 2, 12 does not represent a deck. Does not allow an ace to be 1 or 11. \$\endgroup\$ – paparazzo Sep 5 '17 at 14:59
12
\$\begingroup\$

Avoid singletons

    private static Blackjack game;

and later

    public Blackjack() {
        initializeGame();
        dealCards();
        takeTurns();
        declareWinner();
        playAgain();
    }

    public static void main(String[] args) {
        game = new Blackjack();
    }

What this means is that you can only ever have one game. Also, you can't pause a game in the middle and do something else. If you make a Blackjack object, you play a game to completion.

Consider

    public static void main(String[] args) {
        do {
            Blackjack game = Blackjack.initializeGame();
            game.play();
        } while (Blackjack.shouldPlayAgain());
    }

    public void play() {
        dealCards();
        takeTurns();
        declareWinner();
    }

We don't even need a constructor now. We change initializeGame to be a factory method that produces a Blackjack object.

The new method play takes care of the central portion of what you were doing in the constructor.

Now we can move 'main' out of the Blackjack class if we want. Prior to this, it had to be there because it was tightly coupled with the class. Or we could move the current logic out of main and into something else.

Now we can see easily that we keep playing until we're done. We don't have to hunt down a recursive call.

The constructors never actually finish until the end of the program. They keep piling on the stack until someone eventually says no and stops execution. This pattern may work in blackjack, where people are unlikely to play millions of games. But it won't work in other contexts where a billion iterations is a reasonable possibility.

Prefer interfaces to implementations

    private ArrayList<Person> players;

This could be

   private List<Person> players = new ArrayList<>();

Now we aren't stuck with this single implementation. We can change easily or provide the ability to override this choice. Perhaps the constructor or factory method might take a list of players as a parameter.

Moving the initialization here means that you don't need to do it in the constructor. In fact, you don't need a constructor.

Single responsibility principle

Your Person class covers two things.

  1. It represents the player or dealer.
  2. It represents the hand of cards that that player or dealer has.

You should split those roles.

Constructors

    public Player() {
        super();
        name = "You";
        keyboard = new Scanner(System.in);
    }

You never need to call the no argument super constructor manually. The compiler will do that for you automatically.

If you set the name by default, this means that you can only play the game with a player and a dealer. You can never play with three or more participants.

Again, you don't have to initialize the keyboard in the constructor. You can do so at declaration time. In fact, you may want to do so statically. Then you aren't creating and destroying scanners all the time.

Consider

    public Player() {
        this("You");
    }

    public Player(String name) {
        this.name = name;
    }

Now there's a default or you can specify a name. And the this( means that the no-argument constructor calls the one argument constructor. So both get any benefits of processing.

When is an if overkill?

    public boolean wantToHit() {
        if (total < 16) return true;
        if (total > 16) return false;
        if (ThreadLocalRandom.current().nextInt(0,2) == 0) return true;
        return false;
    }

Consider

    public static final CUSP = 16;

    public boolean wantToHit() {
        if (total == CUSP) {
            return ThreadLocalRandom.current().nextBoolean();
        }

        return total < CUSP;
    }

First, the original could do up to three comparisons. This version only does two, although it always does two.

This turns four returns into two by returning the results of comparisons directly rather than testing the comparison and picking a return value.

I replaced the magic number 16 with a constant. We could call it something else, e.g. BORDER or DECISION_POINT, but CUSP is a correct word.

I'm not convinced that you need ThreadLocalRandom here. Certainly this code is not running multiple threads. It's not clear to me why you would need it to do so.

The nextBoolean method already returns true/false on an evenly distributed basis. You don't need to reimplement it.

Not a regular deck

    public Card drawNextCard() {
        int value = ThreadLocalRandom.current().nextInt(2,12);
        return new Card(value);
    }

As written, this should be static, as it doesn't depend on the values of the current deck.

    public static Card drawNextCard() {

But that's not the big problem. This code should depend on the deck values. A deck has a certain selection of cards in it. So this isn't a real deck. Perhaps that's your intent. Counting cards won't work on this deck, as the cards that you have don't affect the odds of cards being selected.

Another issue is that blackjack card values are not evenly distributed from 2 to 11. There are four times as many cards that have the value 10 as any other value. Consider using a different data structure. For example, you could have

enum Rank {

    ACE(11),
    TWO(2),
    THREE(3),
    FOUR(4),
    FIVE(5),
    SIX(6),
    SEVEN(7),
    EIGHT(8),
    NINE(9),
    TEN(10),
    JACK(10),
    QUEEN(10),
    KING(10);

    private final int value;

    public Rank(int value) {
        this.value = value;
    }

    public Rank chooseRank() {
        Rank[] ranks = values();
        return ranks[(int) (Math.random() * ranks.length)];
    }

    public int getValue() {
        return value;
    }

}

This uses Math.Random rather than ThreadLocalRandom, but you could use ThreadLocalRandom.

Don't simulate reflection

            player.printCards(player.getName().equals("You"));

This relies on the name being "You". Translate it to another language or give it a regular name, and it breaks.

            player.printCards();

If you put the Dealer and Player versions correctly, this will do the right thing.

In Person:

    public void printAllCards() {
        System.out.print("\n" + name + (name == "You" ? " have a" : " has a") + (cards.get(0).getValue() == 8 || cards.get(0).getValue() == 11 ? "n " : " ") + cards.get(0).toString());
        for (int i = 1; i < cards.size(); i++) {
            System.out.print(" and a" + (cards.get(i).getValue() == 8 || cards.get(i).getValue() == 11 ? "n " : " ") + cards.get(i).toString());
        }

        System.out.println(".\n" + name + (name == "You" ? "r" : "'s") + " total is " + total + ".");
    }

    public abstract void printCards();

In Player:

    @Override
    public void printCards() {
        printAllCards();
    }

In Dealer:

    @Override
    public void printCards() {
        Card card = cards.get(0);
        System.out.print("\n" + name + " has a" + (card.getValue() == 8 || card.getValue() == 11 ? "n " : " ") + card.toString());
        System.out.println(" and a hidden card.");
    }

Now you don't need reflection. it will call the right method for you.

                player.printCards(true);

This becomes

                player.printAllCards();

You point out that this is a View method. That's true, but I don't think it's the worst problem. Why does printCards need to know that the names of aces and eights start with vowels? What would happen if you were using different names?

Consider moving these methods into bridge classes. Something like HandDisplay could know the language and what cards are there. Each CardDisplay could know the language and the rank of the card. And we could tell CardDisplay that we want the version of the card name with the noun marker before it. Then it could figure out which one is appropriate based on the name rather than relying on the relationship between the value and the name.

\$\endgroup\$
  • \$\begingroup\$ Somehow it felt bad to me to immediately initialize an object in an attribute decleration. I think all examples in one book I read did not do this. So actually there is nothing wrong with that at all? \$\endgroup\$ – Rick Sep 5 '17 at 13:11
11
\$\begingroup\$

I wanna start of by saying that I think this code is to a very high standard for someone just completing their first project, the game is overall well put together.

So for this review I'm going to primarily focus on your question regarding the MVC separation.

At the moment, your Blackjack class has too many responsibilities. From looking at the code a little bit I can pick out a few.

  • Acts as the "View", in this case your view is printing things to the console.
  • Acts as the "Game", i.e. it handles the Deck, the Players, the Cards etc.
  • Acts as the "Runner" for the program, so the main method / driver

So I think we could break this up into 3 different classes.

Let's talk about what our view can look like

public interface BlackJackView {
    void welcome();
    void displayPlayerHand(Player player);
    void displayWinner();
    ...
}

I went with an interface because the goal here is we want to be able to make multiple different views, for the sake of example, lets say we're writing 3 different implementations. ConsoleView, SwingView and JXView.

public class ConsoleView implements BlackJackView {
    // has an instance of a blackjack to check game state and then update the view
    private final BlackJack blackJack;

    public ConsoleView(BlackJack blackJack){
        this.blackJack= blackJack;
    }

    @Override
    public void welcome(){
        System.out.println("Welcome to a new Blackjack game!");
    }

    @Override
    void displayPlayerHand(Player player){
        // code that iterates through and prints player hand
    }

    @Override
    void displayWinner(){
        // your declareWinner() code
    }

    // all the rest of the methods

}

Now I'm sure someone who know a lot more about Swing or JavaFx could give some basic examples of how to implement those, but lets look at how it could be instantiated.

BlackJack blackJack = new BlackJack();
BlackJackView view= new ConsoleView(blackJack);
// BlackJackView view= new SwingView(blackJack);
// BlackJackView view= new JavaFxView(blackJack);
Game game = new Game(blackJack, view);
game.start();

The Game class here could be in charge of finding out what the game state is from the BlackJack object, and then calling the corresponding view method from the BlackJackView object.

Consider the case where you want to check the display.

public class Game {
    private final BlackJack blackJack;
    private final BlackJackView view;

    public Game(BlackJack blackJack, BlackJackView view){
        this.blackJack = blackJack;
        this.view = view;
    }

    public void start(){
        view.welcome();
        blackJack.init();
        ...
        view.displayWinner();     
    }
}

So no the Game class is your "Runner" and is used in your main method, it contains an instance of a BlackJackGame and a BlackJackView.

Hopefully you found this review helpful, keep up the good work!

\$\endgroup\$
  • \$\begingroup\$ Should the argument in your view initializations be 'blackJack' instead of 'game'? That would make more sense to me. \$\endgroup\$ – Rick Sep 5 '17 at 13:06
  • \$\begingroup\$ yes you're right, thanks for spotting that! Will edit now. \$\endgroup\$ – chatton Sep 5 '17 at 13:09
3
\$\begingroup\$

There's a lot of good advice here so far, but I'd like to turn our attention to the Deck class, and in particular the methods for dealing cards:

public void dealCard(Person person) {
    person.receiveCard(drawNextCard());
}

public void dealInitialCards(Person person) {
    dealCard(person);
    dealCard(person);
}

By using Person here, you've created a dependency from Deck to Person (that is, Deck depends on Person). Now, dependencies aren't inherently bad (you'd be hard-pressed to write a non-trivial program without them!), but unnecessary dependencies make the code hard to maintain for no reason, so let's get rid of them. Furthermore, why is a deck dealing cards? I'd add a dealCard method in Blackjack, like so:

private void dealCard(Person person) {
    Card card = deck.drawNextCard();
    person.receiveCard(card);
}

Now, replace Deck.dealCard with that, and replace Deck.dealInitialCards in the same way.

Good luck!

\$\endgroup\$
2
\$\begingroup\$

Your code is well-structured and nice to read.

I would add the @Override annotation to the wantToHit() methods. You can benefit from early compiler checking. (see also StackOverflow).

You can also add an explicit final keyword to your method parameters to clarify they will not be changed.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.