OOP implementation of Blackjack in Java - part 2

Question

Last time I got a lot of great advice, so this is take two with all that in mind:

My main concern with this implementation is the use of Inheritance (A base Player) vs having that as a interface - as that would lead to duplicate code in both Player and Dealer classes.

Everything works - the last thing for me to implement is having the dealer try to get to 21 after all the players.

But I would still really appreciate a review of this new version.

Deck.java

package com.tn.deck;


public interface Deck<T> {
    T dealCard();
    void shuffle();
}

AbstractPlayer.java

package com.tn.blackjack;

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


public class AbstractPlayer {
    private static final int WINNING_NUMBER = 21;
    List<Card> hand = new ArrayList<>();

    private boolean containsAce() {
        return hand.stream().anyMatch(card -> card.getRank() == Rank.ACE);
    }

    final void drawCards(Card... cards) {
        hand.addAll(Arrays.asList(cards));
    }

    final boolean isBust() {
        return calculateScore() > WINNING_NUMBER;
    }

    final boolean hasBlackjack() {
        return calculateScore() == WINNING_NUMBER;
    }

    final int calculateScore() {
        int score = hand.stream().mapToInt(card -> card.getRank().getValue()).sum();
        return score > WINNING_NUMBER && containsAce() ?
                score - 10 : //Takes care of ace being either 1 or 11
                score;
    }
}

Card.java

package com.tn.blackjack;


public class Card {
    private final Suit suit;
    private final Rank rank;

    Card(Suit suit, Rank rank) {
        this.suit = suit;
        this.rank = rank;
    }

    public Suit getSuit() {
        return suit;
    }

    public Rank getRank() {
        return rank;
    }

    public void print() {
        System.out.printf("%s%s ", suit.getIcon(), rank.getName());
    }
}

CardDeck.java

package com.tn.blackjack;

import com.tn.deck.Deck;

import java.util.*;
import java.util.stream.Collectors;
import java.util.stream.IntStream;


public class CardDeck implements Deck<Card> {
    private List<Card> deck;

    CardDeck() {
        this.deck = initializeDeck(Suit.values(), Rank.values());
        shuffle();
    }

    CardDeck(int numberOfDecks) {
        this.deck = initializeMultipleDecks(numberOfDecks, Suit.values(), Rank.values());
        shuffle();
    }

    private List<Card> initializeDeck(Suit[] suits, Rank[] ranks) {
        return Arrays.stream(suits)
                .flatMap(suit -> Arrays.stream(ranks).map(rank -> new Card(suit, rank)))
                .collect(Collectors.toList());
    }

    private List<Card> initializeMultipleDecks(int numberOfDecks, Suit[] suits, Rank[] ranks) {
        return IntStream.range(0, numberOfDecks)
                .mapToObj(i -> initializeDeck(suits, ranks))
                .flatMap(Collection::stream)
                .collect(Collectors.toList());
    }

    @Override
    public Card dealCard() {
        if(deck.size() < 1) {
            throw new IllegalStateException("Deck is empty");
        }
        Card card = deck.get(0);
        deck.remove(0);
        return card;
    }

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

Dealer.java

package com.tn.blackjack;

import java.util.Arrays;


public class Dealer extends AbstractPlayer {
    private Prompter prompter = new Prompter();
    private CardDeck deck;

    Dealer() {
        initializeDeck();
    }

    public void dealInitialTwoCards(Player[] players) {
        Arrays.stream(players).forEach(player -> player.drawCards(deck.dealCard(), deck.dealCard()));
        drawCards(deck.dealCard(), deck.dealCard());
    }

    public void startPlayerLoop(Player[] players) {
        Arrays.stream(players).forEach(player -> player.performAction(deck.dealCard()));
    }

    private void initializeDeck() {
        int numberOfDecks = prompter.ask("How many decks should be used? ");
        if(numberOfDecks < 1) {
            throw new IllegalArgumentException("Deck size must be at least 1");
        }

        this.deck = numberOfDecks > 1 ?
                new CardDeck(numberOfDecks) :
                new CardDeck();
    }

}

Player.java

package com.tn.blackjack;


public class Player extends AbstractPlayer {
    private Prompter prompter = new Prompter();
    private int id;

