Weekend Challenge - Poker Hand Evaluation

Question

Weekend Challenge #2 - Poker Hand Evaluation

Very early I decided to support the usage of wild jokers, even though I knew that this was going to lead to trouble more work. I also wanted to support big collection of cards, for example if you have 7 cards and choose the best 5-card poker hand of these. Of course, combinatorics can be used for this (7 nCr 5 = 35 combinations), but I wanted a better approach.

So to check for possible poker hands, I really didn't want to make the same mistake someone else did. And as usual, I also tend to prefer flexible solution. So pretty soon, the strategy pattern alarm went off in my head.

I have spent a lot of time writing tests to make sure that it works as it should. Now, I believe it works as it should (or at least the way I want it to).

Class overview

• AceValue - enum to define what the value of an ace is (not so useful here, but very used in other card projects)
• Suite - enum for hearts, spades, diamonds, clubs + one for wildcards and other fun stuff
• ClassicCard - class that contains a Suite and a rank (int). Also contains constants for the different ranks
• PokerHandEval - class that is responsible for determining the PokerHandResult given an array of ClassicCard
• PokerHandAnalyze - class used by PokerHandEval to analyze an array of cards and structure data into integer arrays of used ranks and suits.
• PokerHandType - enum for the standard type of poker hands available
• PokerHandResult - comparable class that holds the result of a poker hand. Includes PokerHandType + parameters ("full house of what?") + "kickers" (pair of 6s, sure, but what are your other highest cards?)
• PokerHandResultProducer - interface for declaring the method used by the strategy pattern that the below three classes implements
  • PokerPair - strategy for finding pairs, two-pair, three-of-a-kind, four-of-a-kind, and full house
  • PokerStraight - strategy for finding straight
  • PokerFlush - strategy for finding flush, straight flush, and royal flush - uses PokerStraight for determining the straights.

Some already existing code

Since I have been using cards earlier in Java, and I thought the result of this weekend challenge would come in handy at some point later in time, I decided to use Java again, and to use some code I already had.

ClassicCard.java

public class ClassicCard {

    private final Suite suite;
    private final int rank;

    public static final int RANK_ACE_LOW = 1;
    public static final int RANK_2 = 2;
    public static final int RANK_3 = 3;
    public static final int RANK_4 = 4;
    public static final int RANK_5 = 5;
    public static final int RANK_6 = 6;
    public static final int RANK_7 = 7;
    public static final int RANK_8 = 8;
    public static final int RANK_9 = 9;
    public static final int RANK_10 = 10;
    public static final int RANK_JACK = 11;
    public static final int RANK_QUEEN = 12;
    public static final int RANK_KING = 13;
    public static final int RANK_ACE_HIGH = 14;
    public static final int RANK_WILDCARD = 20;

    public ClassicCard(Suite suite, int rank) {
        if (suite == null)
            throw new NullPointerException("Suite cannot be null");
        if (!suite.isWildcard() && rank == RANK_WILDCARD)
            throw new IllegalArgumentException("Rank cannot be RANK_WILDCARD when suite is " + suite);
        this.suite = suite;
        this.rank = rank;
    }

    public int getRank() {
        return rank;
    }
    public int getRankWithAceValue(AceValue aceValue) {
        if (isAce())
            return aceValue.getAceValue();
        return rank;
    }
    public boolean isAce() {
        return this.rank == RANK_ACE_LOW || this.rank == RANK_ACE_HIGH;
    }
    public boolean isWildcard() {
        return suite.isWildcard();
    }
    public Suite getSuite() {
        return suite;
    }
}

AceValue.java

public enum AceValue {
    LOW(ClassicCard.RANK_ACE_LOW), HIGH(ClassicCard.RANK_ACE_HIGH);

    private final int aceValue;
    private final int minRank;
    private final int maxRank;
    private final int[] ranks;

    private AceValue(int value) {
        this.aceValue = value;
        this.minRank = Math.min(2, getAceValue());
        this.maxRank = Math.max(ClassicCard.RANK_KING, getAceValue());
        this.ranks = new int[52 / 4];
        for (int i = 0; i < ranks.length; i++)
            ranks[i] = this.minRank + i;
    }

    public int getMaxRank() {
        return maxRank;
    }
    public int getMinRank() {
        return minRank;
    }
    public int getAceValue() {
        return this.aceValue;
    }

    public int[] getRanks() {
        return Arrays.copyOf(this.ranks, this.ranks.length);
    }
}

Suite.java

public enum Suite {

    SPADES, HEARTS, DIAMONDS, CLUBS, EXTRA;

