6
\$\begingroup\$

I am currently learning how to write code in Java. To improve my understanding of OOP concepts, I am creating a playing card and deck class. The deck has the functionality of drawing and shuffling. I also attach an image to my card class (using JavaFX). Am I doing it right?

I am also planning to extend this class into a fully functional card game with JavaFX GUI.

Since I am self-learning, any kind of feedback would be greatly appreciated.

SuitEnum Class

public enum SuitEnum {
    DIAMONDS, CLUBS, HEARTS, SPADES;

}

RankEnumClass

public enum RankEnum {
    TWO, THREE, FOUR, FIVE, SIX, SEVEN, EIGHT, NINE, TEN, JACK, QUEEN, KING, ACE;

}

Card Class

import javafx.scene.image.*;

public class Card implements Comparable<Card>{



private static final String IMAGE_FOLDER_DIR = "image";
private static final String IMAGE_FORMAT = ".png";
private static final String BACK_IMAGE_DIR = ("image/back_image.png");

private Image cardImage;
private Image backImage;

private SuitEnum suit;
private RankEnum rank;


public Card(){

}       

public Card(SuitEnum suit, RankEnum rank){
    this.suit = suit;
    this.rank = rank;

    String location = generateImageLocation();

    try {
        cardImage = new Image(location);

    } catch (Exception ex) {
        System.out.println(String.format("cannot load image from: (%s)", location));        
        cardImage = null;
    }

    try {
        backImage = new Image(BACK_IMAGE_DIR);
    } catch (Exception ex){
        System.out.println(String.format("cannot load image from: (%s)", BACK_IMAGE_DIR));
        backImage = null;
    }
}

public SuitEnum getSuit() {
    return suit;
}



public RankEnum getRank() {
    return rank;
}



public Image getCardImage(){
    return cardImage;
}

private String generateImageLocation(){

    StringBuilder sb = new StringBuilder();

    sb.append(IMAGE_FOLDER_DIR);
    sb.append("/");

    sb.append(suit.toString());
    sb.append("_");
    sb.append(rank.toString());

    sb.append(IMAGE_FORMAT);



    return sb.toString().toLowerCase();

}

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

public int compareTo(Card card) {

    if (this.rank.compareTo(card.rank) > 0){
        return 1;
    } else if (this.rank.compareTo(card.rank) < 0){
        return -1;
    } else {

        if(this.suit.compareTo(card.suit) > 0){
            return 1;
        } else if (this.suit.compareTo(card.suit) < 0){
            return -1;
        } else {
            return 0;
        }

    }


}

@Override
public int hashCode() {
    final int prime = 31;
    int result = 1;
    result = prime * result + ((rank == null) ? 0 : rank.hashCode());
    result = prime * result + ((suit == null) ? 0 : suit.hashCode());
    return result;
}

@Override
public boolean equals(Object obj) {
    if (this == obj)
        return true;
    if (obj == null)
        return false;
    if (getClass() != obj.getClass())
        return false;
    Card other = (Card) obj;
    if (rank != other.rank)
        return false;
    if (suit != other.suit)
        return false;
    return true;
}

public Image getBackImage() {
    return backImage;
}

Deck Class

import java.util.ArrayList;
import java.util.Random;

public class Deck {

    private ArrayList<Card> deck = new ArrayList<>();

    public Deck(){

        for (SuitEnum suit: SuitEnum.values()){
            for (RankEnum rank : RankEnum.values()){
                deck.add(new Card(suit, rank));
            }
        }

    }

    //shuffle deck
    public void shuffle(){
        ArrayList<Card> tempOldDeck = new ArrayList<>();

        for (Card card: deck){
            tempOldDeck.add(card);
        }

        int[] randomPermutation = generateRandomPermutation(deck.size());

        for (int i = 0; i < deck.size(); i++){  
            deck.set(i, tempOldDeck.get(randomPermutation[i] - 1));
        }       
    }

    private int[] generateRandomPermutation(int high){
        return generateRandomPermutation(1, high);
    }

    private int[] generateRandomPermutation(int low, int high){
        ArrayList<Integer> unselectedNumber = new ArrayList<>();

        for (int i = low; i <= high; i++){
            unselectedNumber.add(i);
        }

        Random rng = new Random();
        int[] randomPermutation = new int[high - low + 1];

        for (int i = 0; i < randomPermutation.length; i++){
            int randomIndex = rng.nextInt(unselectedNumber.size());
            randomPermutation[i] = unselectedNumber.get(randomIndex);
            unselectedNumber.remove(randomIndex);
        }

        return randomPermutation;
    }

    //draw top most
    public Card draw() throws EmptyDeckException{
        if (deck.size() > 0){
            Card drawnCard = deck.get(deck.size() - 1);
            deck.remove(deck.size() - 1);
            return drawnCard;
        } else 
            throw new EmptyDeckException();
    }

    //get deck size
    public int size(){
        return deck.size();
    }
}

EmptyDeckException Class

public class EmptyDeckException extends Exception{

    public EmptyDeckException(){
    }

    public EmptyDeckException(String message){
        super(message);
    }

    public EmptyDeckException(Throwable cause){
        super(cause);
    }

    public EmptyDeckException(String message, Throwable cause){
        super(message, cause);
    }
}
\$\endgroup\$
11
\$\begingroup\$

Naming

private static final String BACK_IMAGE_DIR = ("image/back_image.png");

You call this BACK_IMAGE_DIR which suggests that it is a directory, but it's actually a file path. Just BACK_IMAGE might be a better name.

Are you sure that you want this to be a property of a Card? It seems more common to make all cards in a deck share the same back.

Keep it simple

