Newest approach to deck of cards project

Question

As this new approach differs from my previous revisions, it shouldn't need references to them.

After receiving much guidance from CR chat, I ended up utilizing these things:

• enums for the Rank and Suit instead of arrays
• a namespace for the enums and Card class
• to_string()s for representing the Ranks and Suits as std::strings in operator<<
• no longer having operator<< as a friend
• two std::vectors for the Deck class (for retaining the original deck when resetting)

I'd like to know if there's still more that can be done, and I have some questions:

1. Is using two vectors an effective way of preserving the deck (as opposed to giving the consuming code the responsibility to return the cards to the deck)?
2. Should something else be done with the enums in case the user doesn't know which Suits are in which order (when filling the deck with cards)?
3. Although enums are built-in, is const& still good for Card's constructor?
4. Is having the user call empty() on the deck good enough so as to avoid complications with exception-handling within the class?
5. Could the two to_string()s for Rank and Suit be shortened while still resembling a look-up table?

Any other feedback is appreciated as well. Beyond these aspects, I desire overall maintainability for adapting to different games (such as adding a Joker). As such, I don't want to hurt the base design with features only associated with a few games.

Card.h

#ifndef CARD_H
#define CARD_H

#include <iostream>
#include <string>

namespace card
{
    enum Rank {Ace,Two,Three,Four,Five,Six,Seven,Eight,Nine,Ten,Jack,Queen,King};
    enum Suit {Clubs,Diamonds,Hearts,Spades};

    class Card
    {
    private:
        Rank m_rank;
        Suit m_suit;

        std::string to_string(Rank const&) const;
        std::string to_string(Suit const&) const;

    public:
        Card(Rank const&, Suit const&);
        std::string to_string() const;
        Rank rank() const {return m_rank;}
        Suit suit() const {return m_suit;}
    };
}

card::Rank& operator++(card::Rank&);
card::Suit& operator++(card::Suit&);
std::ostream& operator<<(std::ostream&, card::Card const&);

#endif

Card.cpp

#include "Card.h"
#include <stdexcept>

card::Card::Card(Rank const& rank, Suit const& suit)
: m_rank(rank), m_suit(suit) {}

std::string card::Card::to_string(Rank const& rank) const
{
    switch (rank)
    {
    case Ace:   return "A";
    case Two:   return "2";
    case Three: return "3";
    case Four:  return "4";
    case Five:  return "5";
    case Six:   return "6";
    case Seven: return "7";
    case Eight: return "8";
    case Nine:  return "9";
    case Ten:   return "T";
    case Jack:  return "J";
    case Queen: return "Q";
    case King:  return "K";
    default: throw std::logic_error("Invalid Rank");
    }
}

std::string card::Card::to_string(Suit const& suit) const
{
    switch (suit)
    {
    case Clubs:    return "C";
    case Diamonds: return "D";
    case Hearts:   return "H";
    case Spades:   return "S";
    default: throw std::logic_error("Invalid Suit");
    }
}

std::string card::Card::to_string() const
{
    return ("[" + to_string(m_rank) + to_string(m_suit) + "]");
}

card::Rank& operator++(card::Rank& r)
{
    return r = card::Rank(static_cast<int>(r)+1);
}

card::Suit& operator++(card::Suit& s)
{
    return s = card::Suit(static_cast<int>(s)+1);
}

std::ostream& operator<<(std::ostream& out, card::Card const& obj)
{
    return out << obj.to_string();
}

Deck.h

#ifndef DECK_H
#define DECK_H

#include <vector>

class Deck
{
private:
    std::vector<card::Card> m_original; // retains original cards when resetting deck
    std::vector<card::Card> m_playable; // cards are drawn from this one

public:
    typedef std::vector<card::Card>::size_type SizeType;

    void reset();
    void shuffle();
    void add(card::Card const&);
    card::Card draw();
    SizeType size() const {return m_playable.size();}
    bool empty() const {return m_playable.empty();}
};

#endif

Deck.cpp

#include "Card.h"
#include "Deck.h"
#include <algorithm>

void Deck::reset()
{
    // is this already exception-safe by itself?
    m_playable = m_original;
}

void Deck::shuffle()
{
    // my tests tell me that an empty vector will not cause this to fail
    std::random_shuffle(m_playable.begin(), m_playable.end());
}

void Deck::add(card::Card const& card)
{
    // new cards always added to both vectors
    m_original.push_back(card);
    m_playable.push_back(card);
}

card::Card Deck::draw()
{
    // still not exception-safe, unless not needed if handled in client
    card::Card topCard(m_playable.back());
    m_playable.pop_back();
    return topCard;
}

Answer 1

General notes

• The to_strings for Rank and Suit do not belong in the Card class. Move them out of the class and into the card namespace instead. They can be useful functions for client code as well.

• I personally dislike the m_ prefix. Normally you can just use normal variable names instead. If you must have a naming scheme for member variables, I think foo_ is more readable. That way my eyes won't have to discard the m_ before getting to the real name.

• I'd put spaces inside braces and after commas to increase readability:

  bool empty() const { return m_playable.empty(); }
  enum Suit { Clubs, Diamonds, Hearts, Spades };
  

  Put the enum definitions on multiple lines if you think the line gets too wide.

• I'd indent the cases in a switch with only single-line cases. Readability trumps consistency:

  switch (suit) {
      case Clubs:    return "C";
      case Diamonds: return "D";
      case Hearts:   return "H";
      case Spades:   return "S";
      default: throw std::logic_error("Invalid Suit");
  }
  

• "My tests tell me that ..." In C++, where so much is unspecified and undefined, we rely on the standard and/or references, and not on tests. In this case it's easy to be on the safe side:

  void Deck::shuffle()
  {
      if (m_playable.empty()) return;
  
      std::random_shuffle(m_playable.begin(), m_playable.end());
  }
  

• Regarding exception safety for Desk::reset: std::vector::operator= gives the basic exception safety guarantee: If the assignment operator throws, the objects will be in a valid state. Since m_playable already has the required capacity, I can't think of a reason why it should ever throw anyway.

• Deck::draw() is exception-safe: std::list::pop_back will never throw. See below.

• Your Card.h header should include <ostream> instead of <iostream>. Since you only need the std::ostream declaration, you could simply include <iosfwd> and include <ostream> in Card.cpp instead.

• Is there a reason why op++ and op<< for Card isn't in the card namespace?

• You should write unit tests and document your interface.

---

Answers for your questions

Is using two vectors an effective way of preserving the deck (as opposed to giving the consuming code the responsibility to return the cards to the deck)?

I think it is. Asking client code to return the cards is too demanding.

---

Should something else be done with the enums in case the user doesn't know which Suits are in which order (when filling the deck with cards)?

The client code should be perfectly able to handle this as it is. If it doesn't want to use your increment operator, it can simply define four loops when populating the deck, or even implement its own function to iterate the suits.

---

Although enums are built-in, is const& still good for Card's constructor?

Pass enums by value.

---

Is having the user call empty() on the deck good enough so as to avoid complications with exception-handling within the class?

Yes. As it is, Deck::draw() has the strong exception safety guarantee: If something goes wrong, it will be rolled back to its original state. In this case you get this for free: std::list::pop_back() has a nothrow guarantee. That means that the only operation in draw() that can fail is calling m_playable.back(), and that doesn't modify the container. As long as that call succeeds, the entire function will as well.

---

Could the two to_string()s for Rank and Suit be shortened while still resembling a look-up table?

I think the way you have done it is fine. You could use a std::map, but that approach has no advantages compared to your approach. If you want to do it anyway, you can do it like this:

std::string to_string(Rank rank)
{
    static const map<Rank, string> rank_names = { {Ace, "A"}, {Two, "2"}, {Three, "3"} /* ... */ };

    return rank_names[rank];
}

Note that this has O(n log n) lookup time, whereas your implementation is likely to be O(1). You could also define an array and use the enum values as indices. That would also be O(1), but there's still no point -- and that approach violates the abstraction.

---

Finally, I have a question for you: How would you implement a joker?