Implementation of a Card class in Java

Question

I have just coded a Card class which gives a suit, rank and a color to a card. However, I have serious doubts that my code is not well organized and not well structured.

I would like to receive tips on how to improve my code if possible so I can learn from them.

Card class:

import java.util.Random;

public class Card {

    // Setting variables
    String cardSuit, cardRank, cardColor;
    Random generator = new Random();

    // Setting two arrays with the suits and ranks options
    String[] suits = {"hearts", "spades", "diamonds", "clubs"};
    String[] ranks = {"Ace", "2", "3", "4", "5", "6", "7", "8",
    "9", "10", "Jack", "Queen", "King"};

    int randomSuits = generator.nextInt(suits.length);
    int randomRanks = generator.nextInt(ranks.length);


    // Really not sure if I am using the Constructor the right way
     Card() {
         /* Using Random to get a random element from the suits array 
            and an random element from the ranks array.
         */
         cardSuit = suits[randomSuits];
         cardRank = ranks[randomRanks];
         // Setting the color of the card in dependence of the suit
         if (cardSuit.equals("hearts") || cardSuit.equals("diamonds")) {
             cardColor = "red";
         }
         else {
             cardColor = "black";
         }
     }


     // Methods to print out the results
     public void getSuit() {
         System.out.println(cardSuit);
     }

     public void getRank() {
         System.out.println(cardRank);
     }

     public void getColor() {
         System.out.println(cardColor);
     }
}

NewCard class:

public class NewCard {
    public static void main(String[] args) {

        // Creating a new Card object
        Card card1 = new Card();
        card1.getSuit();
        card1.getRank();
        card1.getColor();
    }
}

Answer 1

Naming

Your Card class is currently composed of 3 fields, which represent its suit, its rank and its color:

String cardSuit, cardRank, cardColor;

Note that in the sentence above, the term used where "suit", "rank" and "color" and not "cardSuit", "cardRank" and "cardColor". What this means is that you should have

String suit, rank, color;

There is no need to repeat the class name in the fields composing it. The name given to each field should best represent what it stores. If we take the example of cardSuit then this field really represent the suit (of this card).

Consider also the case of accessing such field, you would have someCard.cardSuit with your current naming: this introduces a repetition and a long name; it is clearer to have someCard.suit.

Controlling access to the fields

When desiging a class, you should also decide on which access to give to each members. For example, a public variable will be accessible by everybody but a private variable will only be visible inside your own class.

When you are declaring the fields as

String cardSuit, cardRank, cardColor;

they actually use the default access, which means every class in the same package as of the Card class will be able to access them. This is most likely not what you want here: these are internal fields and it would be preferable to keep them private. It ensures that only modifications made inside the Card class are allowed, which goes line in line with the OOP principle: encapsulating your state without leaving the chance of others to mess it up.

So, instead, you should have:

private String cardSuit, cardRank, cardColor;

---

Then there is the matter of the constructor. Currently, the constructor doesn't have an access modifier also, which means it can be accessed by classes in the same package (just like above). This also means that the only classes that will be able to construct Card instances are only classes present in the same package.

Such a restriction is probably not what you want: you want everyone to be able to constructor a Card instance. So, in this case, make the constructor public:

public Card() {
    // ...
}

Constants

Instance fields are specific to each instance of a class. So when you declare

// Setting two arrays with the suits and ranks options
String[] suits = {"hearts", "spades", "diamonds", "clubs"};
String[] ranks = {"Ace", "2", "3", "4", "5", "6", "7", "8",
"9", "10", "Jack", "Queen", "King"};

you actually declare a suits and ranks array with the same content for each card instance. However, all the possible values for the suits and the ranks are not specific to any card instance. In fact, it applies for every card and it will always be the same.

In this case, it is preferable to use a constant variable; that is to say a variable defined at a single place, that every instance will share, and that cannot be modified. The typical pattern is to declare them private static final, with a name in uppercase:

