4
\$\begingroup\$

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; }
\$\endgroup\$

1 Answer 1

Reset to default
2
\$\begingroup\$

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?

\$\endgroup\$
7
  • \$\begingroup\$ +1 great stuff yet again. Relieving feedback for the exception-handing. Regarding std::map: ignore what I said about it in chat (if you happened to read it). I soon realized that it would be ineffective as a replacement to switch (it would be re-created after each call). Unfortunately, I cannot initialize it like you've mentioned due to compiler constraints. Had this not been an issue, I could easily make it static const and left a happy camper. \$\endgroup\$ Commented Aug 3, 2013 at 22:07
  • \$\begingroup\$ A non-rhetorical question for me? I love the way you think. :-) As it so happens, I have been thinking about this. I came up with the following points: 1.) they greatly vary from game-to-game, 2.) they don't have a rank and suit, thus my Card class cannot support one, and 3.) as its role (based on the game) vary, the basic implementation of a joker would be quite limited. The third point propels me to my first solution: give it its own class. Putting it in Card seems like a bad idea. \$\endgroup\$ Commented Aug 3, 2013 at 22:11
  • \$\begingroup\$ How would you then mix it with Deck? \$\endgroup\$ Commented Aug 3, 2013 at 22:12
  • \$\begingroup\$ Note that I have updated the answer and added two points. Even if you were able to make a static const map, you still shouldn't. The code may be slightly shorter, but it's less effective and about the same in terms of readability. \$\endgroup\$ Commented Aug 3, 2013 at 22:13
  • \$\begingroup\$ Okay, that I haven't quite contemplated, but I'll give it a go: 1.) give Deck another add() specifically for jokers. Looking at its implementation, that looks like the only viable change. \$\endgroup\$ Commented Aug 3, 2013 at 22:14

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.