Bridge Card Game

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

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.

StackExchange Code Review Q#87992, answer score: 13