private static final String[] SUITS = {"hearts", "spades", "diamonds", "clubs"};
private static final String[] RANKS = {"Ace", "2", "3", "4", "5", "6", "7", "8", "9", "10", "Jack", "Queen", "King"};

Scope of variables

To get a random suit and a random rank, you currently generate two integers as instance members of the Card class:

int randomSuits = generator.nextInt(SUITS.length);
int randomRanks = generator.nextInt(RANKS.length);

Card() {
    cardSuit = SUITS[randomSuits];
    cardRank = RANKS[randomRanks];
    // ...
}

But note this is the only place where you are using those variables.

This is generally not a good idea: it means that you just created two variables that can be used and referenced anywhere in your class, but you only need them in a specific, particular piece of code. The scope of a variable should be as minimal as possible: only declare variable where they are needed.

In this case, they are only needed inside the constructor, so declare them here:

Card() {
    int randomSuits = generator.nextInt(SUITS.length);
    int randomRanks = generator.nextInt(RANKS.length);
    cardSuit = SUITS[randomSuits];
    cardRank = RANKS[randomRanks];
    // ...
}

and, when you do that, you realize that maybe you don't even need to store them at all: they are used only once, so this would be even more easier to read:

Card() {
    cardSuit = SUITS[generator.nextInt(SUITS.length)];
    cardRank = RANKS[generator.nextInt(RANKS.length)];
    // ...
}

Accessors

Accessors, or getter / setter, is the typical way to expose your private fields to the outside world. They are typically named get followed by the name of the field.

In your class, you have

public void getSuit() {
    System.out.println(cardSuit);
}

public void getRank() {
    System.out.println(cardRank);
}

public void getColor() {
    System.out.println(cardColor);
}

but those are not getter. They do not return the value of the field. They print it instead.

Make them real getters with:

public String getSuit() {
    return cardSuit;
}

public String getRank() {
    return cardRank;
}

public String getColor() {
    return cardColor;
}

There implication is not that the Card class now has a way to let the outside world know the value of the suit, the rank and the color of a card. So it is up to the calling code to print the result with:

Card card1 = new Card();
System.out.println(card1.getSuit());
System.out.println(card1.getRank());
System.out.println(card1.getColor());

Random integers

Instances of Random should be reused. Currently, each card instance will create its own Random field with

Random generator = new Random();

It is preferable to have a single Random object that is shared. In this case, we can make it constant:

private static final Random GENERATOR = new Random();

This way, the same Random will be used for all cards.

Putting it all together

By putting all this into code, one could have:

public class Card {

    private static final Random GENERATOR = new Random();

    // Setting two arrays with the suits and ranks options
    private static final String[] SUITS = { "hearts", "spades", "diamonds", "clubs" };
    private static final String[] RANKS = { "Ace", "2", "3", "4", "5", "6", "7", "8", "9", "10", "Jack", "Queen", "King" };

    // Setting variables
    private String suit, rank, color;

    public Card() {
        suit = SUITS[GENERATOR.nextInt(SUITS.length)];
        rank = RANKS[GENERATOR.nextInt(RANKS.length)];
        // Setting the color of the card in dependence of the suit
        if (suit.equals("hearts") || suit.equals("diamonds")) {
            color = "red";
        } else {
            color = "black";
        }
    }

    public String getSuit() {
        return suit;
    }

    public String getRank() {
        return rank;
    }

    public String getColor() {
        return color;
    }
}

Answer 2

Tunaki makes a very good point that your suites and ranks groupings should not be part of a card instance. However, I disagree that they should be kept as strings. I would make these groups into an enum.

public enum Suit {
    Heart,
    Spade,
    Diamond,
    Club
}

Then, you can have a private Rank cardRank field that is assigned like cardRank = Rank.Heart, that is more strictly-coded than using a string, guaranteeing that the compiler catches when you type Haert instead Heart.

Answer 3

I'd extend Hosch250's point and make even better use of enums. First, make the color an enum too:

