6
\$\begingroup\$

Can someone please review this design I created for deck of cards in Java?

package prep.design;

import java.lang.reflect.Array;
import java.util.*;

/**
 * Created by rohandalvi on 5/14/16.
 */
public class CardDeck {

    private ArrayList<Card> cards;

    CardDeck(){
        cards = new ArrayList<>();
        for(Card.Suit s: Card.Suit.values()){
            for(Card.Rank r: Card.Rank.values()){
                cards.add(new Card(r,s));
            }
        }
    }

    public void print(){
        for(Card card: cards){
            System.out.println("Card: "+card.getRank()+" Suit: "+card.getSuit());
        }
    }

    public void shuffle(ArrayList<Card> c){
        if(c==null){
            Collections.shuffle(cards);
        }else{
            Collections.shuffle(c);
        }

    }


    public static void main(String[] args) {
        CardDeck cd = new CardDeck();
        cd.shuffle(null);
        //cd.print();

        cd.deal(5);
    }

    public void deal(int numberOfPlayers){
        ArrayList<Card> tempDeck = new ArrayList<>(cards);
        Map<Integer,List<Card>> playerDeck = new HashMap<>();
        //List<List<Card>> playerDeck = new ArrayList<>();
        shuffle(tempDeck);
        shuffle(tempDeck);


        int i=0;
        while(i!=52){
            int j = i%numberOfPlayers;
            List<Card> tempList;
            if(playerDeck.containsKey(j)){
                tempList = playerDeck.get(j);
            }else{
                tempList = new ArrayList<>();
            }
            tempList.add(tempDeck.get(i));
            playerDeck.put(j,tempList);

            i++;

        }

        System.out.println("Dealt");

        displayPlayerCards(playerDeck);

    }

    public void displayPlayerCards(Map<Integer,List<Card>> playerDeck){
        int i=0;
        for(Integer player: playerDeck.keySet()){
            List<Card> playerCards = playerDeck.get(player);

            System.out.println("Player "+i);
            for(Card c: playerCards){
                System.out.print("Rank: "+c.getRank()+" Suit: "+c.getSuit()+"\t");
            }
            System.out.println();
            i++;
        }
    }
}

class  Card{
    Suit s;
    Rank r;
    public  enum  Suit{
        Spade , Heart , Diamond , Clubs
    }

    public enum Rank{
        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);

        int priority;
        Rank(int s) {
            priority = s;
        }

        public int getPriority(){
            return priority;
        }

        public Rank getRankByPriority(int p){
            switch (p){
                case 1: return Rank.ACE;
                case 2: return Rank.TWO;
                case 3: return Rank.THREE;
                case 4: return Rank.FOUR;
                case 5: return Rank.FIVE;
                case 6: return Rank.SIX;
                case 7: return Rank.SEVEN;
                case 8: return Rank.EIGHT;
                case 9: return Rank.NINE;
                case 10: return Rank.TEN;
                case 11: return Rank.JACK;
                case 12: return Rank.QUEEN;
                case 13: return Rank.KING;

                default: return null;
            }

        }
    }

    Rank getRank(){
        return r;
    }

    Suit getSuit(){
        return s;
    }


    Card(Rank r, Suit s){
        this.r = r;
        this.s = s;
    }

}

class cardComparator implements java.util.Comparator<Card.Rank>{

    @Override
    public int compare(Card.Rank o1, Card.Rank o2) {
        if(o1.getPriority() > o2.getPriority()){
            return 1;
        }else if(o1.getPriority() < o2.getPriority()){
            return -1;
        }else{
            return 0;
        }
    }

    @Override
    public Comparator<Card.Rank> reversed() {
        return null;
    }
}
Improve this question
\$\endgroup\$
1
  • \$\begingroup\$ Personally I'd represent the cards as ints unless there was some sdpoecific reason to make tghem objects... \$\endgroup\$
    – keshlam
    May 16, 2016 at 2:32

4 Answers 4

Reset to default
11
\$\begingroup\$

Card class

Specify the access modifiers of the class and members properly. Use proper names for members:

