Project Euler #54 - Poker Streams

This challenge posted by Durron597 intrigued me, and inspired me to answer his question, and also to determine whether a more functional approach was available for poker hand ranking.

The problem description is:

The file, poker.txt, contains one-thousand random hands dealt to two players. Each line of the file contains ten cards (separated by a single space): the first five are Player 1's cards and the last five are Player 2's cards. You can assume that all hands are valid (no invalid characters or repeated cards), each player's hand is in no specific order, and in each hand there is a clear winner.

How many hands does Player 1 win?

I decided to use a cascading most-signigicant-bits type approach to rank hands against each other. In other words, calculate a unique score for each hand. The actual score does not matter, the only reason for the score is to be a relative ranking against another hand. The hand with the larger score wins. The difference between the scores is not important.

In order to accomplish this, I broke a Java long value in to 8 4-bit segments.

7777 6666 5555 4444 3333 2222 1111 0000
| | | | | | | --> Lowest ranked card
| | | | | | -------> Second Lowest card
| | | | | ------------> Third lowest card
| | | | -----------------> Second highest card
| | | ----------------------> Highest ranked card
| | ---------------------------> SMALLSAME - Rank of low pair, if any
| --------------------------------> LARGESAME - Rank of largest group, if any
-------------------------------------> NAMENIBBLE - Hand type value


There are 13 cards, which fit quite nicely in the 16 avaialble values in a nibble.

I ordered the cards as 2 through ace, with the values (in hex) of 2 through E

The hand classifications in the highest nibble are a bit more complicated. The overall classification uses a little trick of bit manipulation too, so I presen

The implementation; it is really slick. I find the code difficult to review, because you've deliberately set out to present a demonstration of a functional approach. Making the code more elegant may end up disguising the point....

That said, there are improvements available that should not interfere with the implementation

More Abstractions

public static void main(String[] args) throws IOException {
    final long[] times = new long[1000]; 
    final long[] results = new long[1000]; 
    final Path source = Paths.get(args.length == 0 ? "p054_poker.txt" : args[0]);
    for (int i = 0; i  t / 1000000.0).summaryStatistics());

}

In this method, you have at least three different ideas; your calculator -- the featured element of your demonstration, an instrumentation harness around it, and a data provider. I'd prefer and example that teases those ideas apart.

public static void main(String args[]) throws IOException {
    PokerScoringCalculator calculator = new PokerScoringCalculator();
    InstrumentationHarness testHarness = new InstrumentationHarness(calculator);
    TestDataFactory testDataFactory = new testDataFactory(args);

    testHarness.run(testDataFactory.getSource());
}

public static class InstrumentationHarness {
    private final PokerScoringCalculator target;

    ...

    public void run(Path source) {
        final int testIterationCount = 1000;

        final long[] times = new long[testIterationCount];
        final long[] results = new long[testIterationCount];

        for (int i = 0; i < times.length; i++) {
            long nano = System.nanoTime();
            results[i] = calculator.countPlayer1Wins(source);
            times[i] = System.nanoTime() - nano;
        }

        ...
    }
}

Further refactoring here might reveal a Clock abstraction, and a Reporter abstraction that is separate from the TestHarness itself....

Another example

public static long compareHands(String hand) {
    // Convert the String to separate cards
    List cards = Stream.of(hand.split(" ")).collect(
            Collectors.toList());

    long handA = scoreHand(cards.subList(0, 5));
    long handB = scoreHand(cards.subList(5, 10));

    return handA - handB;
}

Buried in here, you've got a parser, a comparator, and the scoring encoder.

// These constants are carefully selected to ensure that
// - STRAIGHT is > 3-of-a-kind
// - STRAIGHT and FLUSH are less than 4-of-a-kind and full-house.
// - STRAIGH + FLUSH (12) is better than others.
// groups representing :
// HIGH_CARD, 1_PAIR, 2_PAIR, 3_OF_A_KIND, FULL_HOUSE,  4_OF_A_KIND

Don't these comments just scream that there's an enumeration waiting to be discovered?

private static final int[] GROUPSCORE = { 0, 1, 2, 3, 9, 10 };
private static final int[] GROUPS = { 
    groupHash(new int[]{ 1, 1, 1, 1, 1 }),
    groupHash(new int[]{ 1, 1, 1, 2 }),
    groupHash(new int[]{ 1, 2, 2 }),
    groupHash(new int[]{ 1, 1, 3 }),
    groupHash(new int[]{ 2, 3 }),
    groupHash(new int[]{ 1, 4 }) };

These arrays do not communicate that score:9 is bound to group[2,3]. It's not even obvious that they should be the same size! There really ought to be a builder here, which takes score pattern pairs as inputs, and gives you the arrays you need at the end. You could be plain

builder.add(new int[] {2,3}, 9);

Reversing the order of the arguments gives you an opportunity to get cute:

builder.add(9, 2, 3);

But I don't approve of that approach -- it makes it look like these are all the same thing. It looks a little bit better with the enumeration.

builder.add(FULL_HOUSE, 2, 3);

I think it's a bit weird that the patterns are described in ascending order. In natural language, the three-of-a-kind comes first; so I would rather see the logic written that way

builder.add(FULL_HOUSE, 3, 2);

If you really wanted to make things readable, you might go with a fluent interface here

builder.forPattern(3,2).scoreAs(FULL_HOUSE);

Readability again:

switch (c) {
        case '2' : return 0;
        case '3' : return 1;
        case '4' : return 2;
        case '5' : return 3;
        case '6' : return 4;
        case '7' : return 5;
        case '8' : return 6;
        case '9' : return 7;
        case 'T' : return 8;
        case 'J' : return 9;
        case 'Q' : return 10;
        case 'K' : return 11;
        case 'A' : return 12;
    }

Why not

"23456789TJQKA".indexOf(c)

Although, as before, there's an argument that the more natural representation is "AKQJ...", which gets reversed.

Magic Numbers

In spades.

Choosing two of the easy ones:

if (hand.size() != 5) {
        throw new IllegalArgumentException("Illegal hand " + hand);
    }
    ...
    long score = IntStream.range(0, 5)
            .mapToLong(i -> shiftCard(0, holding[i], i)).sum();
    ...
    long handA = scoreHand(cards.subList(0, 5));
    long handB = scoreHand(cards.subList(5, 5+5));

Those ar

StackExchange Code Review Q#81874, answer score: 2