Deck of cards design 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&lt;Card&gt; cards;

    CardDeck(){
        cards = new ArrayList&lt;&gt;();
        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&lt;Card&gt; 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&lt;Card&gt; tempDeck = new ArrayList&lt;&gt;(cards);
        Map&lt;Integer,List&lt;Card&gt;&gt; playerDeck = new HashMap&lt;&gt;();
        //List&lt;List&lt;Card&gt;&gt; playerDeck = new ArrayList&lt;&gt;();
        shuffle(tempDeck);
        shuffle(tempDeck);


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

            i++;

        }

        System.out.println("Dealt");

        displayPlayerCards(playerDeck);

    }

    public void displayPlayerCards(Map&lt;Integer,List&lt;Card&gt;&gt; playerDeck){
        int i=0;
        for(Integer player: playerDeck.keySet()){
            List&lt;Card&gt; 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&lt;Card.Rank&gt;{

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

    @Override
    public Comparator&lt;Card.Rank&gt; reversed() {
        return null;
    }
}
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&lt;Card&gt; cards;

    CardDeck()
    {
        cards = new ArrayList&lt;&gt;();

        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&lt;Card&gt; should be static.

public void shuffle(ArrayList&lt;Card&gt; c){
        if(c==null){
            Collections.shuffle(cards);
        }else{
            Collections.shuffle(c);
        }

    }


Thus splits into:

public static void shuffle(ArrayList&lt;Card&gt; 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&lt;? extends Card&gt; cards)
{
    this.cards = new ArrayList&lt;Card&gt;(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&lt;CardDeck&gt; dealAllCards(int numberOfPlayers)
{
    ArrayList&lt;CardDeck&gt; playerHands = new ArrayList&lt;&gt;(numberOfPlayers);
    ArrayList&lt;Card&gt; shuffledDeck = new ArrayList&lt;&gt;(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 &lt; 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&lt;Card&gt; cards;

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

        ArrayList&lt;CardDeck&gt; playerHands = deck.dealAllCards(5);

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

    CardDeck()
    {
        cards = new ArrayList&lt;&gt;();

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

    CardDeck(Collection&lt;? extends Card&gt; cards)
    {
        this.cards = new ArrayList&lt;Card&gt;(cards);
    }

    public ArrayList&lt;CardDeck&gt; dealAllCards(int numberOfPlayers)
    {
        ArrayList&lt;CardDeck&gt; playerHands = new ArrayList&lt;&gt;(numberOfPlayers);
        ArrayList&lt;Card&gt; shuffledDeck = new ArrayList&lt;&gt;(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 &lt; 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&lt;CardDeck&gt; dealAllCards(int numberOfPlayers)
{
    ArrayList&lt;CardDeck&gt; playerHands = new ArrayList&lt;&gt;(numberOfPlayers);
    ArrayList&lt;Card&gt; shuffledDeck = new ArrayList&lt;&gt;(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 &lt; 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.