Determining winner and winning hand in poker (holdem)

Question

I wrote couple classes that calculate winner, winning hand and winning hands rank (straight, flush, fullhouse etc.). I searched for something similar, but only found posts where five cards are used, but in a holdem there are cards 7 cards and have to choose best 5 of them.

How it works:

You pass List<Card> cards to HandRanker::getRank method and retrieve an object PokerHand that contains a hand rank(flush, straight etc) and cards that contains that hand.

In HandRanker class it goes into each ranks method and updates handCards and handValue fields and if they do so method returns true and handValue object is returned.

---

I think it's all working fine. When I was writing code I did a little testing, but haven't written any unit tests yet.

HandRanker.java

    package poker;

import static deck.Card.*;
import static poker.PokerHand.*;
import static java.util.stream.Collectors.*;

import deck.Card;

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

public class HandRanker {
    public static final int FULL_HAND = 5;
    public static final int FLUSH = FULL_HAND;
    public static final int STRAIGHT = FULL_HAND;
    public static final int FULL_HOUSE = FULL_HAND;
    public static final int QUADS = 4;
    public static final int SET = 3;
    public static final int PAIR = 2;
    public static final int HIGH_CARD = 1;

    public static final List<EnumSet<RANK>> STRAIGHTS = setPossibleStraights();
    private static final HandRanker instance = new HandRanker();

    public static HandRanker getInstance() {
        return instance;
    }

    private List<Card> handCards = new ArrayList<>();
    private PokerHand pokerHand;

    private HandRanker() {}

    private static List<EnumSet<RANK>> setPossibleStraights() {
        List<EnumSet<RANK>> straights =  EnumSet.range(RANK.TWO, RANK.TEN).stream()
            .map(rank -> EnumSet.range(rank, RANK.values()[rank.ordinal() + STRAIGHT - 1]))
            .collect(toList());
        Collections.reverse(straights);
        straights.add(EnumSet.of(RANK.ACE, RANK.TWO, RANK.THREE, RANK.FOUR, RANK.FIVE));
        return Collections.unmodifiableList(straights);
    }

    public PokerHand getRank(List<Card> allCards) {
        if(allCards.size() != 7) throw new IllegalArgumentException("You have to pass 7 cards");

        if(isStraightFlush(allCards) ||
               isFourOfAKind(allCards) ||
               isFullHouse(allCards) ||
               isFlush(allCards, true) ||
               isStraight(allCards) ||
               isSet(allCards) ||
               isTwoPair(allCards) ||
               isPair(allCards) ||
               isHighCard(allCards)) {}
        return pokerHand;
    }


    private boolean isStraightFlush(List<Card> allCards) {
        if(isFlush(allCards, false) && isStraight(handCards)) {
            pokerHand = new PokerHand(HAND_RANK.STRAIGHT_FLUSH, handCards);
            return true;
        }
        return false;
    }

    private boolean isFlush(List<Card> allCards, boolean finalResult) {
        Map<SUIT, Long> suitsMap = getSuitMap(allCards);
        SUIT popularSuit = getMostPopularSuit(suitsMap);

        if(suitsMap.get(popularSuit) >= FLUSH) {

            if(finalResult) {
                handCards = allCards.stream()
                    .filter(c -> c.getSuit() == popularSuit)
                    .sorted()
                    .limit(FULL_HAND)
                    .collect(Collectors.toList());

                pokerHand = new PokerHand(HAND_RANK.FLUSH, handCards);
            } else {
                handCards = allCards.stream()
                    .filter(c -> c.getSuit() == popularSuit)
                    .collect(Collectors.toList());
            }
            return true;
        }
        return false;
    }

    private boolean isStraight(List<Card> allCards) {
        EnumSet<RANK> ranks = allCards.stream()
            .map(Card::getRank)
            .collect(toCollection(() -> EnumSet.noneOf(RANK.class)));

        for(Set<RANK> straight : STRAIGHTS) {
            if(ranks.containsAll(straight))  {
                handCards = allCards.stream()
                    .filter(c -> straight.contains(c.getRank()))
                    .collect(toList());

                pokerHand = new PokerHand(HAND_RANK.STRAIGHT, handCards);
                return true;
            }
        }
        return false;
    }

