Simple deck of cards

Question

I had recently completed one programming challenge where I've had to design a deck of cards. The time was limited but I tried my best. I designed the basic operations and am wondering if I can improve the current structure of class and objects. I'm also looking for pros and cons of my design.

import java.util.Random;

public class DeckOfCard {
    /*Collections of cards*/    
    private Card cards[];

    /*Remaining number of cards in the deck*/
    private int currentCount;

    /*Size of the deck which can handle cards*/
    private int size;

    private Suit suits[];

    private Value values[];

    public DeckOfCard(){
        size=52;
        currentCount=0;
        cards = new Card[size];
        suits=Suit.values();
        values=Value.values();

        for(int i =0 ; i<suits.length;i++){
            for(int j =0 ; j<values.length;j++){
                cards[currentCount++] = new Card(suits[i],values[j]);
            }

        }
    }

  /*This is to shuffle the cards in the deck with the remaining cards.
  Variable numberOftime represents the number of time need to shuffle the cards in deck*/

    public void shuffle(int numberOftime){

        Random rand= new Random();


        for(int i=0;i<numberOftime;i++){
            int m=rand.nextInt(currentCount);
            int n=rand.nextInt(currentCount);

            Card temp=cards[m];
            cards[m]=cards[n];
            cards[n]=temp;
        }

    }

/*This function is to deal the cards whatever is on top of the deck.*/
    public void deal(){
        System.out.println(cards[--currentCount]);
    }

/*This is to represents the String representation of the current cards inthe deck.*/ 
    public String toString(){

        StringBuilder sb=new StringBuilder();

        for(int i=0;i < currentCount;i++){
            sb.append(cards[i]);
            sb.append(" ");
        }

        return sb.toString();
    }

/*  Sorting the cards based on the suit and then numbers.
    This sorting is using Bucket Sort to sort the cards runtime= O(n) space=O(n);*/
    public void sort(){

        Card bucketCards[][]= new Card[suits.length][values.length];

        for(int i=0;i<currentCount;i++){
            bucketCards[cards[i].getSuit().ordinal()][cards[i].getValue().ordinal()]=cards[i];
        }

        int pointer=0;

        for(int i=0;i<suits.length;i++){
            for(int j=0;j<values.length;j++){
                if(bucketCards[i][j]!=null)
                    cards[pointer++]=bucketCards[i][j];
            }
        }


    }

    /*This will reset the deck. i.e it will put back all the cards in the deck if it has been dealt.*/
    public void resetdesk(){
        currentCount=52;
    }

/*Printing the stack of cards in format*/ 
    public void printStack(){
        int cardPointer=0;
        for(int i=0;i<suits.length;i++){

            for(int j=0;j<values.length;j++){
                System.out.print(cards[cardPointer++]+" ");
            }

            System.out.println("\n");
        }

    }

    /*Get size of the deck*/
    public int getSize() {
        return size;
    }

/*This represents the suit of the card*/
    private enum Suit{
        CLUB,DIAMOND,SPADE,HEART
    }

/* This represents the number of the card*/
    private enum Value{
        ACE,TWO,THREE,FOUR,FIVE,SIX,SEVEN,EIGHT,NINE,TEN,JACK,QUEEN,KING
    }

/* This class represents the card with specific suit and value. 
 * Cannot change the value once instansiated */
    private class Card{

        private final Suit suit;

        private final Value value;

        public Card(Suit suit, Value value){
             this.suit=suit;
             this.value=value;
        }


        public Suit getSuit() {
            return suit;
        }

        public Value getValue() {
            return value;
        }

        @Override
        public String toString() {
            // TODO Auto-generated method stub
            return suit+"-"+value;
        }
    }
}

Test class

public class Test {

    /**
     * @param args
     * @throws StackOverFlow 
     * @throws StackUnderFlow 
     */
    public static void main(String[] args) {

        int n=10; // Number of cards to deal for test
        DeckOfCard deck=new DeckOfCard();
        deck.printStack();
        deck.shuffle(10);

        System.out.println("----------------------------  After shuffle -----------------------------------------------\n");
            deck.shuffle(30);
            deck.printStack();

        System.out.println("----------------------------  Dealing 10 cards  -----------------------------------------------\n");
            for(int i=0;i<n;i++){
                deck.deal();
            }

        System.out.println("--------------------------    After Sort ---------------------------------------------\n");
            deck.sort();
            deck.printStack();

        System.out.println("----------------------------  Dealing 10 cards  -----------------------------------------------\n");
            for(int i=0;i<n;i++){
                deck.deal();
            }

        System.out.println("----------------------------  Putting back all cards  -----------------------------------------------\n");
            deck.resetdesk();

        System.out.println("--------------------------    After Sort ---------------------------------------------\n");
            deck.sort();
            deck.printStack();

        System.out.println("----------------------------  Dealing 10 cards  -----------------------------------------------\n");
            for(int i=0;i<n;i++){
                deck.deal();
            }
    }
}

Answer 1

I'll give this a quick sweep, but I suspect this can benefit from multiple reviews.

DeckOfCard

• This class could just be named Deck. If you still want to use the longer form, then call it DeckOfCards, which sounds more grammatically-correct.

• You don't need a size member as you can already get the array size via length().

• I don't think you need value and suit members. It looks like you're just gathering them in the constructor in order to populate the deck. If so, then just keep them local to the constructor.

• It may not be necessary to pass such an argument to shuffle(). The user may can just call the method as many times as needed, and at this time, the code could break if the user passes a bad value into your method.

• I don't think deal() should be displaying something. If you give the player an actual Card from the deck, then return that. Otherwise, return a String that can be displayed by the user.

• The comment for resetDeck() is misleading. It won't actually put the cards back, but reset the "pointer" towards the top of the deck.

Card

I cannot see anything noticeably wrong with this class. You keep this pretty basic and don't allow any outside modifications, so well done.

Test

• The class name seems a bit misleading as you're not doing unit-testing via JUnit. Instead, you can simply call this class Main.

• This may just be me, but all those hard-coded dashes look annoying in the source code. If you still think they may be beneficial in the code itself, then you can perhaps create a method that prints any number of dashes.

Answer 2

Since you have posted it under the OOP tag, I will give you some object oriented pointers to think about.

What you have built so far is a prototype and you've done a good job at this. Now, if you want to make this reusable, you will have to refactor this to a proper design.

My first thought is: how do you extend this to include Jokers.

As we know, the Joker is a card with no suit or value (let's call it rank). Therefore I would create a Card as a base class and derive PlayingCard and JokerCard from it. This leaves your design open for extension so that you can later add different cards related to different games (e.g. Top Trumps).

class Card
{
}

class PlayingCard : Card
{
    private Suit suit;
    private Rank rank;
}

class JokerCard : Card
{
}

I would delegate responsibility to another class to provide me with a deck. In essence, this class would be a deck factory and its sole purpose is to provide us with a deck of cards. I would create an interface for this, let's call it IDeckFactory, allowing extensibility, so that different factories can provide us with different decks.

interface IDeckFactory
{
    Card[] GetDeck();
}

class JokerlessDeckFactory : IDeckFactory
{
    public Card[] GetDeck()
    {
        //this is where you put your code you've already written
    }
}

class StandardDeckFactory : IDeckFactory
{
    public Card[] GetDeck()
    {
        //same as before, but remember to add your jokers
    }
}

When it comes to sorting the deck, you would have to provide a different mechanism so that you can sort cards in different orders depending on e.g. which game you're playing. You have already implemented the standard sorting. What about reverse sorting or games where the order differs. E.g. 2 is higher than an A (President card game - aka Élysée). I would provide some kind of predicate or IComparer to allow for this (sorry this is C# language - I'm not sure what the Java equivalent is).

I would also delegate the responsibility of dealing to another class. Its responsibility would be to accept a shuffled deck and return cards one by one (or as many as you request at a time). It seems that it might serve the same purpose as just a simple pile of cards, so let's just call it PileOfCards for now. Its definition would probably look something like this:

class PileOfCards
{
    private List<Card> cards;

    public PileOfCards(List<Card> cards)
    {
        this.cards = cards;
    }

    public Card TakeCard() {}
    public Card[] TakeCards(int amount) {}
    public AddCards(Card[] cards) {}
    public int Count() {}

    //perhaps add shuffling: 
    public void Shuffle() {}
}

There's certainly lots of room for improvement once you take your design further but I hope the above is a good starting point for you.