public enum Color {
    RED, BLACK
}

Since the color is tied to the suit, you could make it a field of the enum, along with the information on how to display the suit -- by default, you'd get the .name() of the enum, i.e. uppercase "HEARTS" or "SPADES" and that wouldn't look so nice:

public enum Suit {
    HEARTS(Color.RED, "hearts"), DIAMONDS(Color.RED, "diamonds"), SPADES(Color.BLACK, "spades"), CLUBS(Color.BLACK, "clubs");

    private Color color;
    private String displayName;

    Suit(Color color, String displayName) {
        this.color = color;
        this.displayName = displayName;
    }

    public Color getColor() {
        return color;
    }

    public String getDisplayName() {
        return displayName;
    }
}

Creating an enum of the ranks seems like a good idea to me, too. I certainly wouldn't have them as Strings, they are difficult to compare. Something like this:

public enum Rank {
    ACE(1, "Ace"), TWO(2, "2"), THREE(3, "3"), FOUR(4, "4"), FIVE(5, "5"), SIX(6, "6"), SEVEN(7, "7"),
    EIGHT(8, "8"), NINE(9, "9"), TEN(10, "10"), JACK(11, "Jack"), QUEEN(12, "Queen"), KING(13, "King");
    private final int value;
    private final String displayName;

    Rank(int value, String displayName) {
        this.value = value;
        this.displayName = displayName;
    }

    public int getValue() {
        return value;
    }

    public String getDisplayName() {
        return displayName;
    }

    public static Rank of(int value) {
        return java.util.Arrays.stream(Rank.values())
                .filter(rank -> rank.getValue() == value)
                .findFirst()
                .orElseThrow(() -> new IllegalArgumentException("No card with value " + value + " exists."));
    }
}

Notice the static utility method of(int) that creates you a Rank enum based on the int argument. Call it like this: Rank.of(11) and it returns you a proper Rank.JACK object.

Now, about the constructor. I would find it really confusing if a constructor initialized an object with a completely random state! Consider, instead, a constructor that accepts a Rank and a Suit, and a static method for creating random cards. That would also allow you to create any specific card you might want:

private final Rank rank;
private final Suit suit;

public Card(Rank rank, Suit suit) {
    this.rank = rank;
    this.suit = suit;
}

public static Card random() {
    Random generator = new Random();
    Rank rank = Rank.values()[generator.nextInt(Rank.values().length)];
    Suit suit = Suit.values()[generator.nextInt(Suit.values().length)];
    return new Card(rank, suit);
}

Now, instead of initializing your card with Card card1 = new Card(); you'd do Card card1 = Card.random();. Much more clear and intuitive. And if you specifically wanted the ace of spades, you'd call Card card1 = new Card(Rank.ACE, Suit.SPADE);. Notice also how I finalized the suit and rank fields so that they cannot be changed after the object is created.

Change the getSuit(), getRank() and getColor() methods to return the enum types and in the end you can print all the information like this:

System.out.println(card1.getSuit().getDisplayName());
System.out.println(card1.getRank().getDisplayName());
System.out.println(card1.getColor().getDisplayName()); // OK, this is left as an excercise for you. ;)

Alternatively, your Card doesn't need a getColor() at all, you could just call card1.getSuit().getColor(). If your Card had the getColor() method anyway, it would be like this:

public Color getColor() {
    return this.suit.getColor();
}

Finally, toString. Consider implementing a toString method in your Card class. It allows you to render your object in a readable format just by printing the object itself. Allow me to demonstrate. Currently, if you did System.out.println(Card.random()); you'd get a pretty much non-sensical output along the lines of "Card@1540e19d". Now, implement this method in your Card class:

@Override
public String toString() {
    return rank.getDisplayName() + " of " + suit.getDisplayName();
}

And the same System.out.println(Card.random()); would print a nice "7 of clubs" instead (your result may vary, thanks to the random part ;)!