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.
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.
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.
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}; } }
Re: Illegal reference to static field from initializer, found #3 below https://dexence.com/java-annoyances/
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.