Card class for a poker card

blueraincap

blueraincap

any opinions on how to simplify or improve a basic Card class, which will be instantiated for use in a Deck class.

Code: 
/**
* PokerCard class represents a playing card of 52 values differing by suits and numeric ranks (1-13).
* Use int[2] to store the values of each card, with [0] for its rank and [1] for its suit.
* Suits consist of spades, hearts, clubs, diamonds coded by 0, 1, 2, 3, respectively.
* Values are from 1 to 13, with Ace, Jack, Queen, King coded by 1, 11, 12, 13, respectively.
*/

public class PokerCard implements Comparable<PokerCard>{
 
  @Override
  public boolean equals(PokerCard that){ //overriding equals for comparing cards
    if (this == that) //if this and obj are the same
      return true;
    else if (that == null)
      throw new NullPointerException("The card is null");
    else if (this.getClass() != that.getClass())
      return false;
    return ( (this.value[rank] == that.value[rank]) && (this.value[suit] == that.value[suit]) ); //when both cards have the same numeric rank and suit
  }
 
  @Override
  public int compareTo(PokerCard that){ //overriding Comparable for comparing cards
    if (this.equals(that))
      return 0;
    else if (this.value[rank] == Ace && that.value[rank] != Ace) //special case when this card is an Ace and that isn't
      return 1;
    else if (this.value[rank] != Ace && that.value[rank] == Ace) //special case when that card is an Ace and this isn't
      return -1;
    else if (this.value[rank] > that.value[rank]) //both non-Ace and this card's numeric value is higher
      return 1;
    else if (this.value[rank] < that.value[rank]) //both non-Ace and this card's numeric value is lower
      return -1;
    else if (this.value[suit] < that.value[suit]) //both cards have the same numeric values and this card's suit is of a higher suit
      return 1;
    else
      return -1; //both cards have the same numeric values and this card's suit is of a lower suit
  }
 
  protected final static int Spade = 0; //initializing constant values
  protected final static int Heart = 1;
  protected final static int Club = 2;
  protected final static int Diamond = 3;
  protected final static int Ace = 1;
  protected final static int Jack = 11;
  protected final static int Queen = 12;
  protected final static int King = 13;
  protected final static int rank = 0; //int[] array index
  protected final static int suit = 1;
 
  private final int[] value = new int[2]; //int[0] for storing numeric value, int[1] for suit; private to preventing tempering
 
  PokerCard(int thisrank, int thissuit){ //sole constructor taking in rank and suit as args
    if(thissuit>Diamond || thissuit<Spade) //invalid suit value
      throw new IllegalArgumentException("invalid suit input");
    if(thisrank>13 || thisrank<1)
      throw new IllegalArgumentException("card numeric rank inputted invalid, must be 1-13"); //invalid card value
    this.value[rank] = thisrank;
    this.value[suit] = thissuit;
  }
 
  public int[] getValue(){ //getter for value
    return value;
  }
 
  public String[] getValueAsString(){ //getting for value but returned as string[2]
    String suitString;
    String rankString;
    switch(value[suit]){
      case Spade: suitString = "Spade";
      break;
      case Heart: suitString = "Heart";
      break;
      case Club: suitString = "Club";
      break;
      default: suitString = "Diamond";
    }
    switch(value[rank]){
      case Ace: rankString = "Ace";
      break;
      case Jack: rankString = "Jack";
      break;
      case Queen: rankString = "Queen";
      break;
      case King: rankString = "King";
      break;
      default: rankString = String.valueOf(value[rank]); //all numeric ranks returned as numeric strings     
    }
    return new String[]{rankString, suitString};
  }
 
 
}
 
One thing *I* would do, is assign an ace the value of 100 for example, rather than 1.
In this way, a single simple value check informs if an "ace" is involved. IMO, this makes more complex analysis and branching easier.

I don't know what your intent for the code is, sooo other than that... go fish!!
 
