Console Card Game - Contract Bridge

Question

My project has three classes: Card, Deck, and BridgeConsole. The Card class has two fields: int rank and int suite. It has three methods: toString, getSuitAsString, and compareTo:

public class Card implements Comparable<Object>{
int suit; // 0-Hearts, 1-Clubs, 2-Diamonds, 3-Spades
int rank; // 2-10, 11-Jack, 12-Queen, 13-King, 14-Ace
public Card(int rank, int suit) {
 this.rank = rank;
 this.suit = suit;
}
public int getRank() {
 return rank;
}
public int getSuit() {
 return suit;
}
public String getSuitString() {
 String str = null;
 switch(suit){
 case 0: 
 str = "Hearts";
 break;
 case 1: 
 str = "Clubs";
 break;
 case 2:
 str = "Diamonds";
 break;
 case 3:
 str = "Spades";
 break;
 default:
 System.out.print("Fatal error!!!");
 break;
 }
 return str;
} 
@Override
public String toString() {
 return rank + " of " + getSuitString() + ", ";
}
@Override
public int compareTo(Object o) {
 Card c2 = (Card) o;
 if( suit < c2.suit) {
 return -1;
 }
 if( suit > c2.suit) {
 return 1;
 }
 if( rank > c2.rank) {
 return 1;
 }
 if( rank < c2.rank) {
 return -1;
 }
 return 0;
}
public boolean compareTo(int firstSuit, Card c2) {
 return suit == firstSuit && rank > c2.rank;
 }
}

My Deck class constructs the deck of cards and has a method to deal cards to the players:

public class Deck {
 ArrayList<Card> deck = new ArrayList<>();
public Deck() {
 for (int s = 0; s < 4; s++) {
 for (int r = 2; r < 15; r++) {
 deck.add(new Card(r, s));
 }
 }
 Collections.shuffle(deck);
}
public void addCard(Card card) {
 if (deck.size() < 52) {
 deck.add(card);
 }
}
public Card dealCard() {
 Card card = null;
 if (deck.size() > 0) {
 card = deck.remove(deck.size() - 1);
 }
 return card;
 }
}

Finally, the game logic and main method are contained in the BridgeConsole class. I used modulo arithmetic to keep track of the previous rounds winner so as to start with that player in the subsequent round.

