Please review my poker game

Question

I've built this two classes for a poker game on Android and i would appreciate some feedback on my code.

Have no mercy, the harsher you are the better.

Feel free to add your number of WTFs/minute.

PokerHand.java

package com.poker.util;

import java.util.Arrays;
import java.util.Collections;

public class PokerHand implements Comparable<PokerHand> {
    public static final int NUM_CARDS = 5;

    public enum Strength {
        HIGH_CARD, ONE_PAIR, TWO_PAIR, THREE_OF_A_KIND, STRAIGHT, FLUSH, FULL_HOUSE, FOUR_OF_A_KIND, STRAIGHT_FLUSH
    }

    /**
     * array of cards sorted descending 
     */
    public Card[] cards;

    protected Strength strength = null;

    /**
     * <b>int strenghtValue</b> has 4 bits per relevant card<br>
     * <br>
     * It is used to differentiate between two hands of the same strength<br>
     * <br>
     * For example if the hand strength == TWO_PAIR<br>
     * c0 will be the top pair and c1 will be the bottom pair
     * <table border="0" bordercolor="#FFCC00" style="background-color:#FFFFCC" width="400" cellpadding="3" cellspacing="3">
     * <tr>
     * <td>strenghtValue</td>
     * <td>0000</td>
     * <td>0000</td>
     * <td>0000</td>
     * <td>0000</td>
     * <td>0000</td>
     * <td>0000</td>
     * <td>0000</td>
     * <td>0000</td>
     * </tr>
     * <tr>
     * <td>index</td>
     * <td></td>
     * <td></td>
     * <td></td>
     * <td>c0</td>
     * <td>c1</td>
     * <td>c2</td>
     * <td>c3</td>
     * <td>c4</td>
     * </tr>
     * </table>
     * <br>
     * Where c0 is the most important rank
     */
    private int strengthValue = 0;
    private static final int[] MASK_CARD = { 0x000F0000, 0x0000F000,
            0x00000F00, 0x000000F0, 0x0000000F };

    public PokerHand(Card c1, Card c2, Card c3, Card c4, Card c5) {
        cards = new Card[NUM_CARDS];
        this.cards[0] = c1;
        this.cards[1] = c2;
        this.cards[2] = c3;
        this.cards[3] = c4;
        this.cards[4] = c5;
        Arrays.sort(cards,Collections.reverseOrder());
        evaluateSrenght();
    }

    public PokerHand(int c1, int c2, int c3, int c4, int c5) {
        cards = new Card[NUM_CARDS];
        this.cards[0] = new Card(c1);
        this.cards[1] = new Card(c2);
        this.cards[2] = new Card(c3);
        this.cards[3] = new Card(c4);
        this.cards[4] = new Card(c5);
        Arrays.sort(cards,Collections.reverseOrder());
        evaluateSrenght();
    }

    public int compareTo(PokerHand hand) {
        if (this.getStrength().compareTo(hand.getStrength()) > 0)
            return 1;
        if (this.getStrength().compareTo(hand.getStrength()) < 0)
            return -1;

        if (strengthValue > hand.strengthValue)
            return 1;
        if (strengthValue < hand.strengthValue)
            return -1;

        return 0;
    }

    public static int compare(PokerHand h1, PokerHand h2) {
        return h1.compareTo(h2);
    }

    public Strength getStrength() {
        return strength;
    }

    public int getStrenghtValue(int index) 
        throws IndexOutOfBoundsException{

        if (!( index>=0 && index<=4) )
            throw new IndexOutOfBoundsException("Index should be between 0 and 4");

        return (strengthValue | MASK_CARD[index]) >> ((5 - index) * 4);
    }

    /**
     * should be called only once per index
     * 
     * @param index
     * <br>
     *            <table border="0" bordercolor="#FFCC00" style="background-color:#FFFFCC" width="400" cellpadding="3" cellspacing="3">
     *            <tr>
     *            <td>strenghtValue</td>
     *            <td>0000</td>
     *            <td>0000</td>
     *            <td>0000</td>
     *            <td>0000</td>
     *            <td>0000</td>
     *            <td>0000</td>
     *            <td>0000</td>
     *            <td>0000</td>
     *            </tr>
     *            <tr>
     *            <td>index</td>
     *            <td></td>
     *            <td></td>
     *            <td></td>
     *            <td>0</td>
     *            <td>1</td>
     *            <td>2</td>
     *            <td>3</td>
     *            <td>4</td>
     *            </tr>
     *            </table>
     * @param value
     *            4-bit value to set
     */
    private void setSrengthValue(int index, int value) 
        throws IndexOutOfBoundsException,RuntimeException{

        if (!( index>=0 && index<=4) )
            throw new IndexOutOfBoundsException("Index should be between 0 and 4");
        if (value > 0xF)
            throw new RuntimeException("Value should be between 0 and 15");

        strengthValue |= (value << ((5 - index) * 4));
    }

