Weekend Challenge - Poker Hand Evaluation

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 fo

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 that simply chooses the best hand as hands are added... and then has a best() method. Your code becomes:

Best results = new Best();
....
return results.best();

The PokerHandResult instances have a 'natural order' (implement Comparable), so the generic class Best 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 results) on this class. Should be replaced with Best 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.... `che

StackExchange Code Review Q#36916, answer score: 25