    private boolean isFourOfAKind(List<Card> allCards) {
        handCards = getHighestCards(allCards, QUADS);

        if(handCards.size() == QUADS) {
            handCards.addAll(getMultipleHighestCards(allCards, FULL_HAND - QUADS));
            pokerHand = new PokerHand(HAND_RANK.FOUR_OF_A_KIND, handCards);
            return true;
        }
        return false;
    }

    private boolean isFullHouse(List<Card> allCards) {
        handCards = getHighestCards(allCards, SET);
        handCards.addAll(getHighestCards(allCards, PAIR));

        if(handCards.size() == FULL_HOUSE) {
            pokerHand = new PokerHand(HAND_RANK.FULL_HOUSE, handCards);
            return true;
        }
        return false;
    }

    private boolean isSet(List<Card> allCards) {
        handCards = getHighestCards(allCards, SET);

        if(handCards.size() == SET) {
            handCards.addAll(getMultipleHighestCards(allCards, FULL_HAND - SET));
            pokerHand = new PokerHand(HAND_RANK.THREE_OF_A_KIND, handCards);
            return true;
        }
        return false;
    }

    private boolean isTwoPair(List<Card> allCards) {
        handCards= getHighestCards(allCards, PAIR);
        allCards.removeAll(handCards);
        handCards.addAll(getHighestCards(allCards, PAIR));

        if(handCards.size() == PAIR + PAIR) {
            handCards.addAll(getMultipleHighestCards(allCards, FULL_HAND - PAIR - PAIR));
            pokerHand = new PokerHand(HAND_RANK.TWO_PAIR, handCards);
            return true;
        }
        return false;
    }

    private boolean isPair(List<Card> allCards) {
        handCards = getHighestCards(allCards, PAIR);

        if(handCards.size() == PAIR) {
            handCards.addAll(getMultipleHighestCards(allCards, FULL_HAND - PAIR));
            pokerHand = new PokerHand(HAND_RANK.PAIR, handCards);
            return true;
        }
        return false;
    }

    private boolean isHighCard(List<Card> allCards) {
        handCards = getHighestCards(allCards, FULL_HAND);
        pokerHand = new PokerHand(HAND_RANK.HIGH_CARD, handCards);
        return true;
    }

    private List<Card> getHighestCards(List<Card> allCards, int count) {
        EnumMap<RANK, Long> ranks = getRankMap(allCards);

        try {
            RANK cardsRank = ranks.entrySet().stream()
                .filter(entry -> entry.getValue() == count)
                .reduce((previous, current) -> current)
                .get().getKey(); //throws exception if there is not same rank cards with specified count

            return allCards.stream()
                .filter( c -> c.getRank() == cardsRank)
                .collect(toList());
        }
        catch(NoSuchElementException e) {
            return new ArrayList<>();
        }
    }

    private List<Card> getMultipleHighestCards(List<Card> allCards, int count) {
        List<Card> highestCards = new ArrayList<>(count);
        for(int index = 0; index < count; index++) {
            List<Card> cards = getHighestCards(allCards, HIGH_CARD);
            allCards.removeAll(cards);
            highestCards.addAll(cards);
        }
        return highestCards;
    }

    private SUIT getMostPopularSuit(Map<SUIT, Long> suits) {
        return suits.entrySet().stream()
            .max((lhs, rhs) -> lhs.getValue() > rhs.getValue() ? 1 : -1)
            .get().getKey();
    }

    private EnumMap<SUIT, Long> getSuitMap(List<Card> allCards) {
        return allCards.stream()
            .collect(groupingBy(
                    Card::getSuit,
                    () -> new EnumMap<>(SUIT.class),
                    counting()
                ));
    }