        if(this.suit.compareTo(card.suit) > 0){
            return 1;
        } else if (this.suit.compareTo(card.suit) < 0){
            return -1;
        } else {
            return 0;
        }

You could write this section more simply as

        return this.suit.compareTo(card.suit);

And this section

    if (rank != other.rank)
        return false;
    if (suit != other.suit)
        return false;
    return true;

could be simplified to

    return rank == other.rank && suit == other.suit;

Variable types should be interfaces

private ArrayList<Card> deck = new ArrayList<>();

As a general rule, prefer

private List<Card> deck = new ArrayList<>();

This allows you to change the implementation easily, without affecting other parts of the program. The exception would be if you were using some functionality that was in ArrayList but not List. I don't see such functionality here nor in the other ArrayList declarations, so they could each be changed to List instead.

Keep it simple, again

        Card drawnCard = deck.get(deck.size() - 1);
        deck.remove(deck.size() - 1);
        return drawnCard;

You could just say

        return deck.remove(deck.size() - 1);

with the same effect.

Block form is clearer than statement form

    } else throw new EmptyDeckException();

As a general rule in the C-style languages, it is more robust and clearer to use block form in control statements.

    } else {
        throw new EmptyDeckException();
    }

Also consider writing the method the other way

    if (deck.size() <= 0) {
        throw new EmptyDeckException();
    }

    return deck.remove(deck.size() - 1);

That handles the exceptional case before going through the main logic.

Favor built-ins over custom

    ArrayList<Card> tempOldDeck = new ArrayList<>();

    for (Card card: deck){
        tempOldDeck.add(card);
    }

You can just say

        List<Card> tempOldDeck = new ArrayList<>(deck);

Then you don't have to specify the for loop.

    for (int i = 0; i < deck.size(); i++){  
        deck.set(i, tempOldDeck.get(randomPermutation[i] - 1));
    }

You can write this as

    deck.empty()
    for (int i : randomPermutation) {
        deck.add(tempOldDeck.get(i - 1);
    }
\$\endgroup\$
10
\$\begingroup\$
  1. Use interfaces as much as you can.

    private List<Card> deck = new ArrayList<>();
    
  2. Checked Exceptions are bad, don't use them

    public class EmptyDeckException extends RuntimeException{
     ..
    }
    
    public Card draw(){ // no throws 
    }
    
  3. You don't need to write a shuffle method yourself, there is a one in the JDK already

    List<Integer> numbers = new ArrayList<>();
    
    for (int i = low; i <= high; i++){
        numbers.add(i);
    }
    Collections.shuffle(numbers);
    
\$\endgroup\$
  • 4
    \$\begingroup\$ Your "Checked Exceptions are bad, don't use them" link does not say that they are bad, and in fact creates good arguments for them. But yes, they shouldn't be in this code. \$\endgroup\$ – AlbeyAmakiir Jun 16 '15 at 3:24
4
\$\begingroup\$

I'll start with more style oriented comments and move to more general OO concepts. And I'll skip what's already been said.

Enums

I much prefer to see enum values declared on individual lines, for readability. I also don't think including the word "enum" in your enum definitions is helpful. I'd like to see

public enum Suit {
    Diamonds,
    Clubs,
    Hearts,
    Spades
}

Spacing

You use spaces inconsistently. In general, this suggests a lack of attention to your code and is unpleasant to read. The end of your compareTo method, for example, looks like

            return 0;
        }

    }


}

and should look like

            return 0;
        }
    }
}

Overrides

compareTo is an interface method, but you've missed the @Override annotation. Always include that annotation when implementing a method from an inherited base class or from an interface.

Builders

Most methods in builders (it's a general design pattern) return a reference to the builder so you can chain method calls like such

private Strin generateImageLocation(){
    return new StringBuilder()
            .append(IMAGE_FOLDER)
            .append("/")
            .....
            .toString()
            .toLowerCase();
}

Data Types

This was you first bigger OO mistake to me. When you think of a deck of cards, do you think of a randomly accessible array of cards? Do you think of a stack of cards? It is possible that your game may require your ability to draw cards from anywhere in the deck, but from the code you've provided it doesn't seem this is the case. I'd like to see

    private Stack<Card> deck = new Stack<Card>();

Then your draw method would look like

 public Card draw(){
    if(deck.size() > 0) {
        return deck.pop();
    } else {
        throw new EmptyDeckException();
    }
}

Hashing

We know enough about decks and cards to write a perfect hash function (one which will return a different value for each of the 52 different possible cards we could have) with the use of enum orinals.

public int hashCode(){
    return (suit.ordinal() * 13) + rank.ordinal();
}

Separation of Concerns

I would like to see the association between a Card and an Image in a different place in your program. In general when you build GUI applications, you want to separate the things the User sees from the representation of data in your program. With you current design, you make it challenging to build a flexible UI. What if 3 months from now you decide you've got a good game, and now you want to support multiple "skins" on your cards, so that users can customize the UI to their liking. Now you've got to come change the implementation of the Card class - not something you should have to do to make a change to the way the UI looks.

\$\endgroup\$
3
\$\begingroup\$

Other than the excellent answers already posted one thing that I asked myself while looking through these is Why even have the empty constructors?

You generally wouldn't want someone coming in an instantiating a card(). remove them to ensure that each object is properly instantiated.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.