Deck of cards implementation

Question

Update: Simple Blackjack implementation

I was playing Blackjack in a phone app and decided to try to do the game in Java, using the console for a start. So far I'd like to review the deck of cards implementation. I decided to use an OOP approach with the classes Value, Suit, Card and Deck.

First of all, I'd like to get some feedback and pieces of advice about how I decided to structure my code. Is there anything wrong? Should something be done differently?

import java.util.Stack;
import java.util.Random;
import java.util.Collections;
public class Blackjack {
 public static String capitalizeFully(String str) {
 String s = new String();
 for (int i = 1; i < str.length(); i++) {
 s += Character.toLowerCase(str.charAt(i));
 }
 return Character.toUpperCase(str.charAt(0)) + s;
 }
 public static class Value {
 private int value;
 private static final String[] string_values = {
 "Ace",
 "Two",
 "Three",
 "Four",
 "Five",
 "Six",
 "Seven",
 "Eight",
 "Nine",
 "Ten",
 "Jack",
 "Queen",
 "King"
 };
 public Value(int value) throws IllegalArgumentException {
 if (value < 1 || value > 13) {
 throw new IllegalArgumentException("Invalid card value (must be between 1 and 13).");
 }
 this.value = value;
 }
 public Value(String value) throws IllegalArgumentException {
 int i = 0;
 String val = capitalizeFully(value);
 while (i < string_values.length && !val.equals(string_values[i])) {
 i++;
 }
 if (i == string_values.length) {
 throw new IllegalArgumentException("Invalid card value.");
 } else {
 this.value = i + 1;
 }
 }
 public String toString() {
 return string_values[value - 1];
 }
 }
 public static class Suit {
 private String name;
 private static final String[] string_names = {
 "Hearts",
 "Diamonds",
 "Clubs",
 "Spades"
 };
 public Suit(String name) throws IllegalArgumentException {
 int i = 0;
 String n = capitalizeFully(name);
 while (i < string_names.length && !n.equals(string_names[i])) {
 i++;
 }
 if (i == string_names.length) {
 throw new IllegalArgumentException("Invalid suit name.");
 } else {
 this.name = n;
 }
 }
 public String toString() {
 return name;
 }
 }
 public static class Card {
 private Value value;
 private Suit suit;
 public Card(Value value, Suit suit) {
 this.value = value;
 this.suit = suit;
 }
 public Card(int value, Suit suit) {
 this.value = new Value(value);
 this.suit = suit;
 }
 public Card(String value, Suit suit) {
 this.value = new Value(value);
 this.suit = suit;
 }
 public Card(Value value, String suit) {
 this.value = value;
 this.suit = new Suit(suit);
 }
 public Card(int value, String suit) {
 this.value = new Value(value);
 this.suit = new Suit(suit);
 }
 public Card(String value, String suit) {
 this.value = new Value(value);
 this.suit = new Suit(suit);
 }
 public String toString() {
 String str = value + " of " + suit;
 return str;
 }
 }
 public static class Deck {
 private Stack<Card> deck;
 private static final int number = 52;
 public Deck() {
 deck = new Stack<Card>();
 Suit hearts, diamonds, clubs, spades;
 hearts = new Suit("hearts");
 diamonds = new Suit("diamonds");
 clubs = new Suit("clubs");
 spades = new Suit("spades");
 Suit[] suits = { hearts, diamonds, clubs, spades };
 for (int i = 0; i < suits.length; i++) {
 for (int j = 1; j <= 13; j++) {
 deck.push(new Card(j, suits[i]));
 }
 }
 }
 public void shuffle() {
 long seed = System.nanoTime();
 Collections.shuffle(deck, new Random(seed));
 }
 public void add(Card card) {
 deck.push(card);
 }
 public Card draw() {
 return deck.pop();
 } 
 public String toString() {
 String str = new String();
 for (int i = 0; i < deck.size(); i++) {
 str += deck.get(i) + "\n";
 }
 return str;
 }
 }
 public static <T> void p(T output) {
 System.out.println(output);
 }
 public static void main(String[] args) {
 Deck deck = new Deck();
 deck.shuffle();
 p(deck);
 }
}

However, this is an implementation for just the 52-card standard deck. How about different decks, with different suits and a different range of values? I thought maybe it'd be a good idea that instead of hardcoding these attributes, I could make an alternative implementation where the user can define his/her own set of suits and range of values, so the deck is constructed given these set of choices.

For example:

Value[] values = { new Value("As", 1), new Value("Dos", 2), ..., new Value("Rey", 12) };
Suit[] suits = { new Suit("Oros"), new Suit("Sotas"), new Suit("Espadas"), new Suit("Bastos") };
Deck spanish_deck = new Deck(suits, values);

Does this second alternative make sense? Is it a good approach?

Update: Simple Blackjack implementation