class  Card{
    Suit s;
    Rank r;

for example:

public class  Card{
    private Suit suit;
    private Rank rank;

print() method

It's better to only create a String representation of an object and return that. You override toString() method which will be used implicitly if the object has to be converted to a String.

It's good practice to override toString() for all custom classes, because this is where people will look for when searching for a String representation of the object. You called it print(), or was it info()? , who knows? Your future self? Probably not. toString() is the way to go.

Your current approach appears to be more convenient, but actually causes some trouble like duplicated code:

System.out.print("Rank: "+c.getRank()+" Suit: "+c.getSuit()+"\t");
System.out.println("Card: "+card.getRank()+" Suit: "+card.getSuit());

Card.toString() could look like this:

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

CardDeck is pretty much a collection of a number of cards. CardDeck.toString() could look something like this:

@Override
public String toString()
{
    String result = cards.size() + " cards:" + System.lineSeparator();
    for (Card card : cards)
    {
        result = result.concat(card + System.lineSeparator());
    }

    return result;
}

Here's what both classes look like so var with a little example program:

Card.java

public class  Card
{
    private Suit suit;
    private Rank rank;

    public  enum  Suit
    {
        Spade , Heart , Diamond , Clubs
    }

    public enum Rank
    {
        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);

        int priority;

        Rank(int s) 
        {
            priority = s;
        }

        public int getPriority()
        {
            return priority;
        }

        public Rank getRankByPriority(int p)
        {
            switch (p)
            {
                case 1: return Rank.ACE;
                case 2: return Rank.TWO;
                case 3: return Rank.THREE;
                case 4: return Rank.FOUR;
                case 5: return Rank.FIVE;
                case 6: return Rank.SIX;
                case 7: return Rank.SEVEN;
                case 8: return Rank.EIGHT;
                case 9: return Rank.NINE;
                case 10: return Rank.TEN;
                case 11: return Rank.JACK;
                case 12: return Rank.QUEEN;
                case 13: return Rank.KING;

                default: return null;
            }
        }
    }

    Card(Rank rank, Suit suit)
    {
        this.rank = rank;
        this.suit = suit;
    }

    Rank getRank()
    {
        return rank;
    }

    Suit getSuit()
    {
        return suit;
    }

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

CardDeck.java

import java.util.ArrayList;

public class CardDeck 
{
    private ArrayList<Card> cards;

    CardDeck()
    {
        cards = new ArrayList<>();

        for(Card.Suit s: Card.Suit.values())
        {
            for(Card.Rank r: Card.Rank.values())
            {
                cards.add(new Card(r,s));
            }
        }
    }    

    public static void main(String[] args)
    {
        CardDeck deck = new CardDeck();

        System.out.println(deck);
    }

