Design review
Card
Let’s start by looking at the Card class. As designed, Card has 4 data members:
value is an unsigned int, while the others are all std::strings.
This is enormously wasteful. A card really only has two properties: rank and suit. If you know the rank, you know the value and the name, so duplicating that data in the class is just wasting space.
I don’t even see how your code even compiles, because rank is a string, but in Deck::makeDeck() you try to shove integer values into it.
In addition, using the wrong types also wastes space. The suit doesn’t need to be a string. It can have only one of 4 possible values. You can fit the suit in a single byte. Using a string is not only ~20–50 times bigger, it might also require memory allocations, and every operation becomes more expensive (for example, comparing suits could be a simple byte value compare, rather than a much more expensive string comparison).
I think the first thing you need to look at is enumerations. Enumerations are perfect for when you have something that has a fixed, small number of values. Like a card suit, for example. You could do:
enum class suit_t
{
hearts,
diamonds,
clubs,
spades
};
That’s it! To make it even better, you can specify that it has to be a single byte in size:
enum class suit_t : unsigned char
{
hearts,
diamonds,
clubs,
spades
};
Then if you need things like conversion to a string for printing, you can add functions or stream inserters as needed:
enum class suit_t : unsigned char
{
hearts,
diamonds,
clubs,
spades
};
// a to_string() function:
auto to_string(suit_t s) -> std::string
{
switch (s)
{
case suit_t::hearts:
return "hearts";
case suit_t::diamonds:
return "diamonds";
case suit_t::clubs:
return "clubs";
case suit_t::spades:
return "spades";
}
}
// a stream inserter:
template <typename CharT, typename Traits>
auto operator<<(std::basic_ostream<CharT, Traits>& out, suit_t s)
-> std::basic_ostream<CharT, Traits>&
{
switch (s)
{
case suit_t::hearts:
out << "hearts";
break;
case suit_t::diamonds:
out << "diamonds";
break;
case suit_t::clubs:
out << "clubs";
break;
case suit_t::spades:
out << "spades";
break;
}
return out;
}
You can do the same thing for the rank:
enum class rank_t : unsigned char
{
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
};
Then you can add whatever functions you need:
constexpr auto to_value(rank_t r) noexcept -> int
{
switch (r)
{
case rank_t::jack:
[[fallthrough]];
case rank_t::queen:
[[fallthrough]];
case rank_t::king:
return 10;
default:
return static_cast<int>(r);
}
}
And with those, making a card type is trivial:
struct card_t
{
rank_t rank;
suit_t suit;
};
That’s about all you need! Then of course you can add useful functions, like a stream inserter:
template <typename CharT, typename Traits>
auto operator<<(std::basic_ostream<CharT, Traits>& out, card_t c)
-> std::basic_ostream<CharT, Traits>&
{
if (out)
{
auto oss = std::basic_ostringstream<CharT, Traits>{};
oss << c.rank << " of " << c.suit;
out << oss.view();
}
return out;
}
And to really illustrate how big a difference makes, compare the sizes of your Card and my card_t. Using Clang and libc++:
Using GCC:
You see? By using enumerations, the card class is ~40–50 times smaller. Assuming a cache line size of 64 b, you can actually fit an entire deck of card_t in just two cache lines, whereas not even a single Card would fit. (And if you were really clever, you could get the size of card_t down to a single byte, so a whole deck could fit in a cache line.) And operations like comparing two cards will be thousands of times faster.
If I were designing a card class, I would make both the suit and the rank inner types of that class, to keep everything clean:
struct card_t
{
enum class rank_t : unsigned char
{
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
};
enum class suit_t : unsigned char
{
hearts,
diamonds,
clubs,
spades
};
rank_t rank = rank_t::ace;
suit_t suit = suit_t::spades;
constexpr auto operator==(card_t const&) const noexcept -> bool = default;
};
If you want to support more deck types, like including the knight rank, you can just add that into the rank_t enumeration (you could adjust the number values so the knight rank is 12, the queen is 13, and the king is 14). If you want more suits, like stars or waves, no problem, just add those to suit_t. If you want to add support for jokers, trumps, or the fool, things get a little more complicated, but not ridiculously so. You could add a “none” suit… or maybe special “white” and “black” suits for the jokers. Or you could use a variant to distinguish between suited cards, trumps, jokers, and the fool.
Deck
Once you’ve shrunk the size of the card class, the deck class will also be much more efficient.
Your deck class has 3 data members, but I don’t see the point of suit, and card seems to be a misguided way to have a local variable in your makeDeck() function. The only data member you actually need is cards.
Now, cards is a vector, and that’s not bad… but consider that you don’t really need the full power of a vector. You know in advance that there can never be more than 52 cards in your deck. Given that, you could use something like Boost’s static_vector, and avoid any dynamic allocation at all.
But for a start, vector is fine.
However, what the deck class really needs is some member functions, because if you’re just going to expose the cards data member, you might as well not have a deck class at all: just use a vector of cards. Designing a good interface is an art and a science, but a decent deck class might look something like this:
class deck_t
{
public:
// creates a full deck of 52 cards
static auto full_deck() -> deck_t;
// creates an empty deck
deck_t();
// creates a deck with the cards given
template <std::input_iterator It, std::sentinel_for<It> Sen>
deck_t(It, Sen);
template <std::input_range Rng>
deck_t(Rng&&);
// sort the deck
auto sort() -> void;
template <typename Cmp>
auto sort(Cmp) -> void;
// shuffle the deck
template <std::uniform_random_bit_generator<std::remove_reference_t<Gen>>
auto shuffle(Gen&&) -> void;
// draw cards from the top of the deck
auto draw() -> card_t;
auto draw(std::size_t) -> std::vector<card_t>;
template <std::output_iterator O>
auto draw(std::size_t, O) -> O;
// draw cards randomly from the deck
template <std::uniform_random_bit_generator<std::remove_reference_t<Gen>>
auto draw_randomly(Gen&&) -> card_t;
template <std::uniform_random_bit_generator<std::remove_reference_t<Gen>>
auto draw_randomly(std::size_t, Gen&&) -> std::vector<card_t>;
template <std::output_iterator O, std::uniform_random_bit_generator<std::remove_reference_t<Gen>>
auto draw_randomly(std::size_t, O, Gen&&) -> O;
// place cards onto the top of deck
auto replace(card_t) -> void;
template <std::input_iterator It, std::sentinel_for<It> Sen>
auto replace(It, Sen) -> It;
template <std::input_range Rng>
auto replace(Rng&&) -> std::borrowed_iterator_t<Rng>;
// place cards at the bottom of the deck
auto place_at_bottom(card_t) -> void;
template <std::input_iterator It, std::sentinel_for<It> Sen>
auto place_at_bottom(It, Sen) -> It;
template <std::input_range Rng>
auto place_at_bottom(Rng&&) -> std::borrowed_iterator_t<Rng>;
// place cards randomly into the deck
template <std::uniform_random_bit_generator<std::remove_reference_t<Gen>>
auto place_randomly(card_t, Gen&&) -> void;
template <std::input_iterator It, std::sentinel_for<It> Sen, std::uniform_random_bit_generator<std::remove_reference_t<Gen>>
auto place_randomly(It, Sen, Gen&&) -> It;
template <std::input_range Rng, std::uniform_random_bit_generator<std::remove_reference_t<Gen>>
auto place_randomly(Rng&&, Gen&&) -> std::borrowed_iterator_t<Rng>;
// move some cards from the top of this deck to the top of another
auto transfer_card_to(deck_t&) -> void;
auto transfer_cards_to(deck_t&, std::size_t) -> void;
auto transfer_all_cards_to(deck_t&) -> void;
// create a new deck from some part of the top of this deck
auto draw_deck(std::size_t) -> deck_t;
// create a new deck by drawing randomly from this deck
template <std::uniform_random_bit_generator<std::remove_reference_t<Gen>>
auto draw_deck_randomly(std::size_t, Gen&&) -> deck_t;
// standard container functions //////////////////////////////////////////
auto begin() const noexcept -> card_t const*;
auto end() const noexcept -> card_t const*;
auto cbegin() const noexcept -> card_t const*;
auto cend() const noexcept -> card_t const*;
auto rbegin() const noexcept -> std::reverse_iterator<card_t const*>;
auto rend() const noexcept -> std::reverse_iterator<card_t const*>;
auto crbegin() const noexcept -> std::reverse_iterator<card_t const*>;
auto crend() const noexcept -> std::reverse_iterator<card_t const*>;
auto front() const -> card_t const&;
auto back() const -> card_t const&;
auto operator[](std::size_t) const -> card_t const&;
auto at(std::size_t) const -> card_t const&;
auto empty() const noexcept -> bool;
auto size() const noexcept -> std::size_t;
auto max_size() const noexcept -> std::size_t; // should return 52
};
You may not need all the functions in that interface, but some of them will sure come in handy. This is what a simple game might look like with this interface:
// start with a full deck, and shuffle it
auto deck = deck_t::full_deck();
// start a new game
while (playing)
{
deck.shuffle(rng);
// also need a discard pile
// (though we could just "discard" by putting cards at the bottom of the deck)
auto discard_pile = deck_t{};
// let's say there are 4 players, each with their own hand
// (we can reuse the deck class for a hand)
constexpr auto player_count = std::size_t{4};
auto player_hands = std::array<deck_t, player_count>{};
// now each player draws 5 cards
for (auto&& player_hand : player_hands)
player_hand = deck.draw_deck(5);
// now each player examines their hand
std::ranges::for_each(player_hands, [](auto&& hand) { hand.sort(); });
// now give each player a chance to exchange cards to get a better hand
for (auto player = std::size_t{0}; player < player_count; ++player)
{
auto exchange_count = // ask player how many cards to exchange
player_hands[player].transfer_cards_to(discard_pile, exchange_count);
deck(player_hands[player], exchange_count);
}
// now sort by the score of the hand
std::ranges::sort(player_hands, hand_score_func);
// and print the results
std::cout << "the results in order are:\n";
std::ranges::for_each(player_hands, [](auto&& hand) { std::cout << '\t' << hand << '\n'; });
// restore all the cards back to the deck for another game
discard_pile.transfer_all_cards_to(deck);
std::ranges::for_each(player_hands, [&deck](auto&& hand) { hand.transfer_all_cards_to(deck); });
}
Dealer
The dealer class structure isn’t bad, but you could use the deck class for both the player and house hands. They are, in a way, decks, after all.
The real problem with the dealer class is that it just does too much. It’s not just a dealer… it plays the whole game… and even handles asking if the player wants to replay… and shuffles the deck… and draws the cards….
Strip it down. First of all, the proper name for this class probably isn’t “dealer”, but rather “game”. All the class should do is handle setting up and then playing through a game. Shuffling? Let the deck class handle that. Randomly drawing cards? Again, let the deck class handle that. Asking the user if they want to replay? Let something else handle that. The game class should be just about the game itself.
class game
{
public:
// play a game
auto play() -> void;
private:
std::mt19937 _rng;
};
And the play function could be something like:
auto game::play() -> void
{
auto deck = deck_t::full_deck();
deck.shuffle(_rng);
auto player = deck.draw_deck(N); // N is how many cards each player starts with
auto house = deck.draw_deck(N);
auto credits = 100;
auto done = false;
while (not done)
{
// player moves first
auto choice = get_player_choice();
switch (choice)
{
case 'd': // player wants to draw cards
{
auto count = get_player_draw_count();
deck.transfer_cards_to(player, count);
}
break;
case 'b': // player wants to make a bet
// and so on
}
// now the house makes its move
// check for whether someone won, and if so, print the result and
// set done to true
}
}
After the game is done and play() returns, then you can have an external loop—in main(), for example—that asks “do you want to play again?”, and if the answer is yes, it will just call play() again.
Think like a game programmer
All games fundamentally have the same structure:
initialize();
while (game_is_not_over())
{
input(); // get input from the user
update(); // update the game state
render(); // display the current state
}
Even a game as simple as a console-only card game uses the same pattern. After initializing the game (setting up the deck and dealing hands to all the players), you start the loop where you first ask the user what they want to do… and then you update the game state, which includes giving the AI its turn and checking to see if there are an winners… then you output the current situation to the player, telling them what cards they’re holding, or whether the game is over (and who won).
If you want to allow replaying, then you just need to wrap all that in a loop:
do
{
auto game = game_t{};
while (game.running())
{
game.get_input();
game.update();
game.render(std::cout);
}
std::cout << "\n\n=============================\n\n\n";
} while (wants_to_play_again());
std::cout << "goodbye\n";
Before starting any games, you could set up your random number generator:
auto main() -> int
{
auto prng = std::mt19937{std::random_device{}()};
do
{
auto game = game_t{prng};
while (game.running())
{
game.get_input();
game.update();
game.render(std::cout);
}
std::cout << "\n\n=============================\n\n\n";
} while (wants_to_play_again());
std::cout << "goodbye\n";
}
Starting from that high-level design, you can fill in the functions you need. And by designing it this way, you will already have the general structure in place to convert to a graphical game if you like. (The differences would mostly be that you’re no longer getting input from std::cin or rendering text output to std::cout; the actual game logic in the update function should be unchanged.)
General issues
Don’t use std::endl
If you just want a newline, use '\n'. std::endl doesn’t just print a newline, it flushes the stream, which can be expensive.
So if you want a spacing of 2 lines, and you do something like std::cout << std::endl << std::endl << "========" << std::endl << std::endl;, you are flushing the stream at least 4 times. That’s ridiculous.
All you need is: std::cout << "\n\n========\n\n";.
Don’t shuffle the deck every time a card is drawn
Shuffling over and over doesn’t actually improve randomness, and it costs.
Instead, just shuffle once at the beginning of the game. That’s how it works in real life, after all.
Shuffle once, and then just keep picking cards off the top of the deck.
Code review
using namespace std;
Never, ever do this. Just put std:: where it’s needed.
Card() {}
~Card() {}
You don’t need to explicitly spell out these functions. Classes get default constructors and destructors by default. Actually writing out these functions causes three problems:
It confuses developers.
An experienced C++ coder who sees these functions will immediately be suspicious, because why would anyone waste time writing code that they don’t need to. If someone sat down and wrote these out, that must mean there’s some reason for it… right? Now the coder will waste time trying to hunt down the reason.
It creates a maintenance burden.
Every line of code you write adds to the maintenance debt. It makes the code harder to read (because more code is harder to read than less code), harder to spot problems (because if you have a bunch of useless, noise code around the important stuff, the important stuff will be harder to see, and so will bugs), and harder to work with (because every time you want to make a change, you have to check more code to see if there will be any conflicts).
It actually slows down your program.
When you define the default constructor or destructor like this, you are creating a non-trivial default constructor and non-trivial destructor. That’s bad. Trivial default constructors and trivial destructors are a good thing, because if they’re trivial, the compiler can optimize them away in most cases. This can give you huge performance gains.
So just don’t write functions you don’t need.
unsigned value=0;
Don’t use unsigned types for regular numbers. They cause surprises when doing calculations and comparisons. Just use an int.
Other than that, I already showed how you can optimize the card class down to something only 2 bytes large, that will be blazingly fast.
Deck() {}
Again, you don’t need this, so don’t write it.
vector<Card> cards;
vector<Card> suit;
Card card;
I don’t understand the point of all these members. A deck shouldn’t be anything other than a bunch of cards. Why do you also need a bunch of suits, and a separate, single card?
void Deck::makeDeck() {
for(int i=0; i<4; i++) {
for(int j=1; j<14; j++) {
There are a lot magic numbers hidden in there. Magic numbers are a bad idea. You should always use named constants that explain what’s going on:
void Deck::makeDeck() {
for (int i = suit_min; i < suit_max; ++i) {
for (int j = rank_min; j < rank_max; ++j) {
You should also give your variables meaningful names. Names like i and j make sense when the loop counter is just an index, and serves no other purpose. But these values do have another meaning:
void Deck::makeDeck() {
for (int suit = suit_min; suit < suit_max; ++suit) {
for (int rank = rank_min; rank < rank_max; ++rank) {
The next problem with this function is that you use an outside variable for the card. That makes no sense. The card should be a local variable.
And of course, the biggest and most obvious problem with the function is that it’s incredibly repetitive. You could simplify it considerably like this:
void Deck::makeDeck() {
// you should really clear the deck first
cards.clear();
// to speed things up, you should reserve the memory needed beforehand
cards.reserve(number_of_cards_in_deck);
for (int suit = suit_min; i < suit_max; ++i) {
for (int rank = rank_min; j < rank_max; ++j) {
auto card = Card{};
// set up some default values
card.value = rank;
// card.rank = rank; // this won't compile because your types are wrong!
switch (suit)
{
case suit_hearts:
card.suit = "HEARTS";
break;
case suit_diamonds:
card.suit = "DIAMONDS";
break;
// and so on...
}
switch (rank)
{
case rank_ace:
card.name = "ACE";
break;
case rank_two:
card.name = "TWO";
break;
// and so on...
// face cards are special because their value isn't the same as
// their rank
case rank_jack:
card.name = "JACK";
card.value = 10;
break;
case rank_queen:
card.name = "QUEEN";
card.value = 10;
break;
case rank_king:
card.name = "KING";
card.value = 10;
break;
}
cards.push_back(std::move(card));
}
}
}
But this is still an ugly function. A much better solution is to make a proper card class, with proper suit and rank classes. Then you can just do something like:
void Deck::makeDeck()
{
constexpr auto all_ranks = std::array{
card_t::rank_t::ace,
card_t::rank_t::two,
// and so on...
};
constexpr auto all_suits = std::array{
card_t::suit_t::hearts,
card_t::suit_t::diamonds,
card_t::suit_t::clubs,
card_t::suit_t::spades
};
cards.clear();
cards.reserve(all_ranks::size() * all_suits::size());
for (auto&& rank : all_ranks)
for (auto&& suit : all_suits)
cards.emplace_back(rank, suit);
}
For a different game type, with different suits or ranks, or with the addition of jokers or trumps, you just need to modify those arrays, and/or add a third array for specials.
That’s how clean and simple C++ code gets when you make proper types. Get the types right, and everything else just magically falls into place.
Dealer() {}
Yet again, unnecessary code.
void Dealer::playAgain() {
deck.makeDeck();
printHand(deck.cards);
char choice='y';
cout<<endl<<endl<<"============================="<<endl<<endl;
cout<<endl<<"Would you like to play again?(y/n): ";
cin>>choice;
if(choice=='y') {
//deal21();
deck.makeDeck();
printHand(deck.cards);
} else {
cout<<"goodbye";
exit(0);
}
}
The way this function is currently written, you only get two games and then the program quits. That’s because you’ve embedded the “play again” option in the middle of the same function that actually plays the game.
I’d say before you consider the notion of “play again”, you first need to consider “play”. If you have a “play” function, then the “play again” function is just:
auto play_again()
{
auto done = false;
while (not done)
{
play();
std::cout << "\n\n=============================\n\n"
"\nWould you like to play again?(y/n): ";
auto choice = '\0';
while (not (std::cin >> choice) or (choice != 'y' and choice != 'n'))
{
if (std::cin.eof())
{
// reached the end of the input, which pretty much means we're done
done = true;
break;
}
// clear any errors on cin
std::cin.clear();
if (not std::cin)
throw std::runtime_error{"input is completely borked"};
// note you could also ignore everything on cin up to the next newline
std::cout << "Sorry, didn’t understand your answer.\n"
"Play again? (y/n): ";
}
if (choice == 'n')
done = true;
}
std::cout << "goodbye\n";
}
Or, even better, separate all that query logic into its own function:
template <std::forward_range Rng>
auto get_choice(std::istream& in, std::ostream& out, std::string_view query, Rng const& choices)
{
while (true)
{
out << query;
out.flush();
if (auto choice = '\0'; in >> choice)
{
if (std::ranges::find(choices, choice) != std::ranges::end(choices))
return choice;
out << "Sorry, '" << choice << "' is not a valid choice.\n";
}
else
{
throw std::runtime_error{"bad input"};
}
}
}
auto play_again()
{
while (true)
{
play();
std::cout << "\n\n=============================\n\n\n";
if (auto const choice = get_choice(std::cin, std::cout, "Would you like to play again?(y/n): ", "yYnN"); choice == 'n' or choice == 'N')
break;
}
std::cout << "goodbye\n";
}
With the “play” function separated, all the actual game logic can go there.
} else {
cout<<"goodbye";
exit(0);
}
Never, ever use std::exit() in C++. It’s a C function; it doesn’t work properly in C++.
void Dealer::getCard(vector<Card> &hand) {
int pick=randomizeDeck(deck.cards);
hand.push_back(deck.cards[pick]);
deck.cards[pick]=deck.cards.back();
deck.cards.pop_back();
}
This is a rather peculiar way to draw a card from a deck. First of all, it doesn’t make sense to pull a card randomly from out of the middle of the deck. That’s not how people play cards in real life. They shuffle the deck once at the beginning of the game, and then just draw from the top of the deck.
But then you do something even more peculiar: you copy the card from the middle of the deck… then copy the card at the top of the deck over that first card… then remove the card at the top of the deck. That’s a lot of gymnastics just to draw a card from a deck. All that wouldn’t be necessary if the deck were shuffled.
void Dealer::printHand(vector<Card> &hand) {
for(int i=0; i<hand.size(); i++) {
cout<<hand[i].name<<" of "<< hand[i].suit<<endl;
}
}
You can take the hand as a const reference here, because you’re not modifying it. Also, I don’t see the logic in making this a member of Dealer. This is a useful function, generally, so why not make it a free function?
In any case, you have a bug. i is an int… but hand.size() is a vector<Card>::size_type, which is an unsigned integer of some kind. Don’t use old-fashioned for-loops with ranges… but if you really must, get the types right. That loop should be:
for (auto i = decltype(hand.size())(0); i < hand.size(); ++i) {
std::cout << hand[i].name << " of " << hand[i].suit << '\n';
}
But again, don’t use old-school for loops for this kind of thing. Use a range-for:
for (auto&& card : hand) {
std::cout << card.name << " of " << card.suit << '\n';
}
or an algorithm:
std::ranges::for_each(hand, [](auto&& card) {
std::cout << card.name << " of " << card.suit << '\n';
});
And even better would be if cards knew how to print themselves. Again, get the types right, and everything else just works. If cards did know how to print themselves, then this function could just be:
auto print_hand(std::ostream& out, std::vector<Card> const& hand)
{
std::ranges::for_each(hand, [&out](auto&& card) { out << card << '\n'; });
}
Even better still would be if you had a class for hands (which could also just reuse the deck class), because then you would’t need this function at all. You could just do std::cout << hand;. As always, spend a little time getting the low-level types right, and the high-level logic becomes trivial.
int Dealer::randomizeDeck(vector<Card> &cards) {
int max=cards.size()-1;
random_device rd;
mt19937 numGen(rd());
static uniform_int_distribution<int> roll(0,max);
return roll(numGen);
}
What you’re doing here is clever, but there are a lot of problems with how you’re doing it.
First, int is the wrong type. If you are indexing a vector, the correct type to use is the vector’s size_type. vector’s size_type is going to be unsigned (which, yes, is not good, but it was a decision made long ago, before people realized the problems it would cause), and may also be much bigger than int. The net result is that if you try to cram the result of cards.size() - 1 into an int… you could end up triggering undefined behaviour. That’s very bad.
The fix here is so trivial it’s almost laughable. Just don’t say int. Just use auto (or nothing at all):
auto get_random_card_index(std::vector<Card> const& cards)
{
auto const max = cards.size() - 1;
std::random_device rd;
std::mt19937 numGen(rd());
static std::uniform_int_distribution roll(0, max);
return roll(numGen);
}
Bug fixed.
The next major problem with this function is that every time you ask for a random index into a set of cards, you’re constructing a new random number generator. That makes no sense, and it is remarkably wasteful. Much better would be just pass a random number generator into the function:
template <std::uniform_random_bit_generator<std::remove_reference_t<Gen>>
auto get_random_card_index(std::vector<Card> const& cards, Gen&& gen)
{
auto dist = std::uniform_int_distribution{0, cards.size() - 1};
return dist(gen);
}
But this still isn't a great function, because it doesn’t take into account the case where the set of cards is empty.
All these problems would evaporate if you just shuffled the deck once, and then picked from the top of the deck.
