nosuchblogger

Now my tests are somewhat easier to write, time to crack on with the next hand identification. Shouldn't be too tricky as it's three-of-a-kind, which is decidedly similar to two-of-a-kind. I start out with the tests:

• three-of-a-kind beats high card
• three-of-a-kind beats pair
• three-of-a-kind beats two-pair
• highest three-of-a-kind wins

Onto the implementation

I decide to copy from TwoPairIdentifier as I know this class is going to be very similar. When writing the TwoPair implementation, I also started by copying the previous implementation. This rings a few alarm bells: doing the same thing twice is a coincidence; a third time there's a pattern.

It doesn't take too much tweaking to make this look right: the nested class is looking for two of the same face value, this changes to three. The remaining change is to just pick the best three-of-a-kind rather than the best two.

Running the tests and we're all back to green again.

The end of the story?

I could leave it here, but as alluded to above, and noted in the issues file, there is duplication - now across three classes - so time to refactor.

Taking the easy one first: Pair, TwoPair and ThreeOfAKind all have nested classes to identify a number of cards (2 or 3 as required) with the same face value. TwoPair and ThreeOfAKind have an almost identical implementation, except in the number of cards to match. To make things easy, I first change Pair so the implementation matches TwoPair.

Now I can simply extract the nested class out, create a constructor which takes the number of same-valued cards, then remove the duplication of the three Identifier classes.

Decisions, decisions

Next is the duplication within the accept methods. The refactor here is harder (for me anyway!) to see. One option is to perhaps introduce some sort of base class which does some of the work, delegating to overridden methods to handle the differences between the implementations: i.e. the Template pattern.

A number of years ago I would have probably gone down this road, but now I tend to avoid unnecessary inheritance if possible: I'm all about the favouring of composition over inheritance. So I just decide to start small and put the first two lines, which are virtually the same, into a new class:

public class Hands {
  public static List<Map.Entry<Integer, Collection<Card>>> existSameValuedCards(Iterable<Card> cards, int numberOfSameValuedCards) {
    Map<Integer, Collection<Card>> groupedByValue = Iterables.groupBy(cards, new CardNumericValue());
    return Iterables.where(groupedByValue.entrySet(), new ContainsSameValuedCards(numberOfSameValuedCards));
  }
}

There's just duplication remaining in the TwoPair and ThreeOfAKind classes, which orders the possible pairs/three-of-a-kinds by value to enable the selection of the best. This is already identical code in the two classes, so I think the simplest option here is to just move this sorting into the new method. The last line from above is therefore replaced with the following two lines:

Collection<Map.Entry<Integer, Collection<Card>>> sameValuedCards = Iterables.where(groupedByValue.entrySet(), new ContainsSameValuedCards(numberOfSameValuedCards));
return Iterables.sort(sameValuedCards, new Comparator<Map.Entry<Integer, Collection<Card>>>() {
  @Override
  public int compare(Entry<Integer, Collection<Card>> o1, Entry<Integer, Collection<Card>> o2) {
    return o2.getKey().compareTo(o1.getKey());
  }});

That's much better and I'm happy with the resulting Identifier classes. But will that last? We'll see. Next up: Straight.