Hand rankings for Poker game

Question

I have tried to write a small part of a Poker game to check the Poker hand rankings. Please review and provide your feedback.

package com.sa.test.conditions.poker;

import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;

class Hand {
    Card[] cards;

    public Card[] getCards() {
        return cards;
    }

    public void setCards(Card[] cards) {
        this.cards = cards;
        Arrays.sort(cards);
    }

    public void printHand() {
        for (Card card : cards) {
            System.out.print(card.suite + " => ");
            System.out.println(card.rank);
        }
    }

    public HandType getType() {

        if (isAllFromSameSuite()) {

            if (isARoyalFlush()) {
                return HandType.ROYAL_FLUSH;
            }
            if (cardsInSequence()) {
                return HandType.STRAIGHT_FLUSH;
            }
            if (!cardsInSequence()) {
                return HandType.FLUSH;
            }
        }

        if (hasNumberOfCardsEqualToN(4)) {
            return HandType.FOUR_OF_A_KIND;
        }

        if (hasNumberOfCardsEqualToN(3) && hasNumberOfCardsEqualToN(2)) {
            return HandType.FULL_HOUSE;
        }

        if (!isAllFromSameSuite() && cardsInSequence()) {
            return HandType.STRAIGHT;
        }

        if (hasNumberOfCardsEqualToN(3)) {
            return HandType.THREE_OF_A_KIND;
        }

        if (hasTotalPairsEqualToN(2)) {
            return HandType.TWO_PAIR;
        }

        if (hasTotalPairsEqualToN(1)) {
            return HandType.PAIR;
        }

        return HandType.HIGH_CARD;
    }

    private boolean isARoyalFlush() {
        int royal = 10;
        for (Card card : cards) {

            if (card.getRank() < royal && card.getRank() != 1) {
                return false;
            }
        }
        // System.out.println("ALl isRoyalFlush");
        return true;
    }

    // This method will check the distance between the cards.. whole purpose is
    // to check of cards are in sequence either forward or backward.
    private boolean cardsInSequence() {
        int maxDiff = 1;
        Card tmpCard = null;
        for (Card card : cards) {
            if (tmpCard != null) {
                // check the difference based on the value of rank
                int diff = Math.abs(tmpCard.getRank() - card.getRank());
                if (diff > maxDiff) {
                    return false;
                }
            }
            tmpCard = card;
        }
        return true;
    }

    private boolean isAllFromSameSuite() {
        Card tmpCard = null;
        for (Card card : cards) {
            if (tmpCard != null && !tmpCard.getSuite().equals(card.getSuite())) {
                return false;
            }
            tmpCard = card;
        }
        return true;
    }

    private boolean hasNumberOfCardsEqualToN(int number) {
        Map<Integer, Integer> ranksMap = getCardRankCountMap();
        for (Map.Entry<Integer, Integer> entry : ranksMap.entrySet()) {

            if (entry.getValue() == number) {
                return true;
            }

        }
        return false;
    }

    private boolean hasTotalPairsEqualToN(int numberOfPair) {
        Card tmpCard = null;
        int pairCount = 0;
        for (Card card : cards) {
            // increment the count if any sibling card matches based on rank
            // which are making a pair.
            if (tmpCard != null && (tmpCard.getRank() == card.getRank())) {
                pairCount = pairCount + 1;
            }
            tmpCard = card;

        }
        // check if count matches the n
        if (pairCount == numberOfPair)
            return true;

        return false;
    }

    private Map<Integer, Integer> getCardRankCountMap() {
        Map<Integer, Integer> ranksMap = new HashMap<Integer, Integer>();
        for (Card card : cards) {
            if (ranksMap.containsKey(card.getRank())) {
                int rank = card.getRank();
                int count = ranksMap.get(rank);
                ranksMap.put(rank, count + 1);
            } else {
                ranksMap.put(card.getRank(), 1);
            }

        }
        return ranksMap;
    }
}

Answer 1

If this should be expandable in the future, you should inject a CardPrinter interface to the constructor, which takes a Cards[] for output of the cards. This interface could hold a CardFormatter interface which formats the apearance and returns a String. Then the CardPrinter will "print" the cards. By using this approach you can, if you decide to print e.g to Html, easily extend your application.

public interface OutputFormatter {
   public String format(Card[] cards);
}  

public interface CardPrinter {
    public void setFormatter(OutputFormatter formatter);
    public void pring(Card[] cards);
}

---

• delete commented code. Commented code is dead code, which only removes readability.

• you should use braces {} for single if statements to make your code less errorprone. No matter what you decide to use (braces or not) you should stick to it. Right now you are mixing the usage.

• if you return true if a condition is true and otherwise false, you can simplify this by returning the condition.

  if (pairCount == numberOfPair)
      return true;
  
  return false;  
  

  will look like

  return pairCount == numberOfPair;
  

• if you need to check the current Card and the next Card it is more readable to use a for loop which ends at length -1. Like

  private boolean hasTotalPairsEqualToN(int numberOfPair) {
      int pairCount = 0;
      for (int i = 0; i < cards.length - 1; i++) {
  
          if (cards[i].getRank() == cards[i + 1].getRank()) {
              pairCount++;
          }
  
      }
      return pairCount == numberOfPair;
  }  
  

• comments like

  // check if count matches the n
  if (pairCount == numberOfPair)  
  

  won't add any value to your code. Comments should describe why something is done. Let the code speak for itself about what is done.

Answer 2

The Hand class is mutable. A general rule of thumb is to make classes immutable for the sake of ease of use. This can be especially apparent when using such objects in Collections as keys. Shifting the assignment of the Card[] cards; field to a constructor gets you half of the way there. The other problem is that you're passing in a reference to the array, so there's no guarantee that it may not be modified after the fact. The best practice in such cases is to make a defensive copy.

The code seems to implicitly assume a hand of size 5. This should really be explicit given that the logic would not hold given hands of different sizes. This would be especially important given certain styles of Poker where you might construct the best 5 card hand from 7 cards.

The private methods could be better named. isARoyalFlush() doesn't actually check for a royal flush, just that all the cards are rank 10 or above, or 1 (e.g one of 10 J Q K A). It's only in combination with isAllFromSameSuite() that this check is valid. isAllFromSameSuite() could be more succinctly named isFlush() (plus I suspect 'suite' is a typo and you mean suit) while I'd rename isARoyalFlush() to isAllRoyal(), hasNumberOfCardsEqualToN() would read more fluently as just hasNumberOfCardsEqualTo().

Some of your if statement checks are redundant:

if (cardsInSequence()) {
    return HandType.STRAIGHT_FLUSH;
}
if (!cardsInSequence()) {
    return HandType.FLUSH;
}

Checking cardsInSqeuence() twice is unnecessary. This can be simplified to:

if (cardsInSequence()) {
    return HandType.STRAIGHT_FLUSH;
} else {
    return HandType.FLUSH;
}

cardsInSequence() would be clearer with an explicitly indexed for loop, although this presumes hand have a minimum of two cards:

private boolean cardsInSequence() {
    for (int i = 1; i < cards.length; i++) {
        if (cards[i].getRank() != cards[i - 1].getRank() + 1) {
            return false;
        }
    }

    return true;
}

isAllFromSameSuite() also doesn't need a tmpCard. As with the above method, you can just compare all the other cards to the first:

private boolean isAllFromSameSuite() {
    for (int i = 1; i < cards.length; i++) {
        if (cards[i].getSuite() != cards[0].getSuite()) {
            return false;
        }
    }

    return true;
}

hasTotalPairsEqualToN(...) could use the ranksMap construct instead, similar to hasNumberOfCardsEqualToN(...). It'd also be a more flexible function to just return the total number of pairs rather than building in the comparison:

if (numberOfPairs() == 2) {
    return HandType.TWO_PAIR;
}

//...

private int numberOfPairs() {
    Map<Integer, Integer> ranksMap = getCardRankCountMap();
    int numberOfPairs = 0;
    for (Map.Entry<Integer, Integer> entry : ranksMap.entrySet()) {
        if (entry.getValue() == 2) {
            numberOfPairs++;
        }

    }
    return numberOfPairs;
}

ranksMap is actually a rather useful construct and doesn't need to be generated each and every time it is used. You might consider either doing so when cards is assigned (in the constructor if you've made the object immutable). Alternately, you might want to lazy-load it, ensuring we only do that work when we need to:

private Map<Integer, Integer> ranksMap;

private Map<Integer, Integer> getCardRankCountMap() {
    if (ranksMap == null) {
        ranksMap = new HashMap<Integer, Integer>();
        for (Card card : cards) {
            if (ranksMap.containsKey(card.getRank())) {
                int rank = card.getRank();
                int count = ranksMap.get(rank);
                ranksMap.put(rank, count + 1);
            } else {
                ranksMap.put(card.getRank(), 1);
            }

        }
    }

    return ranksMap;
}