Implementation of a Card class in Java

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();
    }
}

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 t

StackExchange Code Review Q#125873, answer score: 13