0

I am creating poker simulator for learning purposes, but having problem making Deck class immutable.

public class Deck {

     private final Card[] cards;
     private int cardCount;

     public Deck(@NotNull Card[] c) {
          this.cards = Arrays.copyOf(c, c.length);
          this.cardCount = c.length - 1;
     }

     public Deck shuffle() {
          // shuffle logic
          return new Deck(this.cards);
     }

     public Card pop() {
          return this.cards[cardCount--];
     }
     // other methods
}

Dealer class holds Deck reference and implements dealing logic.

public class Dealer {

     private final Deck deck;

     public Dealer(@NotNull Deck d) {
          this.deck = d;
     }

     public void deal(@NotNull Holder<Card> h) {
          h.hold(deck.pop());
     }
     // other methods
}

As you can see Deck is not totally immutable, because it holds cardCount which is being updated every time when pop is called. !Note - cardCount is not visible to outside, also Card class is immutable.

Question 1: How this design affects Deck immutability?

Question 2: What could be future implications?

Question 3: Is it worth making Deck class totally immutable? If yes, then how?

Thank you for help.

6
  • 1
    It's not only the counter that makes your class mutable, but also the array, as arrays are mutable by nature, even though your reference is final. Commented Aug 6, 2016 at 1:11
  • I make defensive copies for arrays. Commented Aug 6, 2016 at 1:12
  • The real problem here is pop, which you've designed as a mutative operation. No internal fiddling will make your Decks immutable as long as they still need to support a pop method with the given semantics. Commented Aug 6, 2016 at 1:12
  • Why not use a Stack, which is designed for this purpose and is also thread safe ? Commented Aug 6, 2016 at 1:17
  • Stack will be even worse. It's pop method will mutate Deck, and it's peek method only gives top item without mutating, also I do not need push method. Commented Aug 6, 2016 at 1:24

3 Answers 3

2

This is one of the few cases where immutability doesn't help. You seem to need a mutable shared state, as this is the whole purpose of that Deck.

To make it immutable would mean that you'd have to create a new Deck with the remaining cards every time someone pops one out. But you have no control over the old Deck (before the top card removal), so this may still be referenced by some client -- which defeats the whole purpose of the game, in my opinion.

With your current implementation, you can run into problems with multiple threads so consider making your class thread safe. Using a java.util.Stack instead of an array with a counter would be a good first attempt.

Sign up to request clarification or add additional context in comments.

2 Comments

I see now. If the same Deck will be used by different Dealers then I will be in trouble. But I don't see how Stack would help me. If one Dealer updates stack by poping a card, another will not be able to use it anymore. The only solution for this, is to make sure that every Dealer has it's own copy of the Deck.
@DennisGrinch yep ... and not only for each Dealer. Since the Deck is mutable and only removes cards, every time you start a new game you will need a new Deck instance even i.f it is with the same dealer
1

In my opinion, the mutability depends on two things: How many instances you would require if the object were to be immutable, and whether or not an immutable copy would ever be reused.

The first is easy, you don't need to make every permutation of a deck, you can just make a new one when needed, so that's not something which stops you from making it immutable.

Where I start to see a problem is the reusability. You make a deck immutable, but if the immutability only serves to provide a brand new object and throw away the old, it seems more logical to just mutate the object in place and not be creating unnecessary ones.

I do however see a definite use of immutable cards; There will only ever be 54 of them and they can definitely be reused. So what I would do is store a cache of those cards in an immutable List or Set:

private static final List<Card> CARDS = Collections.unmodifiableList(new ArrayList<Card>() {{
    //store all your cards, can be done without double-brace initialiation
}});

From there (in the context of being inside Deck), we can make a copy of this List without costing much memory at all - the Card instances will be the same since Java is "pass-by-value-of-reference".

//Note: The left hand side is purposefully LinkedList and not List, you'll see why
private final LinkedList<Card> shuffledDeck = new LinkedList<>();

public Deck() {
   this.shuffle();
}

public Deck shuffle() {
    this.shuffledDeck.clear(); //wipe out any remaining cards
    this.shuffledDeck.addAll(CARDS); //add the whole deck
    Collections.shuffle(this.shuffledDeck); //Finally, randomize
    return this;
}

public Card pop() {
    return this.shuffledDeck.pop(); //Thanks Deque, which LinkedList implements
}

This leaves you with the saved memory from making Card immutable, whilst not costing you numerous Deck objects.

Comments

1

The biggest problem is that you have a mutating method on the Deck class. You DO need to be able to mutate the cards you have in a deck during a game, but you actually don't need to mutate the actual Deck object for that. Suppose that you have an immutable card class and an immutable deck class. The deck is initialized with a full complement of cards. When you need to play a game you ask for a list of the cards that the dealer will be able to interact with, modify, etc. So the Deck class only really needs to hand a copy of the internal list out. That way the Dealer isn't modifying the internal list and the Deck is ready for a new game when requested and it doesn't matter that the copied list holds references to the same card instances as the internal Deck class since the Cards are also immutable.

public final class Card {

    public enum Suit { HEART, SPADE, CLUB, DIAMOND }
    public enum Value { ACE, TWO, THREE, FOUR, FIVE, SIX, SEVEN, EIGHT, NINE, TEN, JACK, QUEEN, KING }

    private final Suit suit;
    private final Value value;

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

    // ... rest omitted
}

public final class Deck {

    private final List<Card> cards = new ArrayList<>();

    public Deck() {
        for (Card.Suit suit : Card.Suit.values()) {
            for (Card.Value value : Card.Value.values()) {
                cards.add(new Card(suit, value));
            }
        }
    }

    // Get a new list of cards (though the actual card instances are the same
    // that's ok since they are immutable)
    public List<Card> getCards() {
        return new ArrayList<>(cards);
    }
}

Also since no state is being shared and the Deck is no longer mutable, then thread safety is no longer a concern. Another bonus of using a list of cards for the dealer is that there are built in sort (if you had card implement comparable), shuffle, etc, via the Collections class.

Edit: added after reading your comments:

If you want the Deck to hold state then you cannot have it be immutable. It can be implemented a number of different ways. Even though an ArrayList is a fairly large data structure compared to an array in almost all cases the use of it is better than a LinkedList in performance and is better than an array for safety and ease of use. If very, very high level performance is required you can get better with a primitive array (maybe) but the Collections API is quite optimized and the JVM is also extremely good at optimizing code on the fly. In my professional experience so far I have yet to run into a case where I would use an array over a list simply due to performance. In general I would say that is a case of premature optimization (beware of that). Though it makes sense in an abstract sense to use a Stack over an ArrayList, I would argue that the convenience of the built in shuffle method for lists (in Collections) would override that.

Using the same Card objects as above, the mutable Deck could look as follows (with a list):

public class EmptyDeckException extends RuntimeException {

    static final long serialVersionUID = 0L;

    public EmptyDeckException(String message) {
        super(message);
    }
}

public class Deck {

    private final List<Card> cards = new ArrayList<>();

    public Deck() {
        for (Card.Suit suit : Card.Suit.values()) {
            for (Card.Value value : Card.Value.values()) {
                cards.add(new Card(suit, value));
            }
        }
    }

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

    public Card pop() {
        if (!cards.isEmpty()) {
            return cards.remove(cards.size() - 1);
        }
        throw new EmptyDeckException("The deck is empty");
    }
}

And if a stack is insisted on, then it could be like this:

public class Deck {

    private final Stack<Card> cards = new Stack<>();

    public Deck() {
        for (Card.Suit suit : Card.Suit.values()) {
            for (Card.Value value : Card.Value.values()) {
                cards.push(new Card(suit, value));
            }
        }
    }

    public void shuffle() {
        // Now this needs to be done manually and will probably be less performant
        // than the collections method for lists
    }

    public Card pop() {
        return cards.pop();
    }
}

4 Comments

Thnks for input but, but ArrayList is way over bloated data structure for this. I want implement Deck as data structure which holds cards, and only has pop and shuffle methods.
That's fine but if you want your deck to hold the current state it can't be considered immutable. Also, why do you say it's bloated. Is it because of the number of other methods or because you think it isn't performant?
Because ArrayList is for storing, adding and removing elements efficiently. But my deck don't need all the functionality. Better use Stack for that, but even that it is not right data structure, cause it push and then pop elements. On other hand, when u start play poker, you have deck of cards, you shuffle cards, and take one by one from the top.
@DennisGrinch added some examples for a mutable deck, one with a list and one with a stack along with some of my opinions on them in this use case. Hope it helps!

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.