    Player(int id) {
        this.id = id;
    }

    public int getId() {
        return id;
    }

    public void performAction(Card card) {
        System.out.printf("%n%n==== Player %d ====%n%n", getId());
        State state;
        do {
            prompter.printStatus(this);
            state = prompter.getState();
            switch (state) {
                case HIT: drawCards(card);
                    break;
                default:
                    break;
            }
            if(isBust() || hasBlackjack()) {
                prompter.printStatus(this);
                break;
            }

        } while(state == State.HIT);
    }
}

Prompter.java

package com.tn.blackjack;

import java.util.Scanner;


public class Prompter {
    private Scanner scanner = new Scanner(System.in);

    public void printStatus(AbstractPlayer player) {
        player.hand.forEach(Card::print);
        System.out.printf("( score of %d )", player.calculateScore());
        System.out.printf(player.hasBlackjack() ?
                "\tBLACKJACK" : player.isBust() ?
                "\tBUST" : "");
    }

    public State getState() {
        String answer;
        do {
            System.out.printf("%n%nDo you want to (H)it or (S)tand? ");
            answer = scanner.nextLine().trim().toUpperCase();
        } while (!answer.equals("H") && !answer.equals("S"));

        State state;
        switch (answer) {
            case "H": state = State.HIT;
                    break;
            case "S": state = State.STAND;
                    break;
            default: state = null;
        }
        return state;
    }

    public int ask(String question) {
        String answer;
        do {
            System.out.printf("%s", question);
            answer = scanner.nextLine().trim();
        } while (!isInt(answer));
        return Integer.parseInt(answer);
    }

    private static boolean isInt(String s) {
        try {
            Integer.parseInt(s);
        } catch (NumberFormatException ex) {
            return false;
        }
        return true;
    }
}

Rank.java

package com.tn.blackjack;


public enum Rank {
    TWO("2", 2), THREE("3", 3), FOUR("4", 4), FIVE("5", 5),
    SIX("6", 6), SEVEN("7", 7), EIGHT("8", 8), NINE("9", 9), TEN("10", 10),
    JACK("J", 10), QUEEN("Q", 10), KING("K", 10), ACE("A", 11);

    private final String name;
    private final int value;

    Rank(String name, int value) {
        this.name = name;
        this.value = value;
    }

    public String getName() {
        return name;
    }

    public int getValue() {
        return value;
    }
}

State.java

package com.tn.blackjack;


public enum State {
    HIT, STAND
}

Suit.java

package com.tn.blackjack;


public enum Suit {
    SPADE("\u2660"),
    HEART("\u2665"),
    DIAMOND("\u2666"),
    CLUB("\u2663");

    private final String icon;

    Suit(String icon) {
        this.icon = icon;
    }

    public String getIcon() {
        return icon;
    }
}

Game.java

package com.tn.blackjack;

import java.util.stream.IntStream;


public class Game {
    private Prompter prompter = new Prompter();
    private Dealer dealer;
    private Player[] players;

    public Game() {
        initializePlayers();
    }

    public void start() {
        dealer.dealInitialTwoCards(players);
        dealer.startPlayerLoop(players);
    }