    @Override
    public String toString()
    {
        String result = cards.size() + " cards:" + System.lineSeparator();
        for (Card card : cards)
        {
            result = result.concat(card + System.lineSeparator());
        }

        return result;
    }
}

result (excerpt):

$ java CardDeck 
52 cards:
rank: ACE    suit: Spade
rank: TWO    suit: Spade
rank: THREE  suit: Spade
rank: FOUR   suit: Spade
rank: FIVE   suit: Spade
rank: SIX    suit: Spade
rank: SEVEN  suit: Spade
rank: EIGHT  suit: Spade
rank: NINE   suit: Spade
rank: TEN    suit: Spade
...

shuffle() method

A method is a block of code that operates on an object. Your shuffle() method might work on an optional parameter which is entirely independent from the CardDeck object. The part of the method that operates on an arbitrary ArrayList<Card> should be static.

public void shuffle(ArrayList<Card> c){
        if(c==null){
            Collections.shuffle(cards);
        }else{
            Collections.shuffle(c);
        }

    }

Thus splits into:

public static void shuffle(ArrayList<Card> cards)
{
    if(cards!=null)
    {
        Collections.shuffle(cards);
    }
}

public void shuffle()
{
    Collections.shuffle(cards);
}

The second one makes a lot of sense, but the static version does not. What's the point of calling this method directly when you already have Collections.shuffle()?

Also, the static version has nothing to do with the CardDeck class whatsoever. It should not be a member of CardDeck, but either Card or better a separate class with utility functionality named CardUtils for example. But again, there's no real point in having that functionality that does not add anything to Collections.shuffle(). Delete it.

deal() method

I have no idea what this is doing by only looking at it. There are so many card games and all have their own ways to deal cards. Does every player get the same number of cards? Or are all cards distributed to a number of players?

Trying your code it looks like the later is true.

The problem with this method is that it does not return anything. It only prints the piles of card out, without any chance to do anything with them. Instead, provide a useful return value you can work with.

One such return value could be CardDeck. There's a bit of functionality already in there, so why not reuse that? Add a constructor to be able to pass a Collection of cards to it:

CardDeck(Collection<? extends Card> cards)
{
    this.cards = new ArrayList<Card>(cards);
}

When dealing the cards, there's no real point in dealing them one by one. You know how many cards each player receives and should just get that many from the shuffled cards. Think about it like every player takes a certain number of cards from the deck.

public ArrayList<CardDeck> dealAllCards(int numberOfPlayers)
{
    ArrayList<CardDeck> playerHands = new ArrayList<>(numberOfPlayers);
    ArrayList<Card> shuffledDeck = new ArrayList<>(cards);

    int cardsPerPlayer = shuffledDeck.size()/numberOfPlayers; // to be subtracted by 1 after dealing remaining cards
    int remainingCards = shuffledDeck.size()%numberOfPlayers;

    Collections.shuffle(shuffledDeck);

    for (int player = 0; player < numberOfPlayers; ++player)
    {
        if (player == remainingCards) 
        {
            --cardsPerPlayer; // all remaining cards dealed
        }           

        CardDeck hand = new CardDeck(shuffledDeck.subList(0, cardsPerPlayer + 1));
        shuffledDeck.removeAll(hand.cards);

        playerHands.add(hand);
    }

    return playerHands;
}

The full code now looks like this:

Card.java did not change, see above

CardDeck.java

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;

public class CardDeck 
{
    private ArrayList<Card> cards;

    public static void main(String[] args) 
    {
        CardDeck deck = new CardDeck();

        ArrayList<CardDeck> playerHands = deck.dealAllCards(5);

        for(CardDeck playerHand : playerHands)
        {
            System.out.println(playerHand);
        }
    }

    CardDeck()
    {
        cards = new ArrayList<>();

        for(Card.Suit s: Card.Suit.values())
        {
            for(Card.Rank r: Card.Rank.values())
            {
                cards.add(new Card(r,s));
            }
        }
    }

    CardDeck(Collection<? extends Card> cards)
    {
        this.cards = new ArrayList<Card>(cards);
    }

    public ArrayList<CardDeck> dealAllCards(int numberOfPlayers)
    {
        ArrayList<CardDeck> playerHands = new ArrayList<>(numberOfPlayers);
        ArrayList<Card> shuffledDeck = new ArrayList<>(cards);

        int cardsPerPlayer = shuffledDeck.size()/numberOfPlayers; // to be subtracted by 1 after dealing remaining cards
        int remainingCards = shuffledDeck.size()%numberOfPlayers;

        Collections.shuffle(shuffledDeck);

        for (int player = 0; player < numberOfPlayers; ++player)
        {
            if (player == remainingCards) 
            {
                --cardsPerPlayer; // all remaining cards dealed
            }           

            CardDeck hand = new CardDeck(shuffledDeck.subList(0, cardsPerPlayer + 1));
            shuffledDeck.removeAll(hand.cards);

            playerHands.add(hand);
        }

        return playerHands;
    }

    public void shuffle()
    {
        Collections.shuffle(cards);
    }    

    @Override
    public String toString()
    {
        String result = cards.size() + " cards:" + System.lineSeparator();
        for (Card card : cards)
        {
            result = result.concat(card + System.lineSeparator());
        }

        return result;
    }
}

full example output:

$ java CardDeck 
11 cards:
rank: FOUR   suit: Clubs
rank: FIVE   suit: Heart
rank: FOUR   suit: Spade
rank: ACE    suit: Heart
rank: EIGHT  suit: Heart
rank: SIX    suit: Heart
rank: JACK   suit: Spade
rank: EIGHT  suit: Diamond
rank: JACK   suit: Clubs
rank: QUEEN  suit: Clubs
rank: TWO    suit: Spade

11 cards:
rank: SEVEN  suit: Heart
rank: NINE   suit: Spade
rank: SEVEN  suit: Diamond
rank: SEVEN  suit: Clubs
rank: SIX    suit: Clubs
rank: ACE    suit: Clubs
rank: KING   suit: Heart
rank: SEVEN  suit: Spade
rank: FIVE   suit: Clubs
rank: FOUR   suit: Diamond
rank: TEN    suit: Diamond

10 cards:
rank: KING   suit: Clubs
rank: TEN    suit: Clubs
rank: ACE    suit: Diamond
rank: QUEEN  suit: Spade
rank: SIX    suit: Diamond
rank: FIVE   suit: Spade
rank: EIGHT  suit: Spade
rank: QUEEN  suit: Heart
rank: FOUR   suit: Heart
rank: KING   suit: Diamond

10 cards:
rank: THREE  suit: Clubs
rank: EIGHT  suit: Clubs
rank: THREE  suit: Diamond
rank: KING   suit: Spade
rank: TWO    suit: Clubs
rank: JACK   suit: Heart
rank: THREE  suit: Spade
rank: THREE  suit: Heart
rank: JACK   suit: Diamond
rank: TWO    suit: Diamond

10 cards:
rank: NINE   suit: Heart
rank: NINE   suit: Clubs
rank: TEN    suit: Heart
rank: ACE    suit: Spade
rank: NINE   suit: Diamond
rank: TEN    suit: Spade
rank: FIVE   suit: Diamond
rank: QUEEN  suit: Diamond
rank: TWO    suit: Heart
rank: SIX    suit: Spade

further improvements from the comments

Thanks @Boris!

String.concat is conventionally written as +; this is never used in a loop - a StringBuilder (or StringWriter) is used instead.

That's right. CardDeck.toString() could look like:

@Override
public String toString()
{
    StringBuilder result = new StringBuilder(cards.size() + " cards:" + System.lineSeparator());
    for (Card card : cards)
    {
        result.append(card + System.lineSeparator());
    }

    return result.toString();
}

This is great, because it also helps readability. It's not just some String, but a StringBuilder, which is a bit more self-documenting.

Given that cards is immutable (no public acces and not private changes), this String could even be built beforehand and cached in a private member. As I don't think the toString() method will be called in such quantities that it would make a significant difference, I leave such and further optimisations of the method to the interested reader.

Your deal method is very inefficient; List.removeAll will call remove on each of the elements, which is in turn going to loop through the List to find the element; this is O(n*k) - very bad. Instead use List.subList.clear() after you create the CardDeck for the player.

True, I was more concerned about being close to the real world situation that the code is modelling, that is: tacking a number of cards from a set of cards.

How to solve that problem? Not at all. They say it's clever to solve a problem, but wise to avoid it. Let's simply not remove anything at all.

Instead, move the parameters for subList(), which essentially has the same effect. The improved version of dealAllCards() could look like this:

public ArrayList<CardDeck> dealAllCards(int numberOfPlayers)
{
    ArrayList<CardDeck> playerHands = new ArrayList<>(numberOfPlayers);
    ArrayList<Card> shuffledDeck = new ArrayList<>(cards);

    int cardsPerPlayer = shuffledDeck.size()/numberOfPlayers; // to be subtracted by 1 after dealing remaining cards
    int remainingCards = shuffledDeck.size()%numberOfPlayers; // #cards is not a multiple of #players

    Collections.shuffle(shuffledDeck);

    int firstCardIndex = 0;
    int lastCardIndex = 0;

    for (int player = 0; player < numberOfPlayers; ++player)
    {
        if (player == remainingCards) 
        {
            --cardsPerPlayer; // all remaining cards dealed
        }   

        lastCardIndex = firstCardIndex + cardsPerPlayer + 1;    

        playerHands.add(new CardDeck(shuffledDeck.subList(firstCardIndex, lastCardIndex)));

        firstCardIndex = lastCardIndex;
    }

    return playerHands;
}

shuffledDeck is just a local temporary helper to hold the shuffled cards, after all.

Improve this answer
\$\endgroup\$
2
  • \$\begingroup\$ Two comments - 1) String.concat is conventionally written as +; this is never used in a loop - a StringBuilder (or StringWriter) is used instead. You have a Shlemiel the painter problem. 2) Your deal method is very inefficient; List.removeAll will call remove on each of the elements, which is in turn going to loop through the List to find the element; this is O(n*k) - very bad. Instead use List.subList.clear() after you create the CardDeck for the player. \$\endgroup\$
    – Boris the Spider
    May 16, 2016 at 7:19
  • \$\begingroup\$ Thank you @BoristheSpider, given the (to me surprising) popularity of this answer I'm concerned about suggesting code that incorporates bad practices. Your comment is well appreciated. I edited my answer to include some strategies I came up with to (hopefully) solve the issues you pointed out. \$\endgroup\$
    – I'll add comments tomorrow
    May 16, 2016 at 11:53
3
\$\begingroup\$

Consider Enum ordinals

Your Card.Rank enum has a field which you can remove by referring to its ordinal using the ordinal() method. Ordinals are just position in the enum declaration, like arrays they are 0 based. So something you can do is take this chunk:

        int priority;
        Rank(int s) {
            priority = s;
        }

        public int getPriority(){
            return priority;
        }

And replace it with:

public int getPriority() {
        return ordinal() + 1;
}

and, of course, you wouldn't have to explicitly set anything in the constructor.

I second the way @user3495816 proposed to refactor your getRankByPriority method. (I had this section myself but he added it before this post, so this reflects that).

On Clarity

While on the topic of your enumerations, I'd declare each value in its own line. It's neater, easier to discern the values, and eliminates the horizontal scrolling required to see everything.

In addition to this, I'd eliminate all single letter variables where used. Use meaningful names where you can. You don't really gain much by referring to a suit as just s, but you do lose a lot of meaning to an onlooker, which could be you in the near future.

Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ Generally it is not recommended to use ordinals for this as it makes the code a little brittle - the functionality of the code is dependant on the order of declaration. A most interesting point is that different games have differed priorities (Ace high? low?) so should that be part of the enum at all? \$\endgroup\$
    – Boris the Spider
    May 16, 2016 at 7:20
3
\$\begingroup\$

I have a few suggestions:

  1. Since field cards in the CardDeck class is immutable, I would declare is as final, same with the suit and rank fields in the Card class.

  2. I would rewrite method getRankByPriority(int p) as follows:

    public Rank getRankByPriority(int p){
    for(Rank rank : values()) {
        if (p == rank.getPriority()) {
            return rank;
        }
    }
    return null;
}

    Also, don't you want to declare this method as static?

  3. I would suggest changing the compare method in cardComparator:

    @Override
public int compare(Card.Rank o1, Card.Rank o2) {
    // possibly null checks
    return o1.getPriority().compareTo(o2.getPriority());
}

  4. You also could avoid output formatting in the print method and instead either override toString() in a Card class or create a new method in a Card class that would create a String representation of the object.

Improve this answer
\$\endgroup\$
2
  • \$\begingroup\$ Depending on how often it it used, the OP's version of getRankByPriority is better. It uses O(n) memory but runs in O(1), whereas your method uses O(1) memory but runs in O(n). So the OPs method is (likely) faster, and that will begin to add up in tight loops. \$\endgroup\$
    – Boris the Spider
    May 16, 2016 at 7:22
  • \$\begingroup\$ @BoristheSpider I do not agree with you at all. You are right that OPs version is faster but that is hardly a goal while iterating over tens of items. I would say that my version is better because if someone else would add item to enum and forget to to change getRankByPriority program would break. Also your suggestion is the case of premature optimalization. We do not know how JVM would run both codes and only way to find out is to test them. \$\endgroup\$
    – user3495816
    May 16, 2016 at 11:26
2
\$\begingroup\$

Inside or outside?

public void shuffle(ArrayList<Card> c){
    if(c==null){
        Collections.shuffle(cards);
    }else{
        Collections.shuffle(c);
    }
}

The function shuffle may modify either cards that is a property inside the class, or c a user supplied argument.

It decides that based on wheter the input is null or not.

Shuffling an outside collection is already given by the stdlib as Collections.shuffle so the non-null behaviour is just duplication. In short I would keep only the first branch while removing the conditional.

Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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