    private EnumMap<RANK, Long> getRankMap(List<Card> allCards) {
        return allCards.stream()
            .collect(groupingBy(
                    Card::getRank,
                    () -> new EnumMap<>(RANK.class),
                    counting()
                ));
    }
}

Card.java

package deck;

public class Card implements Comparable {

    public static enum SUIT {
        SPADES,
        HEARTS,
        CLUBS,
        DIAMONDS
    }

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

        private final int value;

        private RANK(int value) {
            this.value = value;
        }

        public int getValue() {
            return value;
        }
    }

    private final RANK rank;
    private final SUIT suit;

    public Card(RANK rank, SUIT suit) {
        this.rank = rank;
        this.suit = suit;
    }

    public RANK getRank() {
        return rank;
    }

    public SUIT getSuit() {
        return suit;
    }

    @Override
    public String toString() {
        return "Card{" +
            "rank=" + rank +
            ", suit=" + suit +
            '}';
    }

    @Override
    public int compareTo(Object o) {
        if(this == o) return 0;
        if(o == null || getClass() != o.getClass()) return 0;

        Card card = (Card) o;

        if(rank.getValue() > card.rank.getValue()) return -1;
        if(rank.getValue() < card.rank.getValue()) return 1;
        return 0;
    }

}

Pokerhand.java

package poker;

import deck.Card;

import java.util.Collections;
import java.util.List;

public class PokerHand implements Comparable {
    public static enum HAND_RANK {
        HIGH_CARD(0),
        PAIR(1),
        TWO_PAIR(2),
        THREE_OF_A_KIND(3),
        STRAIGHT(4),
        FLUSH(5),
        FULL_HOUSE(6),
        FOUR_OF_A_KIND(7),
        STRAIGHT_FLUSH(8);

        private final int value;

        HAND_RANK(int value) {
            this.value = value;
        }

        public int getValue() {
            return value;
        }
    }

    private final HAND_RANK handRank;
    private final List<Card> cards;

    public PokerHand(HAND_RANK handRank, List<Card> cards) {
        if(cards.size() != HandRanker.FULL_HAND) throw new IllegalArgumentException("You have to pass five cards");
        this.handRank = handRank;
        this.cards = cards;
        Collections.sort(this.cards); //sorts for compareTo method
    }

    @Override
    public String toString() {
        return "HandValue{" +
            "handRank=" + handRank +
            ", allCards=" + cards +
            '}';
    }

    @Override
    public int compareTo(Object o) {
        if(this == o) return 0;
        if(o == null || getClass() != o.getClass()) return 0;

        PokerHand pokerHand = (PokerHand) o;

        if(handRank.getValue() > pokerHand.handRank.getValue()) return 1;
        if(handRank.getValue() < pokerHand.handRank.getValue()) return -1;

        for(int index = 0; index < cards.size(); index++) {
            if(cards.get(index).getRank().getValue() > pokerHand.cards.get(index).getRank().getValue()) return 1;
            if(cards.get(index).getRank().getValue() < pokerHand.cards.get(index).getRank().getValue()) return -1;
        }
        return 0;
    }
}

1. Any suggestions how to do it better?

Should I write some between class for easier testing. Now it would look like this:

@Test
public void testStraight() {
List<Card> cards = new ArrayList();
cards.add(new Card(RANK.ACE, SUIT.DIAMONDS));
cards.add(new Card(RANK.ACE, SUIT.SPADES));
cards.add(new Card(RANK.ACE, SUIT.HEARTS));
....
....
assertEquals(HandRanker.getInstance(cards), resultCards);
}

You can imagine how much lines it would take to properly test everything.

2. How could I make HandRanker class structure better?

Now it seems very fragile. I'f I change one thing whole class breaks apart. This was the best way I could think of implementing:

 public PokerHand getRank(List<Card> allCards) {
        if(allCards.size() != 7) throw new IllegalArgumentException("You have to pass 7 cards");

   if(isStraightFlush(allCards) ||
           isFourOfAKind(allCards) ||
           isFullHouse(allCards) ||
           isFlush(allCards, true) ||
           isStraight(allCards) ||
           isSet(allCards) ||
           isTwoPair(allCards) ||
           isPair(allCards) ||
           isHighCard(allCards)) {}
    return pokerHand;
}