    private void initializePlayers() {
        int numberOfPlayers = prompter.ask("Not including the dealer - How many players? ");
        this.dealer = new Dealer();
        this.players = IntStream.rangeClosed(1, numberOfPlayers)
                .mapToObj(Player::new)
                .toArray(Player[]::new);
    }
}

I haven't tested the code, but it seems like it contains a bug when a hand contains multiple aces. Eg if a player has AAA7, that would be 20, but you calculate it as 30. — tim
– tim, Commented May 11, 2017 at 19:35
@tim Hm - yes. You are absolutely right. I will fix that. Thanks! — Nilzone-
– Nilzone-, Commented May 12, 2017 at 5:31
Please see what you can and can't do in your question. I've rolled back your fix to the bug. If you want to, you can post that as a partial review of your own code, but don't change the original question after answers has arrived. — holroy
– holroy, Commented May 13, 2017 at 19:43
@holroy Apologies. I thought as long as my fix didn't mess with other peoples reviews (change things they talk about in their review), it was OK. — Nilzone-
– Nilzone-, Commented May 13, 2017 at 19:47

Community · Accepted Answer · 2020-06-10 13:24:26Z

Guess who has a flash back?

Smaller things:

Deck: I can't see any declaration of that type.
Deck.shuffle(): Is only called within the implementation, so no need to make that public.
AbstractPlayer: is not abstract
AbstractPlayer: The hand is not private (it is implementation of abstraction, and should not be accessible to implementation of the abstraction... if that makes any sense)
The Card itself should be displayed by the Prompter, too (think of adding a GUI)
CardDeck: The Constructor has code duplication. The upper one should call the lower one with 1 as parameter. So initializeMultipleDecks will be initializeDecks, and initializeDeck can be removed. Then, you also get rid of the duplicate shuffle() call.
CardDeck: Not sure if you have to pass the Suit[] and Rank[] array for the initializer methods. I think it's okay, to get the values from the enum itself. (If you want to enhance it later: Add the method with the two parameters again, and delegate the call from the no-param method to the newly added one).
CardDeck.dealCard: There's a 'isEmpty' method in List. And: I can't find the place, where you actually check, if there's no card left? So, you will always end with an exception.
CardDeck.dealCard: return deck.remove(0) does the job.
Player.performAction: The method name doesn't tell me, what the method should do - but I see, this one is a bit tricky.
Player.performAction: I think the loop can be simplified: Can't the if condition moved into the while condition? Also: You have an empty default case.
Prompter.getState: State is not clear. The "H" and "S" is duplicated, that should be a constant with a proper name - maybe even provided in the enum itself (String symbol as constructor of the enum?). Also: the default case will return null. Since the code won't ever reach that point, I'd throw an Exception - if that happens, a bug has been introduced.
Prompter.ask: Who asks what whom why?
Rank: Can't remember, if I have ever seen a non-private constructor in an enum and I didn't know you can actually do that. However, it doesn't make any sense, since noone else can call it anyway?

Inheritance

A main object oriented principle is "lose coupling", inheritance can be, as I think it is in your case, the opposite. The concrete implementations are tightly coupled to its super class. My main problem with that, you can neither test the abstraction without a implementation, nor can you test a implementation without its abstraction.

Further, you're violating Liskov's substitution law (The L in SOLID): If your program works with type T, it should work with U extends T interchangeably, without changing your application. I think it's enough to explicitly declare a Dealer and a Player in that case. I hope someone corrects me, if I'm wrong, or describes it better: I always had a hard time understand the L and I usually have other reasons to avoid inheritance.

Another general rule is composition over inheritance. Meaning: Do not use inheritance for code reuse. It's much more flexible, easier to maintain and what not.

Hope that helps...

Once again - thank you for great advice! I'm still unsure of the inheritence-part though. I my mind it makes sense to have a base class we can inherit from since player and dealer share so much common functionality? — Nilzone-
– Nilzone-, Commented May 12, 2017 at 10:16
Yes, it does make sense - at first ;-). Just think about testing your calculateScore method. Why do you need a concrete implementation to test that method? Or: How to you want to reuse that method, without being a player? ... There's a lot of articles and discussions about pros and cons and questions to ask which help on your decision making in the interwebs - there's also a section in the Design Pattners Book, I think. If you would write unit tests, you would prefer composition, I'm 99% sure. — slowy
– slowy, Commented May 12, 2017 at 11:41
Valid points :) One last question; I have a Dealer and a Player - what would an approriate name for the base-class? Calling it AbstractPlayer is a little misleading considering it's not.. — Nilzone-
– Nilzone-, Commented May 12, 2017 at 11:45
I don't think AbstractPlayer is that bad. Wait a minute... If I remember the rules correctly, a Dealer must stand after he has >= 17 points. And he has to have at least 17 and can't hold before that. And if he has an ace and and already 17, the ace must be counted as 11. So, actually, your dealer is a player, only the drawing of the cards is different, isn't it? Actually, I think that's what you have to change to satisfy Liskov's law... if you would extract the dealing and calculation functionalities to a separate type, of course. — slowy
– slowy, Commented May 12, 2017 at 12:12
Using backticks you can format stuff as code, and I would do that on the start of every point in your list referring to code sections. Bold, as I am, I've taken the liberty of doing so with your post. If you don't like it, you can rollback my edit by clicking on the edited by link below your post. — holroy
– holroy, Commented May 13, 2017 at 19:50

