OOP implementation of BlackJack in Java

Question

This code is not done, but still fully functional.

But, before I continue I'd really appreciate some input on the code-structure as is.

AbstractHand.java

package com.tn.deck;

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


public abstract class AbstractHand<T extends Suitable & Rankable> {
    protected List<T> hand = new ArrayList<>();

    public void drawCard(T o) {
        hand.add(o);
    }

    public void drawCards(T[] o) {
        hand.addAll(Arrays.asList(o));
    }

    public abstract void status();
    public abstract int calculateScore();
}

Deck.java

package com.tn.deck;


public interface Deck<T extends Suitable & Rankable> {
    T dealCard();
    T[] dealCards(int n);
    void shuffle();
}

Rankable.java

package com.tn.deck;


public interface Rankable<T extends Comparable<T>> {
    boolean isConsecutive(T other);
}

Suitable.java

package com.tn.deck;


public interface Suitable<T extends Comparable<T>> {
    boolean isSameSuit(T other);
}

Card.java

package com.tn.blackjack;

import com.tn.deck.Rankable;
import com.tn.deck.Suitable;



public class Card implements Suitable<Card>, Rankable<Card>, Comparable<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());
    }

    @Override
    public boolean isConsecutive(Card other) {
        return 1 + rank.ordinal() == other.rank.ordinal() ||
                rank.ordinal() - 1 == other.rank.ordinal();
    }

    @Override
    public boolean isSameSuit(Card other) {
        return suit.equals(other.suit);
    }

    @Override
    public int compareTo(Card other) {
        if(rank.getValue() == other.getRank().getValue() &&
                suit.getIcon().equals(other.getSuit().getIcon())) {
            return 0;
        } else if(rank.getValue() < other.getRank().getValue()) {
            return -1;
        } else {
            return 1;
        }
    }
}

CardDeck.java

package com.tn.blackjack;

import com.tn.deck.Deck;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Random;


public class CardDeck implements Deck<Card> {
    private static final Random rand = new Random(System.nanoTime());
    private List<Card> deck;
    private List<Card> dealtCards;

    CardDeck(Suit[] suits, Rank[] ranks) {
        this.deck = initializeDeckWith(suits, ranks);
        this.dealtCards = new ArrayList<>();
        shuffle();
    }

    private List<Card> initializeDeckWith(Suit[] suits, Rank[] ranks) {
        List<Card> newDeck = new ArrayList<>();
        for(Suit suit : suits) {
            for(Rank rank : ranks) {
                newDeck.add(new Card(suit, rank));
            }
        }
        return newDeck;
    }

    @Override
    public Card dealCard() {
        if(deck.size() < 1) {
            throw new IllegalStateException("Deck is empty");
        }
        int index = rand.nextInt(deck.size());
        Card card = deck.get(rand.nextInt(index));
        deck.remove(index);
        dealtCards.add(card);

        return card;
    }

    @Override
    public Card[] dealCards(int n) {
        if(deck.size() < n) {
            throw new IllegalStateException("Not enough cards left in deck");
        }

        Card[] cardsToDeal = new Card[n];
        for(int i = 0; i < n; i++) {
            int index = rand.nextInt(deck.size());
            Card card = deck.get(index);
            deck.remove(index);
            cardsToDeal[i] = card;
            dealtCards.add(card);
        }
        return cardsToDeal;
    }

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

Dealer.java

package com.tn.blackjack;

import com.tn.deck.AbstractHand;


public class Dealer extends AbstractHand<Card> {
    private CardDeck deck;

    Dealer() {
        this.deck = new CardDeck(Suit.getSuits(), Rank.getRanks());
    }

    public void startInitialDealingOfCards(Player[] players) {
        for(Player player : players) {
            Card[] initialCards = deck.dealCards(2);
            player.drawCards(initialCards);
        }
        Card[] dealersInitialCards = deck.dealCards(2);
        drawCards(dealersInitialCards);
    }

    @Override
    public void status() {
        System.out.printf("%nThe Dealer, has the following hand:%n");
        hand.forEach(Card::print);
        System.out.printf("( score of %d )", calculateScore());
    }