Sure,

  • Use an enumeration for card faces. The index of the enumeration can be used as it's suit number which elements all of the `protected final static ...` numbers. This will also improve the way your switch structure works and it will be far more efficient (comparing strings vs integers). You can attach a `getName` function to the enumeration and leave that behavior to be managed by it. (See note below on `hashCode`). Generally prefer enumerations to static integers for code readability and performance. If you don't use an enumeration, a `static class Face` with these static integers defined on it is another option, though prefer enumerations since you are dealing with integers and you get nice things for free.
  • Your equals method should take an Object type which then gets casted into a `PokerCard`. You should insure the Object is indeed a `PokerCard` before casting and if it is not throw an exception. The current method will literally fail if you try to compare anything else to it. Equals is intended to be used as a comparison method for anything against anything.
  • Always override `hashCode` when you override `equals`. The reason is because maps and other collections depending on `hashCode` as a determinant for uniqueness. If you do not override `hashCode` but override `equals` the definition of `hashCode` becomes undefined and you violate the contract specified by `Object.hashCode`.
  • By overriding `hashCode` you can now store a `HashMap` of `Enum Face` to string and the look up goes from O(N) to O(1) on average. This hashMap can be a private member of your `Face` enumeration with a public facing `getFaceValueFromFace` function or something.
  • Prefer JavaDoc comments (/** ... */) as comments for methods. Your current method will be hard to read and is unsupported by IDE auto-complete.
  • Put spaces between operators (such as <)
  • A major problem with Java is it's lack of really well defined unsigned types. Java SE 8 introduced an API to do math on them but it's not great. You can change your input types to your constructor to `byte` to save memory since you will need exceed -128 or +127.
  • Further, if you want to make things more interesting you could attempt to make an annotation called `@unsigned` for arguments that will do your checks for you here, simplifying your constructor code marginally.
  • The naming `getValueAsString` is ambiguous. Prefer `faceValue` to improve your API (pokerCard.getValueAsString vs pokerCard.faceValue).
  • Instead of returning a `string[2]`look into return a `Pair` if you know you will only return two things.

Hope this helps.
 
tiddlywinks said:
One thing *I* would do, is assign an ace the value of 100 for example, rather than 1.
More...
That would make sense from an English language point of view: "if the value is lower than Ace...". However, currently Ace has the lowest value assigned. From a programming point of view is it then "if the value is higher than Ace ...". This is fundamentally the same thing to program.
 
blueraincap said:
Is that sarcastic?
More...
No, not really. When I looked at that method compareTo() was I of the opinion that it had way too many "if then" statements. And that there should be a more elegant way of describing this. But soon I discovered that that was not the case.
By the way: how many stacks of cards are being used in a poker game? Only one, or more than one? looking at the statement "if(this.equals(that)) return 0" I wondered whether this relates to the value only, or that the suit is also relevant.
 
gaussian said:
Use an enumeration for card faces. The index of the enumeration can be used as it's suit number which elements all of the `protected final static ...` numbers. This will also improve the way your switch structure works and it will be far more efficient (comparing strings vs integers). You can attach a `getName` function to the enumeration and leave that behavior to be managed by it. (See note below on `hashCode`). Generally prefer enumerations to static integers for code readability and performance. If you don't use an enumeration, a `static class Face` with these static integers defined on it is another option, though prefer enumerations since you are dealing with integers and you get nice things for free.
More...

I tried to re-write all the cards as enum, but compilation gave "illegal reference to static field from initializer" for this.value = valueDecoder[valueAsCode], and this.suit = suitDecoder[suitAsCode]. Can you pls explain why the constructor is having problems with static variables?
Code: 
/**
* Card class represents a playing card of 52 values differing by values and suits.
* Each card contains two variables, namely value and suit.
* A card's face is defined to be [2] with its value in [0] and suit in [1].
* Values are encoded in [2, 14] with Ace/King/Queen/Jack as 14/13/12/11.
* Suits are encoded in [1, 4] with Spade/Heart/Club/Diamond as 4/3/2/1.
* Each card's face can be returned as defined codes for computational purpose or as strings for printing purpose.
*/