Imus · Accepted Answer · 2017-05-13 09:17:42Z

@Slowy has already pointed out most of the small things. He also pointed out that something feels wrong with your Dealer class but his explanation of what it is exactly isn't clear.

First a quick point about the L in SOLID (Liskov substitution principle): The principle says that if you are handling an AbstractPlayer anywhere in your code, then it shouldn't matter if it's an instance of AbstractPlayer or an instance of Dealer. This is a bit weird here since you can't directly instantiate an abstract class but the principle stays the same. It doesn't matter that it's an instance of Dealer at all, the expected behaviour stays exactly the same, so you do not violate that principle here.

What you do violate is the S in SOLID (single responsibility). What exactly is the main responsibility of a Dealer? Is that class responsible for handling a deck of cards (shuffling/passing cards to players/...). Or is it responsible for "playing" the house's hand (acting like an AbstractPlayer). The answer is both.

To see how these should be split up, let's look at what actually happens in a game of poker. Immagine you have a Dealer that is only responsible for handling the deck of cards. It doesn't play the game himself. So when it's a player's turn he can ask this Dealer for a card. If the Deck is empty the Dealer will first shuffle the previously collected cards and renews the deck.

When it's the "dealers" turn to play we can treat this as another player. Let's call this one HousePlayer for now. The HousePlayer, just like any other player, asks the Dealer for cards until it's pleased with his hand. (Actually, like @Slowy pointed out, the house player has strict rules on when to hold, so it's fully deterministic).

So which are the logical classes to model an actual real life dealer and what are their responsibilities?

We have a HousePlayer that is an AbstractPlayer with it's own rules on asking for cards and holding. (It also isn't allowed to split or double down).

Next we have the CardDeck that's already handling the shuffling and keeping track of the cards. You might want to add some functionalities to handle resetting when the deck is empty for example.

Last we have the Dealer (which does not extend AbstractPlayer). He is responsible for telling each player to "play" when it's their turn. And calling anything around that. So it could for example tell the CardDeck to initialise. Pass a player's request for a card to a CardDeck, or pass the CardDeck to a player when it's his turn (implementation detail). When everyone (including the house) has played his turn, it could initiate the payout handling. I would say that it shouldn't calculate payouts himself. Since that is another layer of abstraction and thus violates the Single responsibility principle again.

So this Dealer class should look a little bit like this:

public class Dealer {
    private AbstractPlayer dealer = new HousePlayer();
    private List<AbstractPlayer> players = new ArrayList<AbstractPlayer>();
    private CardDeck deck; //initialised in constructor?
    private Payout bank; //really choose better names for this

    //constructor(s)

    //methods for playing a round

    //methods for recolecting cards from players and passing them back to deck
}

Note that this may vary greatly depending on how/where you implement things. I just wanted to give a general idea on what it could be.

A thing to notice here is that the Dealer is now using composition mostly. It contains a HousePlayer, a deck of cards, a list of players, ... But it isn't any of those himself.

Stack Exchange Network

OOP implementation of Blackjack in Java - part 2

2 Answers 2

Smaller things:

Inheritance

You must log in to answer this question.

Linked

Hot Network Questions

OOP implementation of Blackjack in Java - part 2

2 Answers 2

Smaller things:

Inheritance

You must log in to answer this question.

Linked

Related

Hot Network Questions