    public boolean isBlack() {
        return this.ordinal() % 2 == 0 && !isWildcard();
    }
    public boolean isWildcard() {
        return this == EXTRA;
    }
    public boolean isRed() {
        return !isBlack() && !isWildcard();
    }
    public static int suiteCount(boolean includingWildcards) {
        int i = 0;
        for (Suite suite : Suite.values()) {
            if (!suite.isWildcard() || includingWildcards) {
                ++i;
            }
        }
        return i;
    }
}

The Code

Total length: 309 lines of code (comments and whitespace excluded) in 8 files.

PokerFlush.java

/**
 * Checks for FLUSH, ROYAL_FLUSH and STRAIGHT_FLUSH. Depends on {@link PokerStraight} for the straight analyze.
 */
public class PokerFlush implements PokerHandResultProducer {

    private final PokerHandResultProducer straight = new PokerStraight();

    @Override
    public PokerHandResult resultFor(PokerHandAnalyze analyze) {
        List<PokerHandResult> results = new ArrayList<PokerHandResult>();
        for (Suite suite : Suite.values()) {
            if (suite.isWildcard())
                continue;

            PokerHandAnalyze suiteHand = analyze.filterBySuite(suite);
            if (suiteHand.size() < HAND_SIZE)
                continue; // Not enough cards to make a complete hand

            // We have a complete hand, now let's create a HandResult for it.
            PokerHandResult straightResult = straight.resultFor(suiteHand);
            if (straightResult != null) {
                PokerHandType type = straightResult.getPrimaryRank() == AceValue.HIGH.getAceValue() ? PokerHandType.ROYAL_FLUSH : PokerHandType.STRAIGHT_FLUSH;
                results.add(new PokerHandResult(type, straightResult.getPrimaryRank(), 0, null)); // We have a straight so we don't need to provide any kickers.
            }
            else results.add(new PokerHandResult(PokerHandType.FLUSH, 0, 0, suiteHand.getCards()));
        }
        if (results.isEmpty())
            return null;

        return PokerHandResult.returnBest(results);
    }

}

PokerHandAnalyze.java

/**
 * A helper class to analyze ranks and suits for an array of {@link ClassicCard}s. Create new using the static method {@link #analyze(ClassicCard...)}
 */
public class PokerHandAnalyze {

    private final int[] ranks = new int[ClassicCard.RANK_ACE_HIGH];
    private final int[] suites = new int[Suite.values().length];
    private final ClassicCard[] cards;
    private int wildcards;

    private PokerHandAnalyze(ClassicCard[] cards2) {
        this.cards = Arrays.copyOf(cards2, cards2.length);
    }
    /**
     * Create a new instance and analyze the provided cards
     * @param cards The cards to analyze
     * @return Organized analyze of the provided cards
     */
    public static PokerHandAnalyze analyze(ClassicCard... cards) {
        PokerHandAnalyze hand = new PokerHandAnalyze(cards);
        for (ClassicCard card : cards) {
            if (card.isWildcard()) {
                hand.wildcards++;
            }
            else if (card.isAce()) {
                hand.ranks[AceValue.HIGH.getAceValue() - 1]++;
                hand.ranks[AceValue.LOW.getAceValue() - 1]++;
            }
            else hand.ranks[card.getRank() - 1]++;

            hand.suites[card.getSuite().ordinal()]++;
        }
        return hand;
    }

    public int[] getRanks() {
        return ranks;
    }
    public int getWildcards() {
        return wildcards;
    }
    public ClassicCard[] getCards() {
        return cards;
    }
    public int size() {
        return cards.length;
    }
    /**
     * Create a sub-analyze which only includes wildcards and the specified suite. Useful to check for the FLUSH {@link PokerHandType}
     * @param suite The suite to filter by
     * @return A new analyze object
     */
    public PokerHandAnalyze filterBySuite(Suite suite) {
        List<ClassicCard> cards = new ArrayList<ClassicCard>();
        for (ClassicCard card : this.cards) {
            if (card.isWildcard() || card.getSuite().equals(suite)) {
                cards.add(card);
            }
        }
        return analyze(cards.toArray(new ClassicCard[cards.size()]));
    }
}

PokerHandEval.java

/**
 * Class to analyze poker hands by using a collection of {@link PokerHandResultProducer}s and return a {@link PokerHandResult}
 */
public class PokerHandEval {
    public void addTest(PokerHandResultProducer test) {
        this.tests.add(test);
    }

    private final List<PokerHandResultProducer> tests = new ArrayList<PokerHandResultProducer>();

    private PokerHandResult evaluate(PokerHandAnalyze analyze) {
        if (tests.isEmpty())
            throw new IllegalStateException("No PokerHandResultProducers added.");
        List<PokerHandResult> results = new ArrayList<PokerHandResult>();

        for (PokerHandResultProducer test : tests) {
            PokerHandResult result = test.resultFor(analyze);
            if (result != null)
                results.add(result);
        }
        return PokerHandResult.returnBest(results);
    }