    @Override
    public int calculateScore() {
        return hand.stream().mapToInt(card -> card.getRank().getValue()).sum();
    }
}

Player.java

package com.tn.blackjack;

import com.tn.deck.AbstractHand;


public class Player extends AbstractHand<Card> {
    private int id;

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

    @Override
    public void status() {
        System.out.printf("%nPlayer %d, has the following hand:%n", id);
        hand.forEach(Card::print);
        System.out.printf("( score of %d )", calculateScore());
    }

    @Override
    public int calculateScore() {
        return hand.stream().mapToInt(card -> card.getRank().getValue()).sum();
    }
}

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 static Rank[] getRanks() {
        return new Rank[] {
                TWO, THREE, FOUR, FIVE,
                SIX, SEVEN, EIGHT, NINE, TEN,
                JACK, QUEEN, KING, ACE
        };
    }

    public String getName() {
        return name;
    }

    public int getValue() {
        return value;
    }
}

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 static Suit[] getSuits() {
        return new Suit[] { SPADE, HEART, DIAMOND, CLUB };
    }

    public String getIcon() {
        return icon;
    }
}

Game.java

package com.tn.blackjack;

import java.util.Arrays;


public class Game {
    private Dealer dealer;
    private Player[] players;

    public Game() {
        this.dealer = new Dealer();
        this.players = new Player[] {
                new Player(1),
                new Player(2)
        };
    }

    public void start() {
        dealer.startInitialDealingOfCards(players);
        dealer.status();
        Arrays.stream(players).forEach(Player::status);
    }
}

Answer 1

My two cents

Abstraction / inherticance

I always try to avoid abstraction and inheritance. In general, it's really hard, to achieve a high cohesion when working with abstraction, because it's very tempting to mix abstraction and implementation. Beside that, it's not possible to (unit) test abstraction without implementation, the other way round, often you will test abstraction/logic from the super class implicitly when testing implementation. Also: The more inheritance is present, the more complex it gets - if you pass another parameter in the constructor in the base class, have fun changing all your subclasses. Some subclasses override methods, some don't, some call abstract methods, some don't.

A very confusing part of your code is: A Player is a hand. The Dealer is a hand. They both should have a Hand, don't they?

overcomplicating things

I don't see any need for your interfaces, for instances the Rankable and Suitable interfaces - nowhere in the code you declare anything as one of those types. If anyone teached you, to make your application "future proof", by this sort of "over-engineering", don't listen to him: in most of the cases, it's not needed. I used to do that, too, but I just ended up with way too much unnecessary and harder to maintain code. Or sometimes even worse, what I thought, what will help in the future, had to be dismantled, because, well, I ain't no fortune teller and the requirements went the complete opposite way.

separation of concerns

A class should do one thing, also called single responsibility principle. Your Dealer class is a Hand, does calculate the score, prints the status and does some initialization. Since a Dealer should not be a Hand anyway and should be a state of the Dealer, some responsibilites will be moved to the correct place.

Printing/displaying objects is something I see often at the wrong place. Objects shouldn't display itself, there should be a separate type which does that job.

Other things

Noone seems to call isSameSuit? I only find it in the interface and in the implementation. And I think there's other unused methods, too. The IDE should usually show unused code, if not, try to figure out how to activate that. Dead code is something like a deadly sin.

Also: Sometimes you work with arrays, sometimes with Lists, I think you don't need to work with arrays.

Abstract Hand

• The name drawCard is incorrect. You add a card, the drawing happens somewhere else, same for drawCards.

• status() - Status what? What should one do when he overrides that method? I mentioned, the displaying of a type should be in a separate type anyway - but printStatus would be more precise. Or even better: printHand(), since status could mean anything. Try to be a specific as possible.

Deck

The dealCards has a parameter n. It's not clear, what n means, better would have been amountOfCards.

Card.compareTo

• I think there's a bug in the compareTo method, you only compare the value when the suit is the same. If the value is the same, but not the suit, you will return 1, won't you?
• You can write that much easier, I wouldn't have even implemented the first if condition, only < and >, then it's 0 in the end anyway. I would also get rid of the first else if, if the first condition is met, the method will return, if not, it will get into the else-if anyway, so one if on a separate line is enough.

