An easy two-pairs
Following on from the refactoring session, I head into the final straight: well that was the hope. The main piece outstanding is set of hand identification classes. So after the High card and Pair, I pick the next one: Two-Pair.
Copy-Paste
I tend not to like copy and paste development; it just feels all wrong. But it is one way forward, and if I take this approach, I always try and go back and eliminate the duplication. With a TDD approach, this occurs normally, as part of the refactor step.
Anyhow, enough of the excuses.
Two-Pair is obviously going to be somewhat similar to identifying a single pair, so I copy and paste the PairIdentifier class and call it TwoPairIdentifier. I've already added a failing test in the RoundWinnersTest class to tell me when I'm done.
Three-Two-Pairs
The main logical difference between the two at this stage is that Pair matches one and only one pair, whereas TwoPair should match two or more. For example, the two hole cards and five community cards could give us three two-pairs, from which TwoPair should give us the best two-pairs and the highest kicker (whether or not that happens to come from the other pair is irrelevant).
The problem with the current Pair implementation that's been copied is in the where line:
Iterables.where(groupedByValue.values(), new ContainsAtLeastOnePair())
This returns just a collection of collection of cards -for example, assume we had a pair of 2s, 3s and 4s, this line returns us: { { 2h, 2c }, { 3h, 3c }, { 4h, 4c} }. Now finding the best two-pairs is kind-of a pain (we again have to extract the card's face value). What would be better is if we retained the card's face value as part of this Iterables.where line - after all, groupedByValue does have the face value as the Map's key.
So ContainsAtLeastOnePair is modified to take a Map.Entry<Integer, Collection<Card>> instead of just a Collection<Card>. Therefore, the return value is now Collection<Map.Entry<Integer, Collection<Card>>> from which I can simply sort the collection by the key of the Map.Entry and pick the first two elements from it.
That gives me the best two pairs.
The kicker simply remains the best of the remaining cards.
Best of the rest
Running the test it works! The two pair is identified correctly.
Now to look at the duplication between Pair and TwoPair. The obvious one is the nested class IsNotIn. This class supports the identification of the kicker(s) from the cards not making up the pair or two-pair.
Clearly this is a common behaviour, and it seems silly in retrospect to make the HandIdentifier responsible for determining the kickers. After all, it's simply a matter of knowing which cards are ranked (i.e. which cards make up a pair, two-pair, three-of-a-kind, etc.) and all the cards of a hand, then the kickers are simply the highest cards from those non-ranked cards until there are five cards in the final ranked hand.
RankedHand is therefore modified to handle the kickers which removes the IsNotIn duplication and simplifies the HandIdentifier implementations.
Not so simple duplication
The one remaining nested class is ComainsAtLeastOnePair. As mentioned above, for the TwoPair case, I modified it slightly. I could probably refactor Pair to use a similar implementation, then extract the nested class.
Additionally, the implementations of the accept method do share similarities, but are also somewhat different. I consider how this could be refactored.
In the end, I decide not to do any further refactoring at this point, but I'm not entirely happy, so make a note in the issues log. As I look at three-of-a-kind, four-of-a-kind, and so on, I feel I may be looking back at these classes in any case.
The perils of copy-paste
As I'm reviewing the Pair/Two-Pair code, I notice something a little odd. When a two-pair hand is identified, a RankedHand instance is returned. This contains the rank number (zero being the lowest ranked - high card; one being a pair; and so on). I notice both Pair and TwoPair have the rank of 1. This is clearly my copy and paste coming back to haunt me. As it turns out, it's just luck that the two-pair test I wrote happened to succeed. I write another test just to confirm my fears, and indeed it breaks - I make a hand with a pair beat a hand with two pairs!
The fix is simple enough - just update the construction of the RankedHand to pass a 2 instead of a 1. All good again.
Testing problems
It does of course highlight the problem that the tests aren't good enough. The one problem I'm finding is that the tests are somewhat painful to write. There's a lot of duplication - which I don't necessarily consider a major problem within test code - but in this case, it is causing me to write fewer tests. And as there's an individual test for each case, naming is a problem as well.
The next bit of work is therefore to look into a way to improve the tests. One thing I have seen is parameterized tests - this may be an option. However, as this is already quite long, that is for next time.