To me it seems I'm exploiting stuff that I shouldn't do. Is this a good approach? Is there something I haven't thought of?

Also relying on sorting when getting higher card at index 0 in a list seems wrong. If I ever change that Card class HandRanker class will fail.

3. Should I get rid of this sort of responsibilities?

4. Other things that are not industry standard?

Answer 1

Card

The names SUIT and RANK should be Suit and Rank. I'm not sure about your compareTo for two reasons:

• It's inconsistent with equals, which is allowed, but should be avoided if possible and really should be documented.
• It accepts any object, but should accept Card only

 

if(o == null || getClass() != o.getClass()) return 0;

This is wrong..., you're breaking both symmetry and transitivity here.

No need to compare all the world:

public class Card implements Comparable<Card> ... 

You probably should implement equals and hashCode. As there are only 52 cards, you could make the constructor private and provide a factory method returning one of the 52 pre-created instances. But you don't have to.

HandRanker

public static final int FULL_HAND = 5;

Whenever I see such an int, I think about enums. Something like

enum Value implements Predicate<Collection<Card>> {
    FULL_HAND(5) {
        public boolean apply(Collection<Card> cards) {
            ...
        }
    },
    FLUSH(5) {
        ...
    },
    ...
}

I wanted to copy the body of isFullHand to my Value.FULL_HAND#apply, but couldn't find it. There would be no such problem with the enum as it groups the name, the predicate and the numerical value nicely together. In case an enum wouldn't work, you could still create multiple classes doing the same.

Pokerhand

Again, HAND_RANK should be HandRank.

this.cards = cards;
Collections.sort(this.cards); 

You should neither assign a collection without doing a defensive copy, nor should you sort a provided collection (Does every caller know you do? What if I call new PokerHand(HandRank.HIGH_CARD, ImmutableList.of(....)?). It may be sometimes acceptable for performance reasons.

Questions

1. Any suggestions how to do it better?

Should I write some between class for easier testing.

import static ...;

public void testStraight() {
    List<Card> cards = ImmutableList.of(
        ACE.of(DIAMONDS),
        ACE.of(SPADES),
        ...
}

You can imagine how much lines it would take to properly test everything.

My above idea saves no lines, just makes them much shorter. You surely need quite a few such tests, but could also write some loops and maybe even some pseudo-random tests?

I see that even something like

final ImmutableList<Card> cards = ImmutableList.of(
    TEN.of(SPADES),
    TEN.of(DIAMONDS),
    NINE.of(SPADES),
    EIGHT.of(SPADES),
    EIGHT.of(CLUBS));
final Hand hand = new Hand(cards);

is a big pain to write and way beyond reasonable. You'd need a constructor like

final Hand hand = new Hand("TS TD 9S 8S 8C");

2. How could I make HandRanker class structure better?

To me it seems I'm exploiting stuff that I shouldn't do. Is this a good approach?

The condition should loop over HandRanker.values(). You definitely should not do the strange trick with assigning pokerHand in the test. That's so terrible I completely missed it.

Do not return boolean if you want to return pokerHand. Return null if it doesn't apply....

Something like

for (HandRank handRank : HandRank.values() {
     if (handRank.apply(allCards)) return handRank;
}
return null;

should also work. I know too little about poker, but there's some redundancy in your code: HAND_RANK.STRAIGHT and HandRanker.STRAIGHT, which is which?

Is there something I haven't thought of?

Concurrency?

Anyway, this part is ugly and should be rewritten.

Also relying on sorting when getting higher card at index 0 in a list seems wrong.

It'd fine if it was your own list. If you sort something private, then you can expect it to stay sorted. But you've sorted a list provided from the outside and someone may have shuffled it in the meantime (at least in theory).

I'd write something like

Card getHighest(List<Card> cards) {
     assert isSorted(card);
     return cards.get(0);
}