    /**
     * Test a bunch of cards and return the best matching 5-card {@link PokerHandResult}
     * @param cards The cards to test
     * @return The best matching 5-card Poker Hand
     */
    public PokerHandResult test(ClassicCard... cards) {
        return evaluate(PokerHandAnalyze.analyze(cards));
    }

    /**
     * Factory method to create an evaluator for the default poker hand types.
     * @return
     */
    public static PokerHandEval defaultEvaluator() {
        PokerHandEval eval = new PokerHandEval();
        eval.addTest(new PokerPair());
        eval.addTest(new PokerStraight());
        eval.addTest(new PokerFlush());
        return eval;
    }
}

PokerHandResult.java

/**
 * Data for a found poker hand. Provides data for the type of poker hand, the primary rank and secondary rank, and kickers. Including methods for sorting. Also implements hashCode and equals.
 */
public class PokerHandResult implements Comparable<PokerHandResult> {
    private final PokerHandType type;
    private final int primaryRank;
    private final int secondaryRank;
    private final int[] kickers;

    public PokerHandResult(PokerHandType type, int primaryRank, int secondaryRank, ClassicCard[] cards) {
        this(type, primaryRank, secondaryRank, cards, PokerHandResultProducer.HAND_SIZE);
    }
    public PokerHandResult(PokerHandType type, int primaryRank, int secondaryRank, ClassicCard[] cards, int numKickers) {
        this.type = type;
        this.primaryRank = primaryRank;
        this.secondaryRank = secondaryRank;
        this.kickers = kickers(cards, new int[]{ primaryRank, secondaryRank }, numKickers);
        Arrays.sort(this.kickers);
    }


    @Override
    public int hashCode() {
        final int prime = 31;
        int result = 1;
        result = prime * result + ((type == null) ? 0 : type.hashCode());
        result = prime * result + primaryRank;
        result = prime * result + secondaryRank;
        result = prime * result + Arrays.hashCode(kickers);
        return result;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (!(obj instanceof PokerHandResult))
            return false;
        PokerHandResult other = (PokerHandResult) obj;
        if (type != other.type)
            return false;
        if (primaryRank != other.primaryRank)
            return false;
        if (secondaryRank != other.secondaryRank)
            return false;
        if (!Arrays.equals(kickers, other.kickers))
            return false;
        return true;
    }

    private static int compareKickers(int[] sorted1, int[] sorted2) {
        int index1 = sorted1.length - 1;
        int index2 = sorted2.length - 1;
        int compare = 0;

        while (compare == 0 && index1 >= 0 && index2 >= 0) {
            // If one of them is bigger than another we will stop comparing, so decreasing both indexes is perfectly OK.
            compare = Integer.compare(sorted1[index1--], sorted2[index2--]);
        }
        return compare;
    }

    @Override
    public int compareTo(PokerHandResult other) {
        // compare order: HandType, primary rank (int), secondary (used for two pair and full house), kickers
        int compare = this.type.compareTo(other.type);
        if (compare == 0)
            compare = Integer.compare(this.primaryRank, other.primaryRank);
        if (compare == 0)
            compare = Integer.compare(this.secondaryRank, other.secondaryRank);
        if (compare == 0)
            compare = compareKickers(this.kickers, other.kickers);
        return compare;
    }
    public PokerHandType getType() {
        return type;
    }
    /**
     * Return the best {@link PokerHandResult} of a list of results. The method first orders the list and then returns the last result.
     * @param results A list of PokerHandResults
     * @return The best result from the list
     */
    public static PokerHandResult returnBest(List<PokerHandResult> results) {
        if (results.isEmpty())
            return null;
        Collections.sort(results);
        return results.get(results.size() - 1);
    }

    /**
     * Create an integer array of "kickers", to separate FOUR_OF_A_KIND with Ace-kicker vs. King-kicker
     * @param cards The cards in your hand. If null, an empty array will be returned
     * @param skip Ranks that will be skipped (for example, if you have a pair of 4s then you can skip those 4s)
     * @param count How many kickers that should be included. This should ideally be 5 - number of cards required for the {@link PokerHandType} the kickers are provided for
     * @return An array of the ranks that will be used as kickers. Wildcards and the ranks in the skip array are excluded
     */
    private static int[] kickers(ClassicCard[] cards, int[] skip, int count) {
        if (cards == null)
            return new int[]{};
        int[] result = new int[cards.length];
        Arrays.sort(skip);
        for (int i = 0; i < cards.length; i++) {
            int rank = cards[i].getRankWithAceValue(AceValue.HIGH);
            // Check if we should skip this rank in the kicker-data.
            if (cards[i].isWildcard() || Arrays.binarySearch(skip, rank) >= 0)
                continue;
            result[i] = rank;
        }
        Arrays.sort(result);
        return Arrays.copyOfRange(result, Math.max(result.length - count, 0), result.length);
    }