CardDeck

• You can initialize your variables in the class, so the constructor is easier to read.
• initializeDeckWith: I think, the "with" suffix is obsolete. It doesn't really help and the parameters should make it clear enough.
• dealCard / dealCards: You have duplication here (DRY: "don't repeat yourself"), just call the dealCard from dealCards. Except the guard, that should stay. Also: The remove method does return the removed object, so no need to get the object at the specific index and remove it aftwards. Also: If you provide a shuffle method for a deck, I don't think a Random is needed.

Dealer

startInitialDealingOfCards: The method name is not clear, I had to read the method to be sure, what's going on: It deals the first two cards. Maybe dealInitialTwoCards. Even though, many are familiar with the Black Jack rules, not all are.

Rank / Suit

I don't understand, why you are providing separate methods to get the values, enum's already provide those methods. I wouldn't suggest to do that, because that's your own convention. If it doesn't do what the default method for an enum does, give it a proper name.

Hope that helps...

Answer 2

Overall your code looks pretty good. Nice formatting, nice split of responsibilities (so far), ...

There's only 1 thing that bothers me with your interfaces. And that's the YAGNI principle. That's short for "You Aren't Going to Need It".

Since you're modelling a game of blackjack, you're never really going to need to compare suits, so why provide an interface to do so?

Same for the Rankable interface. When would you need this in a blackjack game?

It's usually best to not write code that you don't expect to need any time soon. It will just distract you from doing the actual requirements.

---

Another thing that might be tricky later is the value of an ACE. It could actually be 11 or 1 depending on the total value of your hand. Not that your enum is wrong, but remember that you might have to substract 10 at some point. Alternatively you could give it value 1 in the enum and add 10 when the total hand value is less than 12.

---

The last thing I want to point out is that the calculateScore should be the same for a dealer or a normal player. So you should implement it in the abstract class and drop the abstract keyword. That way you Don't Repeat Yourself (DRY principle).

---

edit: Just remembered the hidden enum functions. Instead of providing public static Suit[] getSuits() { you could just call Suit.values() which gives you the exact same result. Same for the public static Rank[] getRanks() { method.

If you want to keep them for better readability you could also just change their implemenation to this:

public static Rank[] getRanks() {
    return Rank.values();
}

Answer 3

No big problems... just some small pointers. :)

Varargs

public void drawCard(T o) {
    hand.add(o);
}

public void drawCards(T[] o) {
    hand.addAll(Arrays.asList(o));
}

You can combine both methods with varags so that you can pass in one or multiple cards without having to make an array explicitly:

public void drawCards(T... cards) {
    hand.addAll(Arrays.asList(cards));
}

// Usage
// player.drawCards(oneCard);
// player.drawCards(oneCard, twoCard);

Checking for consecutiveness

A simpler check is to make sure that the absolute difference of the cards' values is 1:

@Override
public boolean isConsecutive(Card other) {
    return Math.abs(ordinal() - other.ordinal()) == 1;
}

Enum arrays

Your Rank.getRanks() and Suit.getSuits() can simply call the underlying Enum.values() method as the returned results will be the same.

Java 8 stream processing

In the spirit of Java 8, you can stream through your Card creation:

private List<Card> initializeDeckWith(Suit[] suits, Rank[] ranks) {
    List<Card> newDeck = new ArrayList<>();
    for(Suit suit : suits) {
        for(Rank rank : ranks) {
            newDeck.add(new Card(suit, rank));
        }
    }
    return newDeck;
}

Java 8 way:

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

Answer 4

Bug: the method dealCard can never deal the last card of the deck because you call rand.nextInt twice. Plus, it favors cards from the beginning of the deck. This should also sometimes throw exceptions, since you then call rand.nextInt(0), which would have to return a number between 0 (lower bound, inclusive) and 0 (upper bound, exclusive), and this is impossible.

A better way is to first create the deck, then shuffle the complete deck (Collections.shuffle does that for you) and then always deal the top card from the deck. Separating the shuffling from the dealing more closely models what happens in reality, plus you can better test the part that deals the cards.