10
\$\begingroup\$

So I'm in the process of creating a simple Contract Bridge in Java. I will be using JavaFX to create the GUI component of the program. For right now I am trying to tackle writing each of the classes needed to run the game.

If you're unfamiliar with bridge the short explanation is as follows: you start with a 52 card deck, shuffled w/ no jokers. There will be four players, two teams, the player who is opposite of you will be you team mate. The deck is dealt and each player gets 13 cards. The turns run to the left. Basically, find the highest value card for a given suit, who ever has the largest value wins the round.

I would like some advice as far as design goes, also feedback on the classes I've created so far. It is my first time creating a game so it feels a bit foreign.

I've identified four classes I will create: Card, Deck, Player, and Game.

Here is my Card class:

public class Card {
    private Suit suit;
    private CardValue cardValue;


    public Card (CardValue cardValue, Suit suit)
    {
        this.cardValue = cardValue;
        this.suit = suit;
    }

    public Suit getSuit()
    {
        return suit;
    }

    public void setSuit(Suit suit)
    {
        this.suit = suit;
    }

    public CardValue getCardValue()
    {
        return cardValue;
    }

    public void setCardValue(CardValue cardValue)
    {
        this.cardValue = cardValue;
    }

    public enum Suit {
        SPADES,
        HEARTS,
        DIAMONDS,
        CLUBS;
    }

    public enum CardValue
    {
        ACE(1),
        TWO(2),
        THREE(3),
        FOUR(4),
        FIVE(5),
        SIX(6),
        SEVEN(7),
        EIGHT(8),
        NINE(9),
        TEN(10),
        JACK(11),
        QUEEN(12),
        KING(13);

        private int cardValue;

        private CardValue (int value)
        {
            this.cardValue = value;
        }

        public int getCardValue() {
            return cardValue;
        }
    }
    //TODO
    public String toString() {
        switch (suit) {
            case 1:
                System.out.print
        }
    }
}

Deck:

    public class Deck {
    /** deck instance variable */
    private Card[] deck;
    /** Tracks cards dealt */
    private int cardsDealt;

    /** Construct a standard 52 card deck. Un-shuffled*/
    public Deck() {
        this.deck = new Card[52];
    }

    /** Constructs a standard 52 card deck. Shuffled*/
    public Deck(boolean isShuffled){
        if(isShuffled == false)
            this.deck = new Card[52];
        else{
            this.deck = new Card[52];
            shuffle();
        }
    }

    /** Shuffle deck */
    public void shuffle() {
        for ( int i = deck.length - 1; i > 0; i-- ) {
            int randomize = (int)(Math.random()*( i + 1 ));
            Card temp = deck[i];
            deck[i] = deck[randomize];
            deck[randomize] = temp;
        }
        cardsDealt = 0;
    }

    /** Returns cards left in the deck */
    public int cardsLeft() {
        return deck.length - cardsDealt;
    }
    /** Deals card */
    public Card dealCard() {
        if (cardsDealt == deck.length)
            throw new IllegalStateException("No cards are left in the deck.");
        cardsDealt++;
        return deck[cardsDealt - 1];
    }
}
\$\endgroup\$
13
\$\begingroup\$

Cards

public void setSuit(Suit suit)
{
    this.suit = suit;
}

No! No! No! Just drop the setters. With them, you can never return a Card from any method without the risk that someone changes it. Cards are immutable in all games I know and must be immutable in the code, too.

Moreover, never write methods you don't need (unless you know you'll need them very soon). Especially don't write needless trivial methods.

private Suit suit;

This should be

private final Suit suit;

Make everything final as much as possible.

ACE(1),

This can't be right in Bridge where ace is the highest card (and never has the role of 1 as e.g. in Rummy).

Actually, you needn't to number the CardValues at all. There's a predefined method ordinal(). Arguably, it's not good to use it outside of the class as it's not flexible at all (the value is determined solely by the position). But you can do this:

public enum CardValue {
    TWO,
    THREE,
    FOUR,
    ...
    JACK,
    QUEEN,
    KING,
    ACE,
    ;

    public int getCardValue() {
        return ordinal() + 2;
    }
}

I'd also add a toString() method. The default is good, you may want to capitalize it (as SirPython wrote), but I'm pretty sure, that anything that long is unusable (write down a single hand with 13 cards to see it). And there are standard 1-letter abbreviations, so go for them:

public enum CardValue {
    ...

    public String toString() {
        return name().substring(0, 1);
    }
}

I'd do about the same for values:

public enum CardValue {
    ...

    public String toString() {
        return cardValue < 10 
            ? String.valueOf(cardValue)
            : name().substring(0, 1);
    }

    }