public class BridgeConsole {
static Scanner stdin = new Scanner(System.in);
static Deck deck = new Deck();
static int playerCard;
static ArrayList<Card> player = new ArrayList<>();
static ArrayList<Card> ai1 = new ArrayList<>();
static ArrayList<Card> ai2 = new ArrayList<>();
static ArrayList<Card> ai3 = new ArrayList<>();
static ArrayList<Card> center = new ArrayList<>();
static int winner = 0;
static int team1 = 0;
static int team2 = 0;
static String whoWon = "";
public static int getPlyerCard() {
 do {
 System.out.print("\n player hands contains " + player.size() + " cards."
 + "\n Choose card [index]");
 playerCard = stdin.nextInt();
 while (playerCard > player.size()) {
 System.out.print("Please enter a valid card index. [0-13]");
 playerCard = stdin.nextInt();
 }
 } while (playerCard > player.size());
 return playerCard;
}
public static void trackScore(ArrayList<Card> p_1, ArrayList<Card> p_2, ArrayList<Card> p_3, ArrayList<Card> p_4, ArrayList<Card> center, int winner) {
 switch (winner) {
 case 0:
 team1++;
 break;
 case 1:
 team2++;
 break;
 case 2:
 team1++;
 break;
 case 3:
 team2++;
 break;
 }
}
public static void playRound() {
 System.out.println("PLAYER Hand:");
 int index = 0;
 for (Card c : player) {
 System.out.print("[" + index + "] " + c.toString());
 index++;
 }
 System.out.println("\n AI 1 hand");
 displayCards(ai1);
 System.out.println("\n AI 2 hand");
 displayCards(ai2);
 System.out.println("\n AI 3 hand");
 displayCards(ai3);
 switch (winner) {
 case 0:
 playerCard = getPlyerCard();
 center.add(player.get(playerCard));
 center.add(aiSelect(center.get(0), ai1));
 center.add(aiSelect(center.get(0), ai2));
 center.add(aiSelect(center.get(0), ai3));
 ai1.remove(aiSelect(center.get(0), ai1));
 ai2.remove(aiSelect(center.get(0), ai2));
 ai3.remove(aiSelect(center.get(0), ai3));
 player.remove(playerCard);
 break;
 case 1:
 center.add(aiSelect(ai1));
 center.add(aiSelect(center.get(0), ai2));
 center.add(aiSelect(center.get(0), ai3));
 System.out.print("\nCenter row of cards\n");
 displayCards(center);
 playerCard = getPlyerCard();
 center.add(player.get(playerCard));
 ai1.remove(aiSelect(ai1));
 ai2.remove(aiSelect(center.get(0), ai2));
 ai3.remove(aiSelect(center.get(0), ai3));
 player.remove(playerCard);
 break;
 case 2:
 center.add(aiSelect(ai2));
 center.add(aiSelect(center.get(0), ai3));
 System.out.print("\nCenter row of cards\n");
 displayCards(center);
 playerCard = getPlyerCard();
 center.add(player.get(playerCard));
 center.add(aiSelect(center.get(0), ai1));
 ai2.remove(aiSelect(ai2));
 ai3.remove(aiSelect(center.get(0), ai3));
 player.remove(playerCard);
 ai1.remove(aiSelect(center.get(0), ai1));
 break;
 case 3:
 center.add(aiSelect(ai3));
 System.out.print("\nCenter row of cards\n");
 displayCards(center);
 playerCard = getPlyerCard();
 center.add(player.get(playerCard));
 center.add(aiSelect(center.get(0), ai1));
 center.add(aiSelect(center.get(0), ai2));
 ai3.remove(aiSelect(ai3));
 player.remove(playerCard);
 ai1.remove(aiSelect(center.get(0), ai1));
 ai2.remove(aiSelect(center.get(0), ai2));
 break;
 default:
 break;
 }
 System.out.print("\nCenter row of cards\n");
 displayCards(center);
 System.out.print("\n Winning Card \n");
 System.out.println(selectMax(center.get(0), center.get(1), center.get(2), center.get(3)).toString());
 System.out.print("\n Player who won round \n");
 winner = (winner + getWinningCard(center)) % 4;
 System.out.println("At position [" + getPlayerPosition(center.get(getWinningCard(center)), center) + "]");
 trackScore(player, ai2, ai3, ai1, center, winner);
 center.clear();
}
public static int getPlayerPosition(Card playerCard, ArrayList<Card> center) {
 int index = 0;
 for (int i = 0; i < center.size(); i++) {
 if (playerCard == center.get(i)) {
 index = i;
 }
 }
 return index;
}
public static int getWinningCard(ArrayList<Card> center) {
 Card maxCard = selectMax(center.get(0), center.get(1), center.get(2), center.get(3));
 return center.indexOf(maxCard);
}
public static Card aiSelect(Card current, ArrayList<Card> aiHand) {
 Collections.sort(aiHand);
 // var max will track highest card in AIs hand
 Card max;
 // contains all posible cards of the same sui as the current card
 ArrayList<Card> followSuit = new ArrayList<>();
 for (Card c : aiHand) {
 if (c.suit == current.suit) {
 followSuit.add(c);
 }
 }
 if (!followSuit.isEmpty()) {
 Collections.sort(aiHand);
 max = followSuit.get(followSuit.size() - 1);
 return max;
 } else {
 return aiHand.get(aiHand.size() - 1);
 }
}
public static Card aiSelect(ArrayList<Card> aiHand) {
 Collections.sort(aiHand);
 // var max will track highest card in AIs hand
 return aiHand.get(aiHand.size() - 1);
}
public static void displayCards(ArrayList<Card> hand) {
 for (Card c : hand) {
 System.out.print(c.toString());
 }
}
public static void dealCards(Deck deck, ArrayList<Card> p1, ArrayList<Card> p2,
 ArrayList<Card> p3, ArrayList<Card> p4) {
 for (int i = 0; i < 13; i++) {
 p1.add(deck.dealCard());
 p2.add(deck.dealCard());
 p3.add(deck.dealCard());
 p4.add(deck.dealCard());
 }
}
public static Card selectMax(Card c1, Card c2, Card c3, Card c4) {
 // suit of card 1
 int firsSuit = c1.suit;
 if (c1.compareTo(firsSuit, c2) && c1.compareTo(firsSuit, c3)
 && c1.compareTo(firsSuit, c4)) {
 return c1;
 } else if (c2.compareTo(firsSuit, c1) && c2.compareTo(firsSuit, c3)
 && c2.compareTo(firsSuit, c4)) {
 return c2;
 } else if (c3.compareTo(firsSuit, c1) && c3.compareTo(firsSuit, c2)
 && c3.compareTo(firsSuit, c4)) {
 return c3;
 } else {
 return c4;
 }
}
public static void main(String[] args) {
 System.out.println("Contract Bridge - Console");
 System.out.println("Creating deck...");
 System.out.println("Dealing Cards...");
 dealCards(deck, player, ai1, ai2, ai3);
 while (!player.isEmpty()) {
 playRound();
 }
 System.out.print("Team1: " + team1 + " team2: " + team2);
 stdin.close();
 System.exit(0);
 }
}