• RHok: As for the "second alternative": I think it's a good idea. You could overload the constructor for deck maybe? Without params it uses the default 1-13, Hearts Diamonds Spades Clubs values, else it makes a deck of any combination of values and suits that you put in to it. I'd be interested in seeing your blackjack implementation too, as it's something I've attempted before
• dabadaba: check out my update

Answer 1

For starters, using Java enums will likely save you a lot of the boilerplate-like template code you have for Value and Suit.

Your Card constructors seem to be over-engineered too... what you have are just permutations of ints, Strings and objects arguments... I suggest you pick one and stick with that, preferably just Value and Suit. If your Card objects are meant to be immutable, I'll suggest saving its String representation (used, naturally, in toString()) as an instance field first. In fact, you should probably go a step further and override the equals() and hashCode() methods too to simplify/standardize such usage for the class. This will be helpful if you are using them in most, if not all, Collection classes.

Unfortunately, your implementation for capitalizeFully() doesn't seem too good... First, I'm not sure what it's meant by 'fully' capitalizing a String. I thought you meant converting a String to uppercase. Turns out you are just capitalizing the first letter. So why not call that then?

private static String capitalizeFirstLetter(String input) {
 // ...
}

Unless you are constructing Strings from byte/char arrays, with an optional Charset, you should (almost) never need to instantiate a String by doing new String(). Then, using the toLowerCase() and toUpperCase() methods...

private static String capitalizeFirstLetter(String input) {
 // handle null or "" Strings if required
 return input.substring(0, 1).toUpperCase() + input.substring(1).toLowerCase();
}

If you want to cater for Locale-specific conversions, there are methods for that too.

Last, you need a much better name for your print (?) p() method... what is it supposed to do actually? The returned object doesn't appear to be used in any way.

edit further explanation for an enum-approach

Say you have a Rank and Suit enum:

enum Rank {
 ONE,
 TWO,
 // ...
 JACK,
 QUEEN,
 KING,
 ACE; // opt-ing to 'rank' ACE as the highest
 public int getValue() {
 return ordinal() + 1;
 }
}
enum Suit {
 CLUB,
 DIAMOND,
 HEART,
 SPADE;
}

Some benefits of using enums are:

• Values are Comparable to each other.
• Efficient use in Maps and Sets via EnumMap and EnumSet classes, when required.
• built-in toString() representation.
• built-in index-based lookup for values via ordinal().
• built-in String-based lookup for values via valueOf(String).
• safe ==-based comparison.

For your Card class, you can then do something like:

// note: final modifier to indicate this class cannot be subclassed
// in order to preserve the immutable features
public final class Card {
 private final Rank rank;
 private final Suit suit;
 private final String toString;
 private final int hashCode;
 public Card(Rank rank, Suit suit) {
 this.rank = rank;
 this.suit = suit;
 this.toString = getToString(rank, suit);
 // Objects.hash() is from Java 7
 this.hashCode = Objects.hash(rank, suit);
 }
 private static String getToString(Rank rank, Suit suit) {
 // code review comment: will return e.g. "Seven of Hearts"
 return String.format("%s of %ss",
 capitalizeFirstLetter(rank.toString()),
 capitalizeFirstLetter(suit.toString()));
 }
 @Override
 public String toString() {
 return toString;
 }
 @Override
 public int hashCode() {
 return hashCode;
 }
 @Override
 public boolean equals(Object o) {
 if (!(o instanceof Card)) {
 return false;
 }
 Card otherCard = (Card) o;
 return otherCard.rank == rank && otherCard.suit == suit;
 }
}

I hope this kind of illustrates that your Value/Rank, Suit and Card classes can have similar functionality with less boilerplate-like template code... The idea behind pre-computing the String representation and hash code first is to avoid repeated computations, should the methods be called frequently. This works especially well for immutable classes like the Card class as shown.

• dabadaba: p() is just for printing, when I am using Java it's very annoying for me to have to write System.out.println() again and again, which is pretty long. The returning type is not necessary, I shall remove it. I turned Suit and Value into classes (they were enumoriginally) to avoid delegating the responsability of checking correct attributes to the Cardclass, as well as hardcoding the available choices. I don't understand what you mean with the part where you say if my Card objects are mean to me immutable until the end of the paragraph.
• h.j.k.: Wells, I guess you can configure your IDE with a keyboard shortcut for System.out.println()... can you explain what you mean by 'checking correct attributes to the Card class'?
• dabadaba: What I mean I am throwing exceptions in the Suit and Value classes, as well as creating different constructors, attributes and the toString() method. I am not using an IDE, just Sublime.
• h.j.k.: immutability means a Card object will not have its suit or value (actually, the proper 'card' term is rank) changed after instantiation. This reduces the chances of concurrent modification (leading to invalidated results) when used in a multi-threaded context.
• dabadaba: So you mean that if Card is immutable I should probably get rid of the Suit and Value classes and turn them into Card's String attributes? Could you explain what you mean by then overriding equals() and hashCode()?