Determining winner and winning hand in poker (holdem)

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 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> STRAIGHTS = setPossibleStraights();
private static final HandRanker instance = new HandRanker();

public static HandRanker getInstance() {
return instance;
}

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

private HandRanker() {}

private static List> setPossibleStraights() {
List> 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.AC

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 ...

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> {
    FULL_HAND(5) {
        public boolean apply(Collection 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 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 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");

1. 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 cards) {
     assert isSorted(card);
     return cards.get(0);
}

StackExchange Code Review Q#87908, answer score: 11