I would appreciate any feedback on design, programming style, etc.

Answer 1

Everywhere you are using ArrayList<Card> in variable and method parameter declarations. It's a recommended practice to use the interface type whenever possible, in this case List<Card>. So for example instead of:

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

Write as:

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

---

In the BridgeConsole class everything is static. This can make unit testing hard, and it's quite unusual. It would be better to change everything to non-static, as much as possible. You might want to separate into two classes: one which manages a game state, and one which just starts running the whole thing (with the main method).

---

For modeling suits in card games, a more common approach is to use an enum.

enum Suit {
 Hearts,
 Clubs,
 Diamonds,
 Spades
}

---

In a switch statement, when multiple cases have the same body, you can group them together. So instead of this:

switch (winner) {
 case 0:
 team1++;
 break;
 case 1:
 team2++;
 break;
 case 2:
 team1++;
 break;
 case 3:
 team2++;
 break;
}

You could simplify to:

switch (winner) {
 case 0:
 case 2:
 team1++;
 break;
 case 1:
 case 3:
 team2++;
 break;
}

• user69928: What's the reasoning behind using the interface type?
• janos: It gives you the flexibility to change between implementations. If a method takes a List parameter, it will work with an ArrayList or LinkedList too. Another related good practice is thinking in terms of interfaces instead of implementations. If you're interested in more, I recommend "Item 52: Refer to objects by their interfaces" in Effective Java by Joshua Bloch
• user69928: When you say everything is static, I assume you mean both the variables and methods, correct? Furthermore, I've read that global variable are "evil" and should be avoided, does this mean I should declare and initialzie all my variables within the main method and not at the top of the class. I'm also not clear on what the game state implies, should I move all the methods the concern themselves with the game logic and round to their own class and just have one main class with the main method and only pass an instance of round which will have access internally to the other decision making methods

Answer 2

1. suit and rank fields should be declared private, no need to share things and deal with infinite number of states.

   private final int suit; 
   private final int rank; 
   public Card(int rank, int suit) {
    this.rank = rank;
    this.suit = suit;
   }
   