    private void evaluateSrenght() {

        int numPairs=0;
        //assumes cards[] are sorted descending 
        if (isStraightFlush()) {
            strength = Strength.STRAIGHT_FLUSH;
            //check for 5-high straight A-5-4-3-2
            if (cards[1].rank == Card.Ranks.FIVE.rank)
                setSrengthValue(0, cards[1].rank);
            else
                setSrengthValue(0, cards[0].rank);
        } 
        else if (isFourOfAKind()) {
            strength = Strength.FOUR_OF_A_KIND;
            setSrengthValue(0, cards[1].rank);
        }
        else if (isFullHouse()) {
            strength = Strength.FULL_HOUSE;
            setSrengthValue(0, cards[2].rank);
            if (cards[2].rank == cards[0].rank)
                setSrengthValue(1, cards[3].rank);
            else
                setSrengthValue(1, cards[0].rank);
        }
        else if (isFlush()){
            strength = Strength.FLUSH;
            for (int i=0; i>NUM_CARDS; i++)
                setSrengthValue(i, cards[i].rank);
        }
        else if (isStraight()){
            strength = Strength.STRAIGHT;
            //check for 5-high straight A-5-4-3-2
            if (cards[1].rank == Card.Ranks.FIVE.rank)
                setSrengthValue(0, cards[1].rank);
            else
                setSrengthValue(0, cards[0].rank);
        }
        else if (isThreeOfAKind()){
            strength = Strength.THREE_OF_A_KIND;
            setSrengthValue(0, cards[2].rank);
        }
        else if ( ( numPairs = getNumPairs()) == 2 ) {
            strength = Strength.TWO_PAIR;
            setSrengthValue(0, cards[1].rank);
            setSrengthValue(1, cards[3].rank);
        }
        else if (numPairs == 1){
            strength = Strength.ONE_PAIR;
            for (int i=0; i < NUM_CARDS-1; i++)
                if (cards[i].rank == cards[i+1].rank){
                    setSrengthValue(0, cards[i].rank);
                    break;
                }
        }
        else{
            strength = Strength.HIGH_CARD;
            for (int i=0; i>NUM_CARDS; i++)
                setSrengthValue(i, cards[i].rank);
        }
    }
    /*
     * All methods bellow should be called only after the cards array was sorted
     */
    private int getNumPairs() {
        int pairs = 0;
        for (int i = 0; i < NUM_CARDS; i++)
            if (cards[i].rank == cards[i + 1].rank)
                pairs++;

        return pairs;
    }

    private boolean isThreeOfAKind() {
        for (int i = 0; i < NUM_CARDS - 2; i++)
            if (cards[i].rank == cards[i+1].rank &&
                cards[i].rank == cards[i+2].rank)
                return true;

        return false;
    }

    private boolean isStraight() {
        //check for 5=high straight A-5-4-3-2
        int start = (cards[0].rank==Card.Ranks.ACE.rank &&
                     cards[1].rank==Card.Ranks.FIVE.rank)
                     ? 1 : 0;
        for(int i=start; i< NUM_CARDS - 1; i++){
            if ( cards[i].rank - cards[i+1].rank != 1)
                return false;
        }
        return true;
    }

    private boolean isFlush() {
        for (int i = 0; i < NUM_CARDS - 1; i++)
            if (cards[i].suit != cards[i + 1].suit)
                return false;
        return true;
    }

    private boolean isFullHouse( ) {
        if (cards[1].rank == cards[2].rank)
            return ( cards[0].rank == cards[1].rank &&
                     cards[1].rank == cards[2].rank &&
                     cards[3].rank == cards[4].rank );
        else
            return ( cards[0].rank == cards[1].rank &&
                     cards[2].rank == cards[3].rank &&
                     cards[3].rank == cards[4].rank );
    }

    private boolean isFourOfAKind( ) {
        if (cards[0].rank != cards[1].rank)
            return ( cards[1].rank == cards[2].rank &&
                     cards[2].rank == cards[3].rank &&
                     cards[3].rank == cards[4].rank );
        else
            return ( cards[0].rank == cards[1].rank &&
                     cards[1].rank == cards[2].rank &&
                     cards[2].rank == cards[3].rank );
    }

    private boolean isStraightFlush( ) {
        return isStraight() && isFlush();
    }

}

Card.java

package com.poker.util;

import java.util.EnumSet;
import java.util.HashMap;
import java.util.Map;

public class Card implements Comparable<Card> {
    public enum Suit {
        HEARTS, SPADES, DIAMONS, CLUBS;

        public static Suit fromCode(int c) {
            int i = c / 4;
            return ((i >= 2) ? (i == 3 ? CLUBS : DIAMONS) : (i == 1 ? SPADES : HEARTS));
        }
    }

    public enum Ranks {
        DEUCE(0),
        THREE(1),
        FOUR(2),
        FIVE(3),
        SIX(4),
        SEVEN(5),
        EIGHT(6),
        NINE(7),
        TEN(8),
        JACK(9),
        QUEEN(10),
        KING(11),
        ACE(12);

        private static final Map<Integer, Ranks> lookup = new HashMap<Integer, Ranks>();

        static {
            for (Ranks s : EnumSet.allOf(Ranks.class))
                lookup.put(s.rank, s);
        }