public enum Card{
  AceSpade(14, 4), AceHeart(14, 3), AceClub(14, 2), AceDiamond(14, 1),
  KingSpade(13, 4), KingHeart(13, 3), KingClub(13, 2), KingDiamond(13, 1),
  QueenSpade(12, 4), QueenHeart(12, 3), QueenClub(12, 2), QueenDiamond(12, 1),
  JackSpade(11, 4), JackHeart(11, 3), JackClub(11, 2), JackDiamond(11, 1),
  TenSpade(10, 4), TenHeart(10, 3), TenClub(10, 2), TenDiamond(10, 1),
  NineSpade(9, 4), NineHeart(9, 3), NineClub(9, 2), NineDiamond(9, 1),
  EightSpade(8, 4), EightHeart(8, 3), EightClub(8, 2), EightDiamond(8, 1),
  SevenSpade(7, 4), SevenHeart(7, 3), SevenClub(7, 2), SevenDiamond(7, 1),
  SixSpade(6, 4), SixHeart(6, 3), SixClub(6, 2), SixDiamond(6, 1),
  FiveSpade(5, 4), FiveHeart(5, 3), FiveClub(5, 2), FiveDiamond(5, 1),
  FourSpade(4, 4), FourHeart(4, 3), FourClub(4, 2), FourDiamond(4, 1),
  ThreeSpade(3, 4), ThreeHeart(3, 3), ThreeClub(3, 2), ThreeDiamond(3, 1),
  TwoSpade(2, 4), TwoHeart(2, 3), TwoClub(2, 2), TwoDiamond(2, 1);
 
  private byte valueAsCode; //numeric value codes
  private byte suitAsCode; //spade, heart, club, diamond codes
  private String value; //numeric value
  private String suit; //spade, heart, club, diamond
 
  private static final String[] valueDecoder = new String[]{"*", "*", "2", "3", "4", "5", "6", "7", "8", "9", "10", "Jack", "Queen", "King", "Ace"}; //for converting values in codes to values in strings
  private static final String[] suitDecoder = new String[]{"*", "Diamond", "Club", "Heart", "Spade"}; //for converting suits in oodes to suits in strings
 
  private Card(int valueAsCode, int suitAsCode){
    this.valueAsCode = (byte)valueAsCode;
    this.suitAsCode = (byte)suitAsCode;
    this.value = valueDecoder[valueAsCode]; //Compilation error! illegal reference to static field from initializer
    this.suit = suitDecoder[suitAsCode]; //Compilation error! illegal reference to static field from initializer
  }
   
  public String[] getFace(){
    return new String[]{this.value, this.suit};
  }
 
  public byte[] getFaceAsCode(){
    return new byte[]{this.valueAsCode, this.suitAsCode};
  }
 
}
 
HobbyTrading said:
That would make sense from an English language point of view: "if the value is lower than Ace...". However, currently Ace has the lowest value assigned. From a programming point of view is it then "if the value is higher than Ace ...". This is fundamentally the same thing to program.
More...

No. It is an ease of usability and expedient logic point of view...

If an ace has multiple use values, then assigning a value (to the ace) HIGHER than the maximum sum of 5 non-ace cards (assuming playing 5 card draw) is immediately known.

For instance... a king in the original code is assigned a value of 13... therefore the maximum sum of 4 kings is 13 x 4 = 52, and a fifth card of a queen with an assigned value of 12 in the original post brings the maximum sum of 5 non-ace cards to 64.

Assigning a value to an ace of 100 (for example) rather than 1 is able to isolate ace-based "hands" for further multi use logic branching. ANY hand with a value < 100 needs no ace logic. This can easily apply to winning hand rank hierarchy as well. A hand >= 100 must go through multi-use/multi-value ace-logic. ANY hand summed less than 100 can not possibly be a royal, an ace high(or low) straight, a pair of aces, etc etc etc.
 
Last edited:
You must log in or register to reply here.
Back
Top