    public int getPrimaryRank() {
        return primaryRank;
    }

    public int getSecondaryRank() {
        return secondaryRank;
    }

    @Override
    public String toString() {
        return String.format("PokerHand: %s. %d, %d. Kickers: %s", type, primaryRank, secondaryRank, Arrays.toString(kickers));
    }
}

PokerHandResultProducer.java

/**
 * Interface for scanning for Poker hands.
 */
public interface PokerHandResultProducer {
    /**
     * Constant for how big our hands should be.
     */
    final int HAND_SIZE = 5;
    /**
     * Method which does the job of finding a matching Poker hand for some analyze data.
     * @param analyze {@link PokerHandAnalyze} object containing data for which we should try to find a matching Poker hand.
     * @return {@link PokerHandResult} for the best poker hand we could find.
     */
    PokerHandResult resultFor(PokerHandAnalyze analyze);
}

PokerHandType.java

public enum PokerHandType {
    HIGH_CARD, PAIR, TWO_PAIR, THREE_OF_A_KIND, STRAIGHT, FLUSH, FULL_HOUSE, FOUR_OF_A_KIND, STRAIGHT_FLUSH, ROYAL_FLUSH;
}

PokerPair.java

/**
 * Checks for PAIR, THREE_OF_A_KIND, FOUR_OF_A_KIND, and FULL_HOUSE. Returns HIGH_CARD if nothing better was found.
 */
public class PokerPair implements PokerHandResultProducer {

    @Override
    public PokerHandResult resultFor(PokerHandAnalyze analyze) {
        List<PokerHandResult> results = new ArrayList<PokerHandResult>();
        List<PokerHandResult> pairs = new ArrayList<PokerHandResult>();
        List<PokerHandResult> threeOfAKinds = new ArrayList<PokerHandResult>();
        int[] ranks = analyze.getRanks();
        int remainingWildcards = analyze.getWildcards();

        // Find out how many we should look for, primarily
        int[] sortedCounts = Arrays.copyOf(ranks, ranks.length);
        Arrays.sort(sortedCounts);
        int countForWildcards = sortedCounts[sortedCounts.length - 1];

        for (int index = ranks.length - 1; index >= 0; index--) {
            int count = ranks[index];
            int useWildcards = (count == countForWildcards ? remainingWildcards : 0);
            if (count + useWildcards >= 4) {
                remainingWildcards += count - 4;
                results.add(new PokerHandResult(PokerHandType.FOUR_OF_A_KIND, index + 1, 0, analyze.getCards(), 1));
            }

            // If there already exists some four of a kinds, then there's no need to check three of a kinds or pairs.
            if (!results.isEmpty())
                continue;

            if (count + useWildcards == 3) {
                remainingWildcards += count - 3;
                threeOfAKinds.add(new PokerHandResult(PokerHandType.THREE_OF_A_KIND, index + 1, 0, analyze.getCards(), 2));
            }
            else if (count + useWildcards == 2) {
                remainingWildcards += count - 2;
                pairs.add(new PokerHandResult(PokerHandType.PAIR, index + 1, 0, analyze.getCards(), 3));
            }
        }

        return checkForFullHouseAndStuff(analyze, pairs, threeOfAKinds, results);
    }

    private PokerHandResult checkForFullHouseAndStuff(PokerHandAnalyze analyze, List<PokerHandResult> pairs, List<PokerHandResult> threeOfAKinds, List<PokerHandResult> results) {
        if (!results.isEmpty())
            return PokerHandResult.returnBest(results);

        PokerHandResult bestPair = PokerHandResult.returnBest(pairs);
        PokerHandResult bestThree = PokerHandResult.returnBest(threeOfAKinds);
        if (bestPair != null && bestThree != null) {
            return new PokerHandResult(PokerHandType.FULL_HOUSE, bestThree.getPrimaryRank(), bestPair.getPrimaryRank(), null, 0); // No kickers because it's a complete hand.
        }
        if (bestThree != null)
            return bestThree;

        if (pairs.size() >= 2) {
            Collections.sort(pairs);
            int a = pairs.get(pairs.size() - 1).getPrimaryRank();
            int b = pairs.get(pairs.size() - 2).getPrimaryRank();
            return new PokerHandResult(PokerHandType.TWO_PAIR, Math.max(a, b), Math.min(a, b), analyze.getCards(), 1);
        }

        if (bestPair != null)
            return bestPair;

        // If we have a wildcard, then we always have at least PAIR, which means that it's fine to ignore wildcards in the kickers here as well 
        return new PokerHandResult(PokerHandType.HIGH_CARD, 0, 0, analyze.getCards());
    }

}

PokerStraight.java - (removed) because of question size limit. See separate question

The Test

PokerHandTest.java (175 lines)