2. If suit only allows 0,1,2,3 values, why don't you validate it upon construction?

   public Card(int rank, int suit) {
    if(!isValidSuit(suit){
    throw new IllegalArgumentException("Invalid value for suit");
    }
    this.rank = rank;
    this.suit = suit;
   }
   

   And now you are sure that suit always has a valid value and there is no need for the error msg in the getSuitString method.

3. Your compareTo in the Card class is not type-safe, it will throw an IllegalCastExcpetion if the passed parameter is not a Card, You can instead implement Comparable<Card>

   class Card implements Comparable<Card>{
   @Override
   public int compareTo(Card o) {
   ....
   }
   }
   

4. Try using interfaces instead of concrete types.

   getWinningCard(List<Card> center)
   

5. Avoid magic numbers like 4,15,52 in your code and extract them into constants and give them some useful names.

6. implement equals and hashCode methods and avoid reference equality.

    // compares references rather than equality 
   if (playerCard == center.get(i)) {
    index = i;
   }
   

7. System.exit(0); at the end of the program is useless and you better delete it

• user69928: 2. If I know the only moment where I'm passing values to the card constructor is within the deck constructor, is there any sense in validating the suit?

Answer 3

public boolean compareTo(int firstSuit, Card c2) {
 return suit == firstSuit && rank > c2.rank;
 }
}

Everybody knows that every compareTo compares this to the argument and returns an int. Every compareTo except this one. It's too bad when you run out of names, but even _000 would be better.

AFAIK it's about making tricks and you need something like class TrickEvaluator with a member

@Nullable private final Suit trumphSuit;

and methods like

boolean takes(Card oldCard, Card newCard) {
 if (oldCard.getSuite() == newCard.getSuite()) {
 return newCard.getRank() > oldCard.getRank();
 } else {
 return newCard.getSuite() == trumphSuit;
 }
}
int trickWinner(List<Card> playedCards) {
 checkArgument(0 < playedCards.size() && playedCards.size() <= 4);
 Card winningCard = playedCards.get(0);
 for (Card c : playedCards) {
 if (takes(winningCard, c)) {
 winningCard = c;
 }
 }
 return playedCards.indexOf(winningCard);
}

---

public void addCard(Card card) {

This should be private as cards get never actually added after construction.

public Card dealCard() {

You don't need it. Everyone gets 13 cards at once, so there's no point in dealing one card after each other. It may be a helper method, but then it should be private.

Actually, I'd make the Deck immutable and add a method getHand(int).

---

public class BridgeConsole {

This class is much too long. You should extract everything not dealing directly with the UI into other classes.

static ArrayList<Card> player = new ArrayList<>();
static ArrayList<Card> ai1 = new ArrayList<>();
static ArrayList<Card> ai2 = new ArrayList<>();
static ArrayList<Card> ai3 = new ArrayList<>();

If you use a list of lists here, you can save quite some code with indexing modulo 4. You also gain some flexibility for later as you can more easily use more players (instead of AI). ANd you get rid of that terrible switch.

Answer 4

This might make sense for Bridge, but won't for almost any other card game.

Card c2 = (Card) o;
if( suit < c2.suit) {
 return -1;
}
if( suit > c2.suit) {
 return 1;
}
if( rank > c2.rank) {
 return 1;
}
if( rank < c2.rank) {
 return -1;
}
return 0;

I think it's okay to compare by rank, but in reality (and although it's against intuition), the game should be responsible for determining the relative values of the cards. In very few card games does the suit matter when comparing the value of two cards. Also consider that in some games an Ace is 1, in others, 11. Moving this responsibility elsewhere makes your cards much more reusable in other situations.

• user69928: was overloading the compareTo so I could work with boolean values as opposed to integer values. Though I need the one that everyone knows and uses to sort (Collections.sort()). Is there something wrong with overloading the compareTo?
• RubberDuck: I'm tackling this from the angle that you may want to reuse your Card class in a Blackjack game (or any other game, really). By having your cards able to compare themselves, they're brittle. You can't just plug them into any game and have it just work. Think about the real world game. Do the cards value themselves, or are they given a value by the game rules? I would implement a custom comparator and have the game rank the cards.