patterncppMinor
Deck and Card classes and member-accessing with one header
Viewed 0 times
headerwithdeckcardmemberoneclassesandaccessing
Problem
Previous review of this project:
Card Deck class for a Poker game - version 2
This time, I considered combining both classes into one header file so that I can reduce my use of
Is this the proper thing to do, or could something be done differently? Is there a need for
Deck.h
Card Deck class for a Poker game - version 2
This time, I considered combining both classes into one header file so that I can reduce my use of
#include and to keep them together. I also did this so that I can define my const std::string variables only once for both classes. However, I decided to try using a namespace for these variables for easier access. It makes more sense (to me) to keep them with Card and let Deck refer to them during the latter's construction, but I'm not sure how to do that.Is this the proper thing to do, or could something be done differently? Is there a need for
extern somewhere? The code below doesn't include class implementations, but I've kept them intact from the linked code.Deck.h
#ifndef DECK_H
#define DECK_H
#include
#include
#include
namespace rs
{
const std::string RANKS = "A23456789TJQK";
const std::string SUITS = "HDCS";
}
class Card
{
private:
unsigned rankValue;
char rank;
char suit;
public:
Card();
Card(char r, char s);
bool operator(const Card &rhs) const {return (rankValue > rhs.rankValue);}
bool operator==(const Card &rhs) const {return (suit == rhs.suit);}
bool operator!=(const Card &rhs) const {return (suit != rhs.suit);}
friend std::ostream& operator cards;
int topCardPos;
public:
Deck();
void shuffle();
Card deal();
unsigned size() const {return topCardPos+1;}
bool empty() const {return topCardPos == -1;}
friend std::ostream& operator<<(std::ostream &out, const Deck &aDeck);
};
#endifSolution
I see some things that may help you improve your code.
Consider using an
Instead of using constructs such as
This allows the small storage size of a
Force only valid cards to be created
With the existing default constructor, and with the constructor that takes arguments, any enforcement on validity of initialized values becomes part of the detail of the constructor rather than part of the interface. By using the
Delete unhelpful autogenerated operators
Duplicating cards will get you kicked out of most casinos. It's probably better to avoid such trouble before it begins by making it a little harder to do:
Wrap all of your objects inside a name
Rather than putting just the constants in a namespace, put your whole interface into a namespace. In some future use in which your code is used within an electronic poker game cruise ship simulator, your
Reconsider your class design
The
Reconsider your comparison operators
The comparison operators for the
Consider using an
enum classInstead of using constructs such as
char suit, use the superior safety of an enum class. You can read this question for an overview of when to use one, but I think this would be a better way to do things for this case:enum class suits : char {HEART = 'H', DIAMOND = 'D', CLUB = 'C', SPADE = 'S'};
enum class ranks : char {ACE=1, TWO, THREE, FOUR, FIVE, SIX, SEVEN,
EIGHT, NINE, TEN, JACK, QUEEN, KING };This allows the small storage size of a
char, the convenience of an enum and the type-safety of a class.Force only valid cards to be created
With the existing default constructor, and with the constructor that takes arguments, any enforcement on validity of initialized values becomes part of the detail of the constructor rather than part of the interface. By using the
enum class types mentioned above, we can make it very clear from the interface that it's not going to be possible to create invalid cards:Card(ranks r, suits s);Delete unhelpful autogenerated operators
Duplicating cards will get you kicked out of most casinos. It's probably better to avoid such trouble before it begins by making it a little harder to do:
Card() = delete;
Card(Card&) = delete;Wrap all of your objects inside a name
Rather than putting just the constants in a namespace, put your whole interface into a namespace. In some future use in which your code is used within an electronic poker game cruise ship simulator, your
Card could then be distinguished from a circuit Card that might be part of the electronic game classes, and your Deck could be distinguished from the Decks of a cruise ship.Reconsider your class design
The
Deck class is really just a collection of Card objects. While you may have ideas on how to use your Deck class, it's very likely that you're going to want to have many other kinds of Card collections such as Hand or DiscardPile or Shoe (if you're simulating casino blackjack). In addition, even the notion of Deck can change if you're using it for, say, Pinochle which uses a 48-card deck rather than a 52-card deck. For that reason, it may be better to define a generic Card collection and then specialize that for the various uses. Reconsider your comparison operators
The comparison operators for the
Card class may well be valid for your particular use, in which case, you should certainly use them, but comparisons and relative valuations tend to change by the particular rules of each card game. In some games the Ace is low, and in others high. In some, such as blackjack, it can take on the values of 1 or 11. For that reason, it is likely that the comparison operators are likely to need to be changed. This suggests that they could be made virtual so that derived Card classes can override them.Code Snippets
enum class suits : char {HEART = 'H', DIAMOND = 'D', CLUB = 'C', SPADE = 'S'};
enum class ranks : char {ACE=1, TWO, THREE, FOUR, FIVE, SIX, SEVEN,
EIGHT, NINE, TEN, JACK, QUEEN, KING };Card(ranks r, suits s);Card() = delete;
Card(Card&) = delete;Context
StackExchange Code Review Q#27379, answer score: 4
Revisions (0)
No revisions yet.