2
\$\begingroup\$

A couple of days ago I posted the code for my Simple Blackjack console game. I have got some good reviews and since then I have been trying to improve the code. That previous question can be seen here: Simple Blackjack game in console

I think I have treated every issue that was put up by the previous reviewers. Besides that, I tried to implement the MVC pattern better, but this confused me a lot. I think it has made parts of my code messier than they were before.

  • I have given the Blackjack class less functions (moved most of them to the new ConsoleView and BlackjackController classes.
  • I have changed to Card class to be an enum, so the available cards better represents an actual deck.
  • I have split off a Hand class from the Person class.
  • Added HandDisplay to help with describing the contents of a Hand in plain English.
  • Changed some smaller stuff.
  • I also added comments to most every class and method.

BlackjackController.java

package blackjack;

/**
 * The controller of the Blackjack game.
 * For each new game it creates an instance of {@link Blackjack}
 * and one instance of a {@link ConsoleView}.
 * @author Rick
 */
public class BlackjackController {
    private Blackjack blackjack;
    private BlackjackView view;

    /**
     * Creates an instance of this class and calls {@link #run()}.
     * @param args the default parameter; no arguments needed
     */
    public static void main(String[] args) {
        BlackjackController game = new BlackjackController();
        game.run();
    }

    /**
     * Runs the game in chronological order.
     */
    private void run() {
        do {
            playOneGame();
        } while (view.wantToPlayAgain());
        view.thanksForPlaying();
    }

    private void playOneGame() {
        blackjack = Blackjack.initializeGame();
        blackjack.addPlayers("Player 1");
        blackjack.dealInitialCards();

        view = new ConsoleView(blackjack);
        view.welcome();
        for (Person person : blackjack.getPlayers()) {
            view.showInitialHand(person);
        }
        for (Person person : blackjack.getPlayers()) {
            letPersonPlayItsTurns(person);
        }
        for (Person person : blackjack.getPlayers()) {
            view.displayTotal(person);
        }
        Person winner = blackjack.getWinner();
        view.showWinner(winner);
    }

    private void letPersonPlayItsTurns(Person person) {
        view.showHand(person);

        boolean playOn = true;
        while (playOn) {
            String userInput = "";
            playOn = false;
            do {
                userInput = view.askForTurn(person);
                if (userInput.equals("hit")) {
                    playOn = true;
                }
            } while (!(userInput.equals("hit") || userInput.equals("stay")));

            if (playOn) person.wantsToHit();
            view.showHand(person);
        }
    }

}

BlackjackView.java

package blackjack;

/**
 * The interface to be implemented for creating a
 * view for the {@link Blackjack} game.
 * @author Rick
 */
public interface BlackjackView {

    /**
     * Displays the welcome message and shows the
     * initial hands (by calling {@link #showHand(Person)}) 
     * of all the players.
     */
    public void welcome();

    /**
     * Displays the initial {@link Card} instances in a {@link Person}
     * 's {@link Hand} as it is viewable for the {@link Player}.
     * @param person The {@link Person} to show the {@link Hand}
     * 's contents of.
     */
    public void showInitialHand(Person person);

    /**
     * Displays the {@link Card} instances in one {@link Person}
     * 's {@link Hand} and displays the total value of that
     * {@code Hand}.
     * @param person The {@link Person} to show the {@link Hand}
     * 's contents of.
     */
    public void showHand(Person person);

    /**
     * Asks a {@link Person} if it wants to hit or stay.
     * @param person the {@link Person} the question is asked to
     * @return a {@link String} "hit" or "stay"
     */
    public String askForTurn(Person person);

    /**
     * Displays the total value of a {@link Person}'s {@link Hand}
     * @param person the {@Person} whose total will be displayed
     */
    public void displayTotal(Person person);

    /** 
     * Shows who the winner is
     * @param person the winning {@link Person}
     */
    public void showWinner(Person person);

    /**
     * Asks if the user wants to play again
     * @return {@code true} if the user wants to play again,
     * {@code false} if he doesn't
     */
    public boolean wantToPlayAgain();

    /**
     * Displays a message thanking the user for playing after
     * he says he doesn't want to play again.
     */
    public void thanksForPlaying();
}

ConsoleView.java

package blackjack;

import java.util.Queue;
import java.util.Scanner;

/**
 * The view of this game in a console window.
 * @author Rick
 * @since 1.5
 */
public class ConsoleView implements BlackjackView{
    private Blackjack blackjack;
    private Scanner keyboard = new Scanner(System.in);

    /**
     * Constructs a new view for the given {@link Blackjack} instance.
     * @param blackjack the {@link Blackjack} instance for this view.
     */
    public ConsoleView(Blackjack blackjack) {
        this.blackjack = blackjack;
    }

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

    @Override
    public void showInitialHand(Person person) {
        Hand hand = person.getOpenHand();
        Queue<Card> cards = hand.getCards();
        System.out.println(person.getName() + " has " + HandDisplay.cardsToString(cards) + ".\n");
        return;
    }

    @Override
    public void showHand(Person person) {
        Hand hand = person.getHand();
        Queue<Card> cards = hand.getCards();
        System.out.println(person.getName() + " has " + HandDisplay.cardsToString(cards) + ".");
        System.out.println("That makes a total of " + hand.getTotal() + ".\n");
    }

    public String askForTurn(Person person) {
        System.out.print("Do you want to 'hit' or 'stay'? ");
        String aiResponse = person.autoRespondWantToHit();
        if (aiResponse.length() > 0) {
            System.out.print(aiResponse + "\n");
            return aiResponse;
        }
        return keyboard.nextLine();
    }

    public void displayTotal(Person person) {
        System.out.println(person.getName() + "'s total is " + person.getHand().getTotal() + ".");
    }

    public void showWinner(Person person) {
        System.out.println(person.getName() + " is the winner!\n");
    }

    public boolean wantToPlayAgain() {
        System.out.print("\nDo you want to play again? 'y'/'n': ");
        String input = keyboard.nextLine();
        if (input.equals("y") || input.equals("n")) {
            return (input.equals("y"));
        }
        return wantToPlayAgain();
    }

    public void thanksForPlaying() {
        System.out.println("\nThanks for playing!");
    }
}

Blackjack.java

package blackjack;

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

/**
 * Business class of the Blackjack game.
 * @author Rick
 * @since 1.5
 */
public class Blackjack {
    private List<Person> players = new ArrayList<Person>();
    private Deck deck = new Deck();

    /**
     * @return a {@link List} of every {@link Person} currently in the game, including the {@link Dealer}.
     */
    public List<Person> getPlayers() {
        return players;
    }

    /**
     * @return the {@link Deck} of cards that is currently in use.
     */
    public Deck getDeck() {
        return deck;
    }

    /**
     * Factory method to get a new instance of a Blackjack game
     * @return a Blackjack instance
     */
    public static Blackjack initializeGame() {
        return new Blackjack();
    }

    /**
     * Adds {@link Player} instances to the {@link List} of players currently in the game.
     * @param playerNames a {@link String} array of the names of the players to be added
     */
    public void addPlayers(final String... playerNames) {
        players.add(new Dealer(this));
        for (String playerName : playerNames) {
            players.add(0, new Player(playerName, this));
        }
    }

    /**
     * Takes the a {@link Card} from the {@link Deck} and gives it to a {@link Person}.
     * @param person The {@link Person} to receive the {@link Card}.
     */
    public void dealCard(Person person) {
        Card card = deck.drawNextCard();
        person.receiveCard(card);
    }

    /**
     * Gives two {@link Card} instances to each {@link Person} in the game.
     */
    public void dealInitialCards() {
        for(Person person : players) {
            dealCard(person);
            dealCard(person);
        }
    }

    /**
     * Calculates which player is the winner according to the following rules:
     * If the dealer busts, the player wins.
     * If the dealer and player tie but don't bust, the dealer wins.
     * @return the {@link Person} who won the game
     */
    public Person getWinner() {
        int highest = 0;
        Person topPlayer = players.get(0);
        for (Person player : players) {
            int total = player.getHand().getTotal();
            if (total >= highest && total <= 21) {
                highest = total;
                topPlayer = player;
            }
        }
        return topPlayer;
    }
}

Deck.java

package blackjack;

import java.util.LinkedList;
import java.util.NoSuchElementException;
import java.util.Collections;

/**
 * A deck that contains a list of {@link Card} instances.
 * Can contain four references to each {@code Card} rank.
 * @author Rick
 */
public class Deck {
    private LinkedList<Card> cards = new LinkedList<Card>();

    /**
     * Creates a new {@code Deck} with all 52 cards.
     */
    public Deck() {
        fillDeckWithAllCards();
    }

    /**
     * @return a {@link LinkedList} containing all cards in this deck.
     */
    public LinkedList<Card> getCards() {
        return cards;
    }

    /**
     * Takes the next {@link Card} reference off the deck. 
     * If the deck is empty, fills it up with all 52 cards 
     * again.
     * @return the first {@link Card} reference on the deck.
     */
    public Card drawNextCard() {
        try {
            return cards.remove();
        } catch (NoSuchElementException e) {
            fillDeckWithAllCards();
            return drawNextCard();
        }
    }

    /**
     * Fills up the {@link LinkedList} with 4 references to
     * each {@link Card} rank and shuffles the deck.
     */
    private void fillDeckWithAllCards() {
        for(int i = 0; i < 4; i++) {
            for(Card card : Card.values()) {
                cards.add(card);
            }
        }
        shuffle();
    }

    /**
     * Shuffles the deck.
     */
    public void shuffle() {
        Collections.shuffle(cards);
    }
}

Person.java

package blackjack;

/**
 * A {@code Person} is someone who sits at the table to play
 * Blackjack.
 * @author Rick
 */
public class Person {
    private final Blackjack blackjack;
    private final String name;
    private Hand hand = new Hand();

    /**
     * Constructs a Person with the name "Unnamed Person".
     * @param blackjack the instance of the game this {@code 
     * Player} is playing
     */
    public Person(final Blackjack blackjack) {
        this("Unnamed Person", blackjack);
    }

    /**
     * Constructs a Person with the given name.
     * @param name the name this Person is going to get as a 
     * {@link String}, preferably in Title case.
     * @param blackjack the {@link Blackjack} instance that called
     * this constructor
     */
    public Person(final String name, final Blackjack blackjack) {
        this.name = name;
        this.blackjack = blackjack;
    }

    /**
     * @return the name as a {@link String} exactly as is
     */
    public String getName() {
        return name;
    }

    /**
     * @return the {@link Hand} this player currently has
     */
    public Hand getHand() {
        return hand;
    }

    /**
     * @return the part of the {@link Hand} that is currently 
     * viewable to the player
     */
    public Hand getOpenHand() {
        return hand;
    }

    /**
     * Puts the {@link Card} in this Player's {@link Hand}.
     * @param card the {@link Card} this {@code Player} is receiving.
     */
    public void receiveCard(final Card card) {
        hand.addCard(card);
    }

    /**
     * A class for when this Person is AI and has to respond
     * by itself instead of waiting for user input.
     * @return an empty {@link String} if this is a user and not 
     * AI, "hit" or "stay" if this is AI.
     */
    public String autoRespondWantToHit() {
        return "";
    }

    /**
     * Asks the game to give this {@code Person} a new {@link Card}
     */
    public void wantsToHit() {
        blackjack.dealCard(this);
    }
}

Player.java

package blackjack;

/**
 * A player in a Blackjack game.
 * @author Rick
 */
public class Player extends Person {

    /**
     * Constructs a Player with the name "Unnamed Player".
     * @param blackjack the {@link Blackjack} instance that called
     * this constructor
     */
    public Player(final Blackjack blackjack) {
        this("Unnamed Player", blackjack);
    }

    /**
     * Constructs a Player with the given name.
     * @param name the name this Player is going to get as a 
     * {@link String}, preferably in Title case.
     * @param blackjack the {@link Blackjack} instance that called
     * this constructor
     */
    public Player(final String name, final Blackjack blackjack) {
        super(name, blackjack);
    }

}

Dealer.java

package blackjack;

import java.util.Random;

/**
 * The Dealer in a Blackjack game.
 * @author Rick
 */
public class Dealer extends Person {
    private static final int CUSP = 16;

    /**
     * Creates a new {@code Dealer} with name "The Dealer".
     * @param blackjack the {@link Blackjack} instance that called
     * this constructor
     */
    public Dealer(final Blackjack blackjack) {
        super("The Dealer", blackjack);
    }

    /**
     * Only returns a {@link Hand} with the open {@link Card} 
     * that all {@link Player} instances can see.
     * @return a {@link Hand} with the {@link Dealer}'s first
     * {@link Card} in it
     */
    @Override
    public Hand getOpenHand() {
        Hand hand = new Hand();
        hand.addCard(super.getHand().getCards().peek());
        return hand;
    }

    public String autoRespondWantToHit() {
        if (this.getHand().getTotal() == CUSP) {
            return (new Random().nextBoolean() ? "hit" : "stay");
        }
        return (this.getHand().getTotal() < CUSP ? "hit" : "stay");
    }
}

Hand.java

package blackjack;

import java.util.PriorityQueue;
import java.util.Queue;
import java.util.List;
import java.util.ArrayList;

/**
 * A {@code Hand} holds a collection of {@link Card} instances.
 * @author Rick
 * @since 1.5
 */
public class Hand {
    private Queue<Card> cards = new PriorityQueue<Card>();

    /**
     * Adds a {@link Card} to the collection.
     * @param card the {@link Card} to be added
     */
    public void addCard(Card card) {
        cards.add(card);
    }

    /**
     * @return the total value of this {@code Hand} in a Blackjack game.
     * If there are aces, returns the highest value under 22.
     */
    public int getTotal() {
        List<Integer> totals = new ArrayList<Integer>();
        totals.add(new Integer(0));
        for(Card card : cards) {
            for (int i = 0; i < totals.size(); i++) {
                totals.add(i, new Integer(totals.get(i).intValue() + card.getValue()));
                totals.remove(i+1);
            }
            if (card.getRank() == 1) totals.add(new Integer(totals.get(totals.size() - 1) - 10));
        }
        for (Integer total : totals) {
            if (total.intValue() <= 21) return total;
        }
        return totals.get(totals.size()-1).intValue();
    }

    /**
     * @return a {@link Queue} containing the cards in this {@code Hand}
     */
    public Queue<Card> getCards() {
        return cards;
    }
}

Card.java

package blackjack;

/**
 * Enumeration of all possible {@code Card} values and ranks.
 * @author Rick
 * @since 1.5
 */
public enum Card implements Comparable<Card> {

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

    private final int rank;
    private final int value;

    /**
     * @param rank an {@code int} to signify the rank
     * @param value the value of this rank in a Blackjack game.
     * The value of 1 for an Ace is excluded here and should be 
     * treated as a special case elsewhere.
     */
    private Card(final int rank, final int value) {
        this.rank = rank;
        this.value = value;
    }

    /**
     * @return an {@code int} to signify the rank
     */
    public int getRank() {
        return rank;
    }

    /**
     * @return the value of this rank in a Blackjack game.
     * The value of 1 for an Ace is excluded here and should be 
     * treated as a special case elsewhere.
     */
    public int getValue() {
        return value;
    }
}

HandDisplay.java

package blackjack;

import java.util.Collections;
import java.util.Queue;
import java.util.PriorityQueue;

/**
 * A utility class helping to display the cards in a {@link Hand} in plain English.
 * @author Rick
 * @since 1.5
 */
public final class HandDisplay {

    /** An array of the English names of playing cards.  */
    public static final String[] CARD_NAMES = {"joker","ace","two","three","four","five","six","seven","eight","nine","ten","jack","queen","king"};
    /** An array of the English names of playing cards in plural form.  */
    public static final String[] CARD_NAMES_PLURAL = {"jokers","aces","twos","threes","fours","fives","sixes","sevens","eights","nines","tens","jacks","queens","kings"};

    /**
     * Creates a {@link String} displaying the contents of a {@link Queue} of {@link Card} instances.
     * @param cards a {@link Queue} containing zero or more {@link Card} instances.
     * @return a {@link String} describing the cards in plain English, or "nothing" if there are zero cards.
     */
    public static String cardsToString(final Queue<Card> cards) {
        if (cards.size() == 0) return "nothing";

        StringBuilder sb = new StringBuilder(34);
        PriorityQueue<Card> cardsCopy = new PriorityQueue<Card>(cards);

        while (cardsCopy.size() > 0) {
            Card card = cardsCopy.remove();
            int amountOfSameRank = 1 + Collections.frequency(cardsCopy, card);
            for (int j = 1; j < amountOfSameRank; j++) { // also remove all other Card instances with the same rank
                cardsCopy.remove(card);
            }

            sb.append(amountOfSameRank + " " + ( amountOfSameRank == 1 ? CARD_NAMES[card.getRank()] : CARD_NAMES_PLURAL[card.getRank()] ));

            switch (cardsCopy.size()) {
                case 0:      break;
                case 1:      sb.append(" and ");
                         break;
                default: sb.append(", ");
                         break;
            }
        }
        if (cards.size() == 1) sb.append(" and a hidden card");
        return sb.toString();
    }
}

Here's some things I was wondering while I was making this:

  • I only need the PriorityQueue<Card> that Hand holds to be an actual Queue in HandDisplay. Should I keep it as a Collection<Card> instead of as a Queue<Card> on all other places?
  • Everywhere I use it, I need the LinkedList<Card> inside Deck to really be LinkedList (I believe; I only use it with remove() and shuffle). Should I still save this attribute as an interface, and if so, as which interface? List? Queue? Collection?
  • The wantsToHit() method in Person really doesn't feel right to me. Should I just let ConsoleView tell Blackjack to deal this Person a card?
  • More than half of the methods in BlackjackView have to deal with a Person instance, receive this Person as an argument and display something of this Person. Should I create an extra view to deal with this, e.g. `PersonView'?
  • After implementing both values 1 and 11 for an Ace, the Dealer's decision making is probably flawed, I will look at this later.

I think I may have tackled this all the wrong way. The way I created the model classes is probably based too much on me knowing there is a controller and a view class running the game. Actually: I started with the next step in the controller and the view, before implementing a functionality in the model. Now I feel like I should write the complete code for the model first and making it able to run without the view and controller (i.e. fake user input for testing and maybe some System.outs so I can see it actually works) and only after I have done that create a view and a controller.

\$\endgroup\$
1
\$\begingroup\$

I'm glad you're continuing work on your Blackjack project. I think you've made a lot of progress but I think you're right in that there are some crossed wires between different responsibilities in your code. Let's walk through it and try and identify the single responsibility of each class and see how we can change it to reduce the complexity of the code.

Let's start off with Card. There's no much to change here, one thing that I think would be useful is a toString method. If you're not familiar with toString, it's a method that is inherited by all Objects from the Object class. It is a String representation of the Object. (by default it is the memory address, but since it's an enum, it would look like SEVEN, ACE etc)

Let's make it look something like this

first add the code from the HandDisplay class.

private static final String[] CARD_NAMES = {
        "joker", "ace", "two", "three",
        "four", "five", "six", "seven",
        "eight", "nine", "ten", "jack",
        "queen", "king"
};

now we can add our toString

@Override
public String toString() {
    return CARD_NAMES[rank] + " value: " + value;
}

with this method, we can simply do

System.out.println(card.toString());

or even more simply

System.out.println(card);

since it knows to call .toString() without us telling it to!

Let's move onto the Deck class.

You had this question

Everywhere I use it, I need the LinkedList inside Deck to really be LinkedList (I believe; I only use it with remove() and shuffle). Should I still save this attribute as an interface, and if so, as which interface? List? Queue? Collection?

The fact that you're using a LinkedList is an implementation detail and the calling code shouldn't be forced to use a LinkedList just because you're using one, so you're right about this. You should stick to an interface, in this case List makes the most sense I think. You could do something like this to achieve this.

public List<Card> getCards() {
        return new ArrayList<>(cards);
}

But before you go and change this, instead of updating this method to return a List instead of a LinkedList. We can actually just deleted instead! We don't need to provide access to this list at all!

While we're here, I want to say that you should never return a reference to a mutable object directly. You should return a defensive copy. If you return the reference to the underlying object, the calling code can mutate that object and unintentionally alter the state of the object. You can do this like

return new ArrayList<>(underlyingList);

However the contents of the underlyingList can still be changed, so watch out!

Back to the Deck class. You're using a LinkedList which gets the job done as a way of representing the Cards in the deck. I wanted to provide an alternative solution, how about a Stack. Since you're only removing Cards from the top, you could simply do

private Stack<Card> cards = new Stack<>();

and

public Card draw() {
    if (cards.isEmpty()) {
        fillDeckWithAllCards();
    }
    return cards.pop();
}

Another change I've made is LBYL (Look before you leap). In your current code, you use try/catch as a form of control flow. In general this should be avoided. There are plenty of good SO questions and answers about this topic.

Next up is the Hand class. I really don't have much to say on this one, but I would change the return type of the getCards method to a List instead of a Queue. Remember just because you use a Queue as the underlying data structure, doesn't mean the caller needs to be forced into using that. How about this.

public List<Card> getCards() {
    return new ArrayList<>(cards); // defensive copy and conversion to list
}

While I'm here I'll also add a toString method for a Hand

@Override
public String toString() {
    StringBuilder sb = new StringBuilder("\nHand:\n");
    for (Card card : cards) {
        sb.append(card).append("\n");
    }
    sb.deleteCharAt(sb.length() - 1); // remove last new line
    return sb.toString();
}

Next I want to talk about your Person class. I don't actually like the name of this class. I would prefer Player. Is every Player in your game a Person? What if I want to make an AIPlayer, they're not really a Person, but they are a Player. This is of course entirely up to you, but I'm going to make the parent class called Player in the code examples here!

So there are going to be some important changes here. In your question, you said

The wantsToHit() method in Person really doesn't feel right to me. Should I just let ConsoleView tell Blackjack to deal this Person a card?

It's a good sign that it feels off, you're on the right track but it's a bit tangled up. But no the view should not influence the game, it should do nothing but well, view!

Now I don't think a Player needs to know about the Blackjack at all!

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

okay no more blackjack.

we don't need to know about the blackjack Object, but we do need to be able to receive cards, good thing we have this method already.

public void receiveCard(Card card) {
    hand.addCard(card);
}

One thing that worries be is this method

public String autoRespondWantToHit() {
    return "";
}

Later on you are relying on it being an empty String to identify it as a player choice, instead of dealing with Strings to me it makes a lot more sense to deal with yes/no if the player wants to hit. Let's change it, and maybe give it a better name while we're at it

public abstract boolean wantsToHit();

public abstract boolean wantsToPlayAgain();

these two methods are in the parent class for Dealer and the new HumanPlayer implementation. I don't think there can be any confusion for what's what with names like those! (maybe we can add some AIPlayers too)

For now we have two implementations of these abstract methods, HumanPlayer and Dealer. Let's look at the Dealer ones first.

@Override
public boolean wantsToHit() {
    boolean couldGoEitherWay = hand.getTotal() == CUSP;
    if (couldGoEitherWay) {
        return rnd.nextBoolean(); // true or false instead of  "hit" or "stay";
    }
    return hand.getTotal() < CUSP;
}

@Override
public boolean wantsToPlayAgain() {
    return false; // or true, depending on how much the Dealer wants to play!
}

look familiar? It's exactly the same as your one, but it returns true/false instead of "hit"/"stay".

how about the HumanPlayer

@Override
public boolean wantsToHit() {
    return chooseBetweenOptions("hit", "stay"); // user input hit/stay
}

@Override
public boolean wantsToPlayAgain() {
    return chooseBetweenOptions("y", "n"); // user input y/n
}

private boolean chooseBetweenOptions(String option1, String option2) {
    String input = userInput.nextLine();
    while (!(input.equals(option1) || input.equals(option2))) {
        input = userInput.nextLine();
    }
    return input.equals(option1);
}

again, the same as yours, but this time the HumanPlayer has a Scanner instance, this user input isn't in the View anymore. Again, I want to reiterate, the View is only there to view! Not to read in user input.

Since we're talking about it, let's go have a look at the BlackjackView interface.

I like these method names, welcome, showInitialHand, showHand, displayTotal, showWinner and thanksForPlaying. These all look good since they are just a view on the state of the game. What worries me are these askForTurn and wantToPlayAgain. These last two methods prompt for user input! A view shouldn't know or care about how these choices are made, only how to show them. This functionality has just been added to the player class, so we can just remove these methods completely from the interface.

Let's have a look at the Blackjack game itself.

Firstly, let's make a defensive copy, not return the reference directly!

public List<Player> getPlayers() {
    return new ArrayList<>(players);
}

At the moment, you pass in player names and player objects are created inside the constructor. This limits you to only being able to use your player objects. I would prefer to see something like this

public Blackjack(Dealer dealer, List<Player> players) {...}

Now, you can make any kind of player. Human Players, AI Players etc. You're no longer restricted to a single type.

Your initializeGame method, doesn't actually initialize anything

public static Blackjack initializeGame() {
    return new Blackjack();
}

there's no difference between this and just making a new object. I think it should look like this.

public Blackjack(Dealer dealer, List<Player> players) {
    this.players = new ArrayList<>();
    this.players.add(dealer); // dealer is first
    this.players.addAll(players);
    dealInitialCards();
}

I've also made dealInitialCards private, there's no reason for this to be needed outside original object construction.

Okay, now let's tie this view and model together in our controller.

At the moment, the controller has the same problem of being limited to a single view, in this case the ConsoleView, we should be able to give it any view. (At the moment there is only one, but we want to keep our options open!)

Let's allow for this, let's update the constructor.

public BlackjackController(Blackjack blackjack, BlackjackView view) {
    this.blackjack = blackjack;
    this.view = view;
}

The current letPersonPlayitsTurn method looks over complicated to me, compare the existing version to something like this

private void letPersonPlayItsTurns(Player player) {
    view.showHand(player);
    while (true) {
        view.askForTurn(player); // prompts for the hit/stay
        if (player.wantsToHit()) { // reads in from Scanner if 'hit'
            blackjack.dealCard(player);
            view.playerHit(player);
            view.showHand(player);
        } else { // 'stay'
            view.playerStay(player);
            break;
        }
    }
}

Notice the view isn't in charge of any decision making We don't check for hard coded "hit" or "stay" strings.

We're using polymorphism in that the Dealer and HumanPlayer have different implementations is how they decide if they wantToHit or wantsToPlayAgain

the playOneGame method hasn't changed a lot, I've actually just removed some code from it

private void playOneGame() {

    view.welcome();
    for (Player player : blackjack.getPlayers()) {
        view.showInitialHand(player);
    }
    for (Player player : blackjack.getPlayers()) {
        letPersonPlayItsTurns(player);
    }
    for (Player player : blackjack.getPlayers()) {
        view.displayTotal(player);
    }

    Player winner = blackjack.getWinner();
    view.showWinner(winner);

    if (winner.wantsToPlayAgain()) {
        playOneGame();
    }
}

And the run method is also trimmed a little

public void run() {
    playOneGame();
    view.thanksForPlaying();
}

I think the main method doesn't belong in the controller class, let's make a new Main class and put it there.

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

public class Main {
    public static void main(String[] args) {
        Dealer dealer = new Dealer();
        Player playerOne = new HumanPlayer("Player 1");
        List<Player> allPlayers = new ArrayList<>();
        allPlayers.add(playerOne);
        Blackjack blackjack = new Blackjack(dealer, allPlayers);
        BlackjackView view = new ConsoleView();
        BlackjackController controller = new BlackjackController(blackjack, view);
        controller.run();
    }
}

Now just to demonstrate how easy it is to add a new type of player, let's ad a very basic AI player that can play the game with you

public class YoloBot extends Player {

    public YoloBot(String name) {
        super(name);
    }

    @Override
    public boolean wantsToHit() {
        return true;
    }

    @Override
    public boolean wantsToPlayAgain() {
        return true;
    }
}

There we go we now have a player that can also play the black jack game. And how easy is it to add him to the game? Let's go to the main method.

Player playerOne = new HumanPlayer("Player 1");
Player yoloBot = new YoloBot("Yolo Bot 1.0");
List<Player> allPlayers = new ArrayList<>();
allPlayers.add(playerOne);
allPlayers.add(yoloBot);

and now he'll play with us (and probably lose every game)

but notice how I didn't have to go in and alter any of the blackjack game code? Or the view code? Or the controller code?

Because I'm programming to the super class, I can just make any player that extends it, and update the main method!

Now my goal here was to try and remove as many dependencies as possible, refactor to adhere to the SRP (Single Responsibility Principle) and make the code more modular and reusable.

I wanted to touch on just a few more things before I wrap up.

I think you were going a bit overkill on having different Display classes just for displaying different types of things. Just implement the toString method and you can pretty much just print them!

You're right in that you should program to the interface where possible.

You should make defensive copies for mutable objects.

Avoid returning Strings if you're just comparing what they entered. You should do the comparison in the method to return a true/false, then use that information to perform the desired task.

I hope that this review was useful for you.

Here's a link to the full source code if you want to have a look!

https://www.dropbox.com/s/ic8dnvdrqy5p6r0/blackjack.zip?dl=0

EDIT:

here's an example of the program's output. I didn't stick to the way you did it, but you can update it however you like of course.

Welcome to a new Blackjack game!

Player: Dealer has Hand: seven value: 7.

Player: Player 1 has Hand: jack value: 10.

Player: Dealer has Hand: seven value: 7 three value: 3.

That makes a total of 10.

Does Player: Dealer want to 'hit' or 'stay'? Player: Dealer chose to hit Player: Dealer has Hand: seven value: 7 three value: 3 four value: 4.

That makes a total of 14.

Does Player: Dealer want to 'hit' or 'stay'? Player: Dealer chose to hit Player: Dealer has Hand: nine value: 9 seven value: 7 four value: 4 three value: 3.

That makes a total of 23.

Does Player: Dealer want to 'hit' or 'stay'? Player: Dealer chose to stay Player: Player 1 has Hand: jack value: 10 eight value: 8.

That makes a total of 18.

Does Player: Player 1 want to 'hit' or 'stay'? stay Player: Player 1 chose to stay Player: Dealer's total is 23. Player: Player 1's total is 18. Player: Player 1 is the winner!

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks. It all makes so much sense when you explain how it should be done. Just one question: now there is a Scanner in HumanPlayer, which makes this only work in a console view. Shouldn't the model classes be independent of the view? How to change this so I could also implement for example a GUI view? \$\endgroup\$ – Rick Sep 10 '17 at 13:20
  • \$\begingroup\$ you're right! This is more of a HumanConsolePlayer I guess, if you had some gui, maybe what you could do is have the method take the text from some sort of input box, and then check the String for hit/stay in a similar way, you could maybe have a constructor like new HumanGuiPlayer(name, textBox) or something similar \$\endgroup\$ – chatton Sep 10 '17 at 13:26
  • \$\begingroup\$ That is a working solution, but it doesn't really sit right with me. I think the model should work the same no matter what the application looks like from the outside. Now, probably this Blackjack game is not the perfect example to use perfect MVC on, but as I am learning I am trying to grasp the concept. There seems to be a lot of grey areas here, but multiple sources (i.e. web.archive.org/web/20090805073932/http://java.sun.com/…) say the view is what allows for user input and the controller is what handles this input. \$\endgroup\$ – Rick Sep 17 '17 at 14:35

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.