public class PokerHandTest {
    private static final ClassicCard WILDCARD = new ClassicCard(Suite.EXTRA, ClassicCard.RANK_WILDCARD);

    private int findHighestIndexForStraight(int[] ranks, int wildcards) {
        int res = PokerStraight.findHighestIndexForStraight(ranks, wildcards);
        return res == -1 ? res : res - 1;
    }

    @Test
    public void moreCards() {
        PokerHandEval eval = PokerHandEval.defaultEvaluator();
        assertPoker(PokerHandType.THREE_OF_A_KIND, 2, eval.test(card(DIAMONDS, RANK_JACK), card(HEARTS, RANK_ACE_HIGH), card(SPADES, RANK_7),
                card(DIAMONDS, RANK_2), card(HEARTS, RANK_2), card(DIAMONDS, RANK_4), WILDCARD));

        assertPoker(PokerHandType.TWO_PAIR, 12, 10, eval.test(card(DIAMONDS, RANK_3), card(SPADES, RANK_2), card(DIAMONDS, RANK_10),
                card(CLUBS, RANK_10), card(CLUBS, RANK_7), card(SPADES, RANK_5), 
                card(SPADES, RANK_QUEEN), card(HEARTS, RANK_QUEEN), 
                card(HEARTS, RANK_ACE_HIGH), card(DIAMONDS, RANK_7)));

        PokerHandResult eq1;
        PokerHandResult eq2;
        eq1 = eval.test(card(CLUBS, RANK_10), card(CLUBS, RANK_7), card(SPADES, RANK_KING), 
                card(SPADES, RANK_QUEEN), card(HEARTS, RANK_QUEEN), card(HEARTS, RANK_ACE_HIGH), card(DIAMONDS, RANK_ACE_HIGH));
        eq2 = eval.test(card(CLUBS, RANK_JACK), card(CLUBS, RANK_7), card(SPADES, RANK_KING), 
                card(SPADES, RANK_QUEEN), card(HEARTS, RANK_QUEEN), card(HEARTS, RANK_ACE_HIGH), card(DIAMONDS, RANK_ACE_HIGH));
        assertEquals(eq1, eq2);

        eq1 = eval.test(WILDCARD, card(SPADES, RANK_QUEEN), card(HEARTS, RANK_QUEEN),   card(HEARTS, RANK_7), card(DIAMONDS, RANK_4));
        eq2 = eval.test(card(DIAMONDS, RANK_QUEEN), card(CLUBS, RANK_QUEEN), card(SPADES, RANK_QUEEN),  card(HEARTS, RANK_7), card(DIAMONDS, RANK_4));
        assertPoker(PokerHandType.THREE_OF_A_KIND, RANK_QUEEN, eq1);
        assertEquals(eq2, eq1);

        PokerHandResult result;
        result = eval.test(WILDCARD, WILDCARD, WILDCARD, WILDCARD, card(DIAMONDS, RANK_6));
        assertPoker(PokerHandType.STRAIGHT_FLUSH, result);

        result = eval.test(WILDCARD, WILDCARD, WILDCARD, card(HEARTS, RANK_10), card(DIAMONDS, RANK_6));
        assertPoker(PokerHandType.FOUR_OF_A_KIND, result);

        result = eval.test(WILDCARD, WILDCARD, WILDCARD, WILDCARD, WILDCARD);
        assertPoker(PokerHandType.ROYAL_FLUSH, result);

        result = eval.test(WILDCARD, WILDCARD, WILDCARD, WILDCARD, card(SPADES, RANK_10));
        assertPoker(PokerHandType.ROYAL_FLUSH, result);
    }
    public void assertPoker(PokerHandType type, int primary, PokerHandResult test) {
        assertPoker(type, test);
        assertEquals(primary, test.getPrimaryRank());
    }
    public void assertPoker(PokerHandType type, int primary, int secondary, PokerHandResult test) {
        assertPoker(type, primary, test);
        assertEquals(secondary, test.getSecondaryRank());
    }

    @Test
    public void royalAndFlushStraights() {
        PokerHandEval eval = PokerHandEval.defaultEvaluator();
        PokerHandResult result = eval.test(card(HEARTS, RANK_10), card(HEARTS, RANK_JACK), card(HEARTS, RANK_QUEEN), card(HEARTS, RANK_KING), card(HEARTS, RANK_ACE_LOW));
        assertPoker(PokerHandType.ROYAL_FLUSH, result);

        result = eval.test(card(HEARTS, RANK_2), card(HEARTS, RANK_3), card(HEARTS, RANK_4), card(HEARTS, RANK_5), card(HEARTS, RANK_6));
        assertPoker(PokerHandType.STRAIGHT_FLUSH, result);
    }