This way you get 2, 3, ..., 9, T, J, Q, K, A (where T standing for 10 might be a bit uncommon, but it's the best if you want a single char).

Concerning your Card::toString, I'd go simply for

 public String toString() {
     return cardValue.toString() + suit.toString();
 }

Deck

public Deck(boolean isShuffled){
    if(isShuffled == false)
        this.deck = new Card[52];
    else{
        this.deck = new Card[52];
        shuffle();
    }
}

Instead of x == false, write !x. Or better, reverse the branches. Or even better, extract the common part:

public Deck(boolean isShuffled){
    this.deck = new Card[52];
    if(isShuffled) {
        shuffle();
    }
}

However, this doesn't look right. You created an array of cards without filling it. So there's nothing to shuffle!

You can move the array allocation to where it belongs:

private final Card[] deck = new Card[52];

(subject to what has been said about magic constants in code), and in your constructor you need something like

int index = 0;
for (Suit s : Suit.values()) {
    for (CardValue v : CardValue.values()) {
         deck[index++] = new Card(v, s);
    }
}

This looks like the right algorithm

public void shuffle() {
    for ( int i = deck.length - 1; i > 0; i-- ) {
        int randomize = (int)(Math.random()*( i + 1 ));
        Card temp = deck[i];
        deck[i] = deck[randomize];
        deck[randomize] = temp;
    }
    cardsDealt = 0;
}

however, nobody should ever use Math.random() for this.

Random random = new Random();
...
int randomize = random.nextInt(i + 1);

is the way to go. It doesn't make you sweet when trying to find out how the double gets rounded and it ensures a uniform distribution. It also allows you to use a fixed seed and so make it repeatable.

There are tons of card-related questions here on CR (the most recent is mine), you may want to look at them. To summarize, your code is fine, my most important points are immutability and YAGNI.

\$\endgroup\$
  • \$\begingroup\$ Wow, really helpful feedback. This is my first card game, so I'm a bit uncertain of how to write the program. I was wondering if it be better to write the suit enum as you did your question: SPADES('S'), HEARTS('H'), CLUBS('C'), DIAMONDS('D'), ; Would this make the toString much cleaner to write? \$\endgroup\$ – user69928 Apr 26 '15 at 3:43
  • \$\begingroup\$ @n0tion Writing SPADES('S') is fine, just a little bit more verbose. There's a speed advantage (you don't call substring) and it's more explicit. I actually took it over from the question I reviewed, my preferred choice would be the less verbose version (though the difference is tiny because of Lombok). However, it all doesn't matter much as it's all private and simple to change (in a CR I tend to show an alternative). \$\endgroup\$ – maaartinus Apr 26 '15 at 3:53
  • 1
    \$\begingroup\$ A bridge writer (I forget the name. but I agree with him) suggests S4 or ♠4 rather than 4S or 4♠ as notation for the Spade Four, since 4S and 4♠ would be notations for the bid "four spades". Using the suit as a prefix will also facilitate "compressing" string representations, especially when showing multiple cards. \$\endgroup\$ – supercat Apr 27 '15 at 2:47
7
\$\begingroup\$

I don't believe that this is exactly what you were looking for in a review, but I thought I'd point it out:

In the Card class's toString method, it looks like you are starting to create a switch statement which will check each possible numerical value of the enum property suit and, depending on the number, log out what the card is.

However, I think a better way to do this would be to, in the enum, set a string property that contains the name of the suit.

Then, your Suit enum would become this:

public enum Suit {
    SPADES("Spades"),
    HEARTS("Hearts"),
    DIAMONDS("Diamonds"),
    CLUBS("Clubs");

    ...
}

In the "..." section, you would put something like a name field, a constructor, and a method for getting the name.

\$\endgroup\$
  • \$\begingroup\$ Something along the lines of this I presume: private String suit; private Suit (String suit){ this.suit = suit; } public String getSuit() { return suit; } \$\endgroup\$ – user69928 Apr 26 '15 at 1:27
  • \$\begingroup\$ @n0tion Precisely! \$\endgroup\$ – SirPython Apr 26 '15 at 1:29
5
\$\begingroup\$

I have some small suggestions that will not change any thing special about your code, but could help the readability of your code.

    if(isShuffled == false)
        this.deck = new Card[52];
    else{
        this.deck = new Card[52];
        shuffle();
    }

Mixing using brackets and not using brackets is not something I find elegant. I would suggest you to always use brackets. It saves adding them when your one-liner needs more lines of code. It also prevents some bugs from happening because you forget to add them when you add that line of code to fix things quickly. Not a deal breaker, just something nice to do in Java.


Comments need to be useful. They need to add something that the code can't provide by itself :

    /** Shuffle deck */
    public void shuffle() {

When it's Javadoc, it's more important than a normal comment. What did your comment provide that reading the definition of the method did not ? Nothing. Just reading the name of the method is sufficient. So, why do you have it?

Your Javadoc should provide information useful for the client of your class. I don't necessarily to read the implementation of your code to know what it do. In case of your shuffle, you could briefly explain the method you use, or any pitfall of your method.

Quick note too, the format of the Javadoc should be like this :

    /**
     * Shuffle deck 
     */
    public void shuffle() {

You have one magic number in there : 52, which is your typical deck size. You could define a private final static int DECK_SIZE = 52 to easily access the number and quickly change it if need be. Keep in mind too that not all games required 52 cards to play. So if you want to reuse your code, it would be one step away from making it a variable and have a Deck that could be reuse for other games.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the input @Marc-Andre. I'll make the appropriate corrections. Should I simply remove the current comments and write the Javadoc once I've completed the program and have a better idea of how everything will fit together? \$\endgroup\$ – user69928 Apr 26 '15 at 2:07
  • \$\begingroup\$ This is a personal opinion, but at the moment your javadoc seems useless. Most of the time, your code is good and well named in terms of variable that your comment only repeat information. \$\endgroup\$ – Marc-Andre Apr 26 '15 at 2:12

Your Answer

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