Usage flexibility of Deck class

Previous review of this project:

Deck of cards program using a card value map

Based on the previous feedback I've received, my program could become more flexible (it formerly created a standard 52-card deck by itself). Now, the user can add Cards to a Deck, and Card is no longer hidden. I like this approach better (also easier to work with), but I'm sure it can still be improved. Regarding its use, I have a few questions:

-
Although accessors aren't always recommended, would they still be necessary for allowing the driver to know each Card value/rank/suit (unless they're maintained elsewhere)? For instance, Blackjack requires card numerical values for determining a bust.

-
Is the exception-handling in deal() effective (when called on an empty Deck), although it'll still need to return a Card? This is assuming that the user will not always call empty() before calling deal().

-
Is this particular approach too flexible? If so, what could be done instead?

Deck.h

#ifndef DECK_H
#define DECK_H

#include 
#include 
#include 

class Card
{
private:
    unsigned value;
    char rank;
    char suit;

public:
    Card(unsigned v, char r, char s) : value(v), rank(r), suit(s) {}
    friend std::ostream& operator cards;
    int topCardPos;

public:
    Deck();
    void addCard(unsigned value, char rank, char suit);
    void shuffle();
    Card deal();
    std::size_t size() const {return topCardPos+1;}
    bool empty() const {return topCardPos == -1;}
    friend std::ostream& operator<<(std::ostream &out, const Deck &deck);
};

#endif

Deck.cpp

```
#include "Deck.h"
#include
#include

Deck::Deck() : topCardPos(-1) {}

void Deck::shuffle()
{
topCardPos = cards.size()-1;
std::random_shuffle(cards.begin(), cards.end());
}

void Deck::addCard(unsigned value, char rank, char suit)
{
cards.push_back(Card(value, rank, suit));
topCardPos++;
}

Card Deck::deal()
{
try
{
topCardPos--;
return cards.at(topCa

1: Yes, your Card class should expose its rank and suit through a public interface. (See the previous thread and @user225770's answer for value.) It should at least give read-only access, but write access may not be necessary. (It is probably better to create a copy than to alter a card.)

The simplest indication of this is your output stream operator overload. The fact that it needs access to these members is a good indication that other client code (i.e. code using this class) will need access as well.

2: It depends, but in either case it should be modified a bit. Exceptions are used for exceptional circumstances, scenarios that you don't expect to run into a lot. If it's rare that your deck runs out of cards, then throwing an exception is fine. However, you shouldn't throw an exception for your own function. Let the exception propagate to the calling code, and let them decide how to handle the exception.

However, if it's a common occurrence that your deck runs out of cards, you should use another approach. What immediately comes to mind is to simply reduce the chance of this happening by checking empty() before calling deal(). You can verify this behavior by adding an assert at the beginning of the function.

(What you are technically doing is defining a precondition for your function. In short, this means that your function requires and assumes that the deck is not empty when deal() is called.)

3: No, I don't think this solution is over-engineered. However, a more interesting question is "How can I tell if I have over-engineered my solution?". Why, thank you for asking! This leads me to my next, and perhaps most important point:

Start doing unit tests. You are writing good code. The best way for you to improve now is by doing test-driven development. By writing unit tests, several of your questions will answer itself. For example: Should my class expose this member through its interface? Unit tests will answer this question for you by letting you know what you need access to to test and use your code in unit tests.

Test-driven development will let you know if your code is over-engineered. You write a test that works as a requirement to your code. Do you need to do X to make the test pass? Then implement it. Don't you? Then You Ain't Gonna Need It (yet).

StackExchange Code Review Q#27534, answer score: 7