    @Test
    public void rankHands() {
        PokerHandEval eval = PokerHandEval.defaultEvaluator();
        PokerHandResult highCard       = eval.test(card(HEARTS, RANK_7), card(CLUBS, RANK_JACK), card(HEARTS, RANK_6), card(HEARTS, RANK_4), card(DIAMONDS, RANK_2));

        PokerHandResult pairLowKicker  = eval.test(card(HEARTS, RANK_7), card(CLUBS, RANK_7), card(HEARTS, RANK_6), card(HEARTS, RANK_4), card(HEARTS, RANK_2));
        PokerHandResult pairHighKicker = eval.test(card(HEARTS, RANK_7), card(CLUBS, RANK_7), card(HEARTS, RANK_KING), card(HEARTS, RANK_4), card(HEARTS, RANK_2));
        PokerHandResult pairHigher     = eval.test(card(HEARTS, RANK_KING), card(CLUBS, RANK_KING), card(HEARTS, RANK_6), card(HEARTS, RANK_4), card(HEARTS, RANK_2));

        PokerHandResult twoPair        = eval.test(card(HEARTS, RANK_KING), card(CLUBS, RANK_KING), card(HEARTS, RANK_6), card(DIAMONDS, RANK_6), card(HEARTS, RANK_2));
        PokerHandResult threeOfAKind   = eval.test(card(HEARTS, RANK_KING), card(CLUBS, RANK_KING), card(SPADES, RANK_KING), card(HEARTS, RANK_4), card(HEARTS, RANK_2));

        PokerHandResult flush         = eval.test(card(HEARTS, RANK_7), card(HEARTS, RANK_2), card(HEARTS, RANK_6), card(HEARTS, RANK_9), card(HEARTS, RANK_QUEEN));
        PokerHandResult fourOfAKind    = eval.test(card(HEARTS, RANK_7), card(SPADES, RANK_7), card(DIAMONDS, RANK_7), card(CLUBS, RANK_7), card(HEARTS, RANK_QUEEN));

        PokerHandResult straight       = eval.test(card(HEARTS, RANK_2), card(CLUBS, RANK_3), card(HEARTS, RANK_4), card(HEARTS, RANK_5), card(DIAMONDS, RANK_6));
        PokerHandResult straightWild   = eval.test(card(HEARTS, RANK_2), card(CLUBS, RANK_3), WILDCARD, card(HEARTS, RANK_5), card(DIAMONDS, RANK_6));
        assertEquals(straight, straightWild);
        PokerHandResult straightLow       = eval.test(card(HEARTS, RANK_ACE_HIGH), card(CLUBS, RANK_2), card(HEARTS, RANK_3), card(HEARTS, RANK_4), card(DIAMONDS, RANK_5));

        PokerHandResult straightFlush  = eval.test(card(HEARTS, RANK_8), card(HEARTS, RANK_9), card(HEARTS, RANK_10), card(HEARTS, RANK_JACK), card(HEARTS, RANK_QUEEN));
        PokerHandResult royalFlush     = eval.test(card(HEARTS, RANK_10), card(HEARTS, RANK_JACK), card(HEARTS, RANK_QUEEN), card(HEARTS, RANK_KING), WILDCARD);

        PokerHandResult fullHouse      = eval.test(card(HEARTS, RANK_10), card(CLUBS, RANK_10), WILDCARD, card(HEARTS, RANK_KING), card(HEARTS, RANK_KING));
        assertPoker(PokerHandType.FULL_HOUSE, fullHouse);
        assertEquals(RANK_KING, fullHouse.getPrimaryRank());
        assertEquals(RANK_10, fullHouse.getSecondaryRank());

        // Add hands to list
        List<PokerHandResult> results = new ArrayList<PokerHandResult>();
        assertAdd(results, PokerHandType.HIGH_CARD, highCard);
        assertAdd(results, PokerHandType.PAIR, pairLowKicker);
        assertAdd(results, PokerHandType.PAIR, pairHighKicker);
        assertAdd(results, PokerHandType.PAIR, pairHigher);
        assertAdd(results, PokerHandType.TWO_PAIR, twoPair);
        assertAdd(results, PokerHandType.THREE_OF_A_KIND, threeOfAKind);
        assertAdd(results, PokerHandType.FLUSH, flush);
        assertAdd(results, PokerHandType.FOUR_OF_A_KIND, fourOfAKind);
        assertAdd(results, PokerHandType.STRAIGHT, straightLow);
        assertAdd(results, PokerHandType.STRAIGHT, straight);
        assertAdd(results, PokerHandType.STRAIGHT, straightWild);
        assertAdd(results, PokerHandType.STRAIGHT_FLUSH, straightFlush);
        assertAdd(results, PokerHandType.ROYAL_FLUSH, royalFlush);

        // Shuffle just for the fun of it
        Collections.shuffle(results);

        // Sort the list according to the HandResult comparable interface
        Collections.sort(results);

        // Assert the list
        Iterator<PokerHandResult> it = results.iterator();
        assertEquals(highCard, it.next());
        assertEquals(pairLowKicker, it.next());
        assertEquals(pairHighKicker, it.next());
        assertEquals(pairHigher, it.next());

        assertEquals(twoPair, it.next());
        assertEquals(threeOfAKind, it.next());

        assertEquals(straightLow, it.next());

        assertEquals(straight, it.next());
        assertEquals(straightWild, it.next());

        assertEquals(flush, it.next());
        assertEquals(fourOfAKind, it.next());
        assertEquals(straightFlush, it.next());
        assertEquals(royalFlush, it.next());


        // Make sure that we have processed the entire list
        assertFalse("List is not completely processed", it.hasNext()); 
    }

    private static void assertAdd(List<PokerHandResult> results, PokerHandType type, PokerHandResult result) {
        assertPoker(type, result);
        results.add(result);
    }

    private static void assertPoker(PokerHandType type, PokerHandResult result) {
        if (type == null) {
            assertNull(result);
            return;
        }
        assertNotNull("Expected " + type, result);
        assertEquals(result.toString(), type, result.getType());
    }


    private static ClassicCard card(Suite suite, int rank) {
        return new ClassicCard(suite, rank);
    }

}

Code Review Question(s)

I have tried to make good use of strategy pattern and some factory methods (PokerHandAnalyze and PokerHandEval), I'd like to know if these are "correct". If you see any other common patterns that I have used without knowing it, I'd also like to hear that.

But most importantly: Is there anything that I could have done better here? And can you find any mistakes?

Answer 1

AceValue

This class has me confused. I am not sure what the ranks[] is for.... and there are magic numbers, and no comments?

Suite

This has an isBlack() method, but that indicates that DIAMONDS are black... but they are red!

suiteCount(boolean) could be much simpler:

return Suite.values - (includingWildcards ? 0 : 1);

General

Right, after this, it becomes very complicated to review... Frankly too much to understand in 30 minutes or less... and pulling the code locally so I can run it through is not easy as well... and by the time I decided to do that, I had spent too long on it already. This, in itself, is an interesting observation. I tried to read through the classes, and understand how 'it hangs together', and then decided that the only way to understand the code is to 'debug' it and step through.

Then I realized there's no main method, and I guess you use JUnit to run it?

Finally, I have just never played poker... (not even strip...) so I don't have an instinctual idea of where the code should be going.

Bottom line is that the code requires a native/instinctual understanding of the problem in order to make sense of the code.

This in itself suggests there is a structural/presentation problem.

The only reasonable thing to do is a face-value review, and not as in-depth as I would like...

PokerFlush

I dislike constants that are pulled 'implicitly' from other classes. In this case, HAND_SIZE 'magically' appears, and I see it is a constant in the PokerHandResultProducer interface the PokerFlush class implements. This is a poor location for the HAND_SIZE, and a poor way to reference it. It should be something like PokerRules.HAND_SIZE.

I don't like the results List and the PokerHandResult.returnBest combination. I think there is a better way. A class called Best<T> that simply chooses the best hand as hands are added... and then has a best() method. Your code becomes:

Best<PokerHandResult> results = new Best<PokerHandResult>();
....
return results.best();

The PokerHandResult instances have a 'natural order' (implement Comparable), so the generic class Best<T> only has to run the compareTo() method and keep the best.

PokerHandAnalyze

private final int[] ranks = new int[ClassicCard.RANK_ACE_HIGH]

it took me some minutes to understand that... I know it may make sense to you, but, even when I look at it carefully, I cannot understand why it's not ClassicCard.RANK_WILDCARD. At minimum this line needs a good comment, better would be a constant with a better name.... i.e. wtf does RANK_ACE_HIGH have to do with an array-size?

I know you use the factory method here, but it is being used wrong...

Factory method is good for obscuring the physical implementation of an interface, or for creating otherwise complicated immutables. In this case, the factory method is really just making the constructor simpler.... by moving all the logic to the factory method. This does not, in fact make the class simpler, but just moves the constructor logic in to a non-logical place.

In my assessment, that factory method should be removed and the logic moved to the constructor. Then make all the instance-fields final, and you have a nice immutable class. If you really want to keep the factory method, then your fields should still be final, but you need to pass them all in to the constructor.

getRanks() should return a copy of the array so that it's immutable state is preserved (you have marked the array as final, but you are not protecting the final-state of the data in the array)

As far as I can tell, the suites[] array is 'dead' code (completely unused).

getCards() should also protect the data by returning a copy of the array.

PokerHandEval

Never, under any circumstance, use classes/methods with the name 'test' unless it is designed to test your code (not test the data).... ;-) This just leads to confusion. The Class is called "...Eval" so why can you not use evaluate or some variant?

From what I can tell, your tests are ordered in the reverse order to what they should be... you look for the lowest-ranking results first, and then the increasingly-higher results. If you reverse the order, you can 'short-circuit' the process and exit when you have your first result (i.e. why should you look for a pair if you already have a flush?).

Again, here you could also use the 'Best' class to only keep the best result.

PokerHandResult

Looks like a reasonably clean immutable class... but should be declared final (public final class PokerHandResult)

I don't like the returnBest(List<PokerHandResult> results) on this class. Should be replaced with Best<T> or moved to a central PokerRules static class

PokerHandResultProducer

As I have said, the HAND_SIZE does not belong here. Otherwise it's a fine interface (not much to it...).

PokerHandType

fine ... ;-)

PokerPair

You have split this method in two parts, and it's just a cheap way to reduce 'complexity'. But, in reality, it has added complexity.... checkForFullHouseAndStuff ... really? There is no added value from that method, and it just creates two levels of 'return' to the user. Sometimes methods just have to be complicated....

The first thing that does is return if another result was already found. Other than that, it does not use the results list.

Like other occasions, a Best<T> class would be useful... for results, pairs, and threeOfAKind.

Conclusion

I can't really help too much on your questions about other patterns used, and as for how you use the strategy and factory patterns, I feel the strategy is fine (except for the HAND_SIZE and the order-of-evaluation). The Factory patterns are cumbersome in places.

I did not see any bugs that would affect your game play. The Black-Diamond is the only issue I see... (and that's good for skiing).

Answer 2

Almost four years later... I discovered a bug. Consider the following tests:

assertEquals(PokerHandType.THREE_OF_A_KIND, eval.evaluate(
    card(Suite.CLUBS, ClassicCard.RANK_ACE_HIGH),
    card(Suite.HEARTS, ClassicCard.RANK_5),
    card(Suite.SPADES, ClassicCard.RANK_KING),
    card(Suite.DIAMONDS, ClassicCard.RANK_ACE_HIGH),
    card(Suite.SPADES, ClassicCard.RANK_ACE_HIGH)
).getType());

That's a three of a kind and should pass, right? Actually yes, it does.

Next one:

assertEquals(PokerHandType.THREE_OF_A_KIND, eval.evaluate(
    card(Suite.CLUBS, ClassicCard.RANK_ACE_HIGH),
    card(Suite.SPADES, ClassicCard.RANK_ACE_HIGH),
    card(Suite.SPADES, ClassicCard.RANK_KING),
    card(Suite.HEARTS, ClassicCard.RANK_8),
    card(Suite.HEARTS, ClassicCard.RANK_5),
    card(Suite.SPADES, ClassicCard.RANK_2),
    card(Suite.EXTRA, ClassicCard.RANK_WILDCARD)
).getType());

Oh, a wildcard! But this is still considered a three of a kind, right? Wrong. It should be a three of a kind, but the code considers this a full house because ace can be both high (14) and low (1), so we have a full house with three 14's and two 1's.

We can even get rid of a few extra cards and take a look at this one:

assertEquals(PokerHandType.THREE_OF_A_KIND, eval.evaluate(
    card(Suite.CLUBS, ClassicCard.RANK_ACE_HIGH),
    card(Suite.SPADES, ClassicCard.RANK_ACE_HIGH),
    card(Suite.EXTRA, ClassicCard.RANK_WILDCARD)
).getType());

The code still considers this a full house of both high and low aces.

How about something simpler then?

assertEquals(PokerHandType.PAIR, eval.evaluate(
    card(Suite.CLUBS, ClassicCard.RANK_ACE_HIGH),
    card(Suite.DIAMONDS, ClassicCard.RANK_ACE_HIGH)
).getType());

That's got to be one pair right? Nope, the code says that is two pair. Again, two pair of both high and low aces.

So what is a possible fix for this? In checkForFullHouseAndStuff in PokerPair, pass the results first through the following method:

private PokerHandResult fixDoubleAce(PokerHandResult result, PokerHandAnalyze analyze) {
    if (result.getPrimaryRank() != ClassicCard.RANK_ACE_HIGH || result.getSecondaryRank() != ClassicCard.RANK_ACE_LOW) {
        return result;
    }
    switch (result.getType()) {
        case FULL_HOUSE:
            return new PokerHandResult(PokerHandType.THREE_OF_A_KIND, result.getPrimaryRank(), 0, analyze.getCards());
        case TWO_PAIR:
            return new PokerHandResult(PokerHandType.PAIR, result.getPrimaryRank(), 0, analyze.getCards());
        default:
            throw new IllegalStateException("Unexpected scenario cleaning double ace from " + result);
    }
}

This method first checks if the result is for both high and low aces and if it is, it returns a degraded poker hand of only the primary rank. So full house becomes three of a kind and two pair becomes one pair.