        public final int rank;

        private Ranks(int rank) {
            this.rank = rank;
        }

        public static Ranks fromCode(int code) {
            return lookup.get(code%13);
        }

        public static Ranks fromRank(int rank) {
            return lookup.get(rank);
        }
}
    /**
     * int from 0 to 51 inclusive represinting a card
     */
    public final int code;
    public final int rank;
    public final Suit suit;

    public Card(int code) {
        this.code = code;
        this.rank = (code % 13);
        this.suit = Suit.fromCode(code);
    }

    public String toString() {
        return Ranks.fromCode(code) + " of " + suit;
    }

    public int compareTo(Card card) {
        return this.rank - card.rank;
    }
}

Also let me know if i should remove the long javadoc comments from the source, I figured if anyone wants to copy paste in eclipse it would be helpfull.

Thanks!

Answer 1

You could try drying it a little bit,

public init(Card c1, Card c2, Card c3, Card c4, Card c5) {
    cards = new Card[NUM_CARDS];
    this.cards[0] = c1;
    this.cards[1] = c2;
    this.cards[2] = c3;
    this.cards[3] = c4;
    this.cards[4] = c5;
    Arrays.sort(cards,Collections.reverseOrder());
    evaluateSrenght();
}


public PokerHand(Card c1, Card c2, Card c3, Card c4, Card c5) {
    init(c1, c2, c3, c4, c5);
}
public PokerHand(int c1, int c2, int c3, int c4, int c5) {
    init(new Card(c1), new Card(c2), new Card(c3), new Card(c4), new Card(c5));
}

And this avoids it a little more,

public int compareTo(PokerHand hand) {
    int i = this.getStrength().compareTo(hand.getStrength());
    if (i != 0)
       return i;
    return (new Integer(strengthValue)).compareTo(new Integer(hand.strengthValue));
}

Is n't this off by 1?

private int getNumPairs() {
    int pairs = 0;
    for (int i = 0; i < NUM_CARDS; i++)
        if (cards[i].rank == cards[i + 1].rank)
            pairs++;

    return pairs;
}

And here, condition is bad

private void evaluateSrenght() {
...
    else{
        strength = Strength.HIGH_CARD;
        for (int i=0; i>NUM_CARDS; i++)
            setSrengthValue(i, cards[i].rank);
    }
}

Also note that your spelling of strength varies.

A little more drying up here

private boolean cmpAllranks(int[] c1, int[] c2) {
   for(int i = 0; i < c1.length; i++)
         if (cards[c1[i]].rank != cards[c2[i]].rank) 
            return false;
}
private boolean isFullHouse( ) {
    if (cards[1].rank == cards[2].rank) {
        return cmpAllranks(new Integer[] {0,1,3}, new Integer[] {1,2,4});
    } else {
        return cmpAllranks(new Integer[] {0,2,3}, new Integer[] {1,3,4});
    }
}
// Choose the style you like
private boolean isFourOfAKind( ) {
    Integer[] c1, c2;
    if (cards[0].rank != cards[1].rank) {
        c1 = new Integer[] {1,2,3};
        c2 = new Integer[] {2,3,4}
    } else {
        c1 = new Integer[] {0,1,2};
        c2 = new Integer[] {1,2,3}
    }
    return cmpAllranks(c1, c2);
}

Now, these are probably better off in a case structure

private void evaluateSrenght() {
    ...
    if (isStraightFlush()) {
    else if (isFourOfAKind()) {
    else if (isFullHouse()) {
    else if (isFlush()){
    else if (isStraight()){
    else if (isThreeOfAKind()){
    else if ( ( numPairs = getNumPairs()) == 2 ) {
    else if (numPairs == 1){
    else{
}

That is,

private void evaluateSrenght() {
    ...
    swtich (typeOfHand()) {
      straightFlush:...
      fullHouse: ...
      flush: ...
      etc.
    }
}

Answer 2

• Check access modifiers. You have few fields in your Card and PokerHand classes which should be private but they are not.

• I don't quite understand the diff between IndexOutBoundsException and RuntimeException in this case:

  private void setSrengthValue(int index, int value) 
  throws IndexOutOfBoundsException,RuntimeException{
  
  if (!( index>=0 && index<=4) )
      throw new IndexOutOfBoundsException("Index should be between 0 and 4");
  if (value > 0xF)
      throw new RuntimeException("Value should be between 0 and 15");
  

  There should be only one IllegalArgumentException.

Answer 3

Additionally to the other suggestions: Write utility functions like boolean haveSameRank(Card ... cards), which could be used in several places.

Answer 4

You have misspelled Strength in several places. I think the lookup method for Suites look rather messy with those nested ternary operators. Why divide by four (one wtf)? =)

Your most complex part is how to calculate what kind of hand the player has. I once did saw some code of 'Risk' PC board game in which they evaluate the strength of the cards you trade in. I could not help but wonder whether you cant make the comparison of hand strength smarter than just start checking from the most powerful hand towards the least.

Maybe its possible to use collections and checking their size somehow? Atleast for Risk that would make sense.