3
\$\begingroup\$

I'm attempting to improve my coding style by concentrating on simple and focused OOP client-server interfaces and error handling - atm in the context of designing a Blackjack game. I'm starting with the Card class that represents an individual card. I'm looking for any improvements on or flaws with my approach.

#include <sstream>
#include <stdexcept>
#include <string>

class Card
{
public:
    enum Ranks { Ace = 1, Ten = 10, Jack, Queen, King };
    enum Suits { Spades, Hearts, Diamonds, Clubs };

    Card( int rank, int suit ) throw(std::range_error)
    {
            if( rank < 1 || rank > 13 ) {
                    std::stringstream errMsg;
                    errMsg << "Card(): rank '" << rank << "' out of range";
                    throw std::range_error( errMsg.str() );
            } else if( suit < 0 || suit > 3 ) {
                    std::stringstream errMsg;
                    errMsg << "Card(): suit '" << suit << "' out of range";
                    throw std::range_error( errMsg.str() );
            }

            m_rank = rank;
            m_suit = suit;
    }

    int rank() const { return m_rank; }
    int suit() const { return m_suit; }
    std::string str( bool useStrings = false ) const
    {
            std::stringstream ret;
            if( useStrings )
                    ret << sRanks[m_rank] << " of " << sSuits[m_suit];
            else
                    ret << cRanks[m_rank] << cSuits[m_suit];
            return ret.str();
    }

private:
    static const char cRanks[14];
    static const char cSuits[4]
    static const std::string sRanks[14];
    static const std::string sSuits[4];

    int m_rank;
    int m_suit;
};

const std::string Card::sSuits[4] = {"Spades", "Hearts", "Diamonds", "Clubs"};
const std::string Card::sRanks[14] = { "\0", "Ace", "Two", "Three", "Four", "Five",
                    "Six", "Seven", "Eight", "Nine", "Ten", "Jack", "Queen", "King" };
const char Card::cSuits[4] = { 's', 'h', 'd', 'c' };
const char Card::cRanks[14] = { '\0', 'A', '2', '3', '4', '5', '6', '7',
                                '8', '9', 'T', 'J', 'Q', 'K' };

The client can create a card either using the Card enum constants or integers, like

Card card(Card::Ace, Card::Diamonds);
Card card2(0, 3); // throws an exception

I'm dubious on how useful the public enum constants are for client code. Also, I'm very new to exception programming and I'm not sure if this is a good approach or whether the constructor should throw an exception. Any improvements or suggestions?

\$\endgroup\$
6
\$\begingroup\$

If you are going to go to the trouble of creating Rank and Suit enum then use them.

Card( int rank, int suit ) throw(std::range_error)

I would have done

Card(Rank rank,Suit suit )

That way you can't accidentally go: Card(Card::Hearts,2) The compiler type checking will catch this at compiletime. Just use the enums all through your code. C++ is a strongly typed language use the compiler to catch your errors.

The exception specifications have been deprecated. They were mostly useless anyway (if you fail and throw an illegal exception it causes your application to terminate without unwinding the stack). So in general don't use them. For the one case where they are usfull (no throw specifications) they are good but I use them more as documentation to remind me that I should make sure the method does not throw.

But yes the constructor should throw an exception if it gets a value outside the expected range. There is no point in allowing an invalid object to propagate threw your code it will just cause problems with your invariants.

Prefer to use the initializer list rather than initializing variables in the constructor.

Card(Rank rank, Suit suit)
    : m_rank(rank)
    , m_suit(suit)

Again another place that it would have been nice to use the enum to make the code more eadable:

        if( rank < 1 || rank > 13 ) {

Try:

        if( rank < Ace || rank > King ) {

Hate getters/setter they spoil encapsulation. Do you really need to get the values of the card? Any operation that manipulates the card is usually a method on the card or a friendly class.

int rank() const { return m_rank; }
int suit() const { return m_suit; }

Don't try and write C++ like Java. We don;t need a string function. It is much more preferable (and flexable) to write an output (and probably input) stream operator.

std::string str() const
{
        return std::string( Ranks[m_rank] + Suits[m_suit] );
}

I would have done:

friend std::ostream& operator<<(std::ostream& stream, Card const& card)
{
    return stream << card.Ranks[card.m_rank] << card.Suits[card.m_suit];
}

Adding Cards to a deck. (Based on comment below)

#include <vector>

class Card
{
    public:
        enum Ranks { Ace = 1, Ten = 10, Jack, Queen, King };
        enum Suits { Spades, Hearts, Diamonds, Clubs };

        Card(Ranks rank, Suits suit){}
};

// Need to add the ++ operators for Ranks/Suits
Card::Ranks& operator++(Card::Ranks& r)
{
    return r = Card::Ranks(static_cast<int>(r)+1);
}
Card::Suits& operator++(Card::Suits& s)
{
    return s = Card::Suits(static_cast<int>(s)+1);
}



int main()
{
    std::vector<Card>   deck;

    // Does not look that verbose to me.
    for(Card::Ranks rank=Card::Ace;rank <= Card::King;++rank)
    {
        for(Card::Suits suit=Card::Spades;suit <= Card::Clubs;++suit)
        {
            deck.push_back(Card(rank, suit));
        }
    }

    // Compared to 
    for(int rank=Card::Ace;rank <= Card::King;++rank)
    {
        for(int suit=Card::Spades;suit <= Card::Clubs;++suit)
        {
            deck.push_back(Card(rank, suit));
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ regarding the initializer list, what benefit does it give? I know that members have their values set in the initializer list before the constructor body runs, but is there any other benefit? \$\endgroup\$ – weevil Mar 3 '12 at 23:10
  • \$\begingroup\$ Yes. If the objects are class objects then there constructors are run. So if you don't use the initializer list then you will run the constructor to initialize them then you will run the assignment operator to put the value you want in them. Which is a bit impractical. \$\endgroup\$ – Martin York Mar 3 '12 at 23:21
  • \$\begingroup\$ But you may argue my objects are POD and have no constructor. But I would counter argue with 2 points: A) Half of coding is being consistent so you should treat all members the same otherwise you initialize half in one place and half in another. B) What happens if somebody changes the type from enum Suit to class Suit. Now you need to modify your code to make sure it works correctly with a class. While changing the type of an object should really require no code changes otherwise maintaining the code becomes much harder than it needs to be. \$\endgroup\$ – Martin York Mar 3 '12 at 23:22
  • \$\begingroup\$ regarding the use of enums as constructor parameters, I didn't want to limit the client to having to use them. Like if a Deck class needs to build a collection of cards by iterating through a range of suits and ranks, this would be impossible as far as I know due to casting an int argument to an enum parameter, but also verbose and somewhat nonsensical in the case of suits: for( int rank = Card::Ace; rank <= Card::King; ++rank ) for( int suit = Card::Spades; suit <= Card::Clubs; suit++ ) { // add card to collection \$\endgroup\$ – weevil Mar 3 '12 at 23:25
  • \$\begingroup\$ Impossible that is without casting rank back to type Ranks and suit to Suits, which seems like we're creating too much complexity and burden to warrant the type safety \$\endgroup\$ – weevil Mar 3 '12 at 23:44
0
\$\begingroup\$

I cleaned it up a bit taking most of Loki's advice, though I'm not sure about the public getter functions not being a good approach here. In the Blackjack program I'm making, and in most card games, your program logic will want to get the rank or suit of the card in question, not just print a string representation of the card. And the client probably shouldn't be able to change the value of the card, so there's no setter function. If a client has say a Hand class that represents a Blackjack hand, and needs to keep a cumulative point total of card ranks, I don't see another way to do this other than declaring Hand a friend inside Card and removing the getter functions, which seems like a poor approach.

One last question I have would be how I could create my own manipulator so that my overloaded stream operator could format its output in a variable way, like

std::cout << print_as_phrase << card << std::endl;

where print_as_string is a manipulator that causes my overloaded operator to return the commented out option in my code below. Right now I can only think of adding a function to card like void print_as_phrase( bool printPhrase = true ); to set a Card data member and have my overloaded friend operator function check that, but that seems more cumbersome than the original str() method I was using.

#include <iostream>
#include <string>
#include <stdexcept>
#include <sstream>
#include <vector>

class Card
{
public:
    enum Rank {
        Ace = 1, Two, Three, Four, Five, Six, Seven,
        Eight, Nine, Ten, Jack, Queen, King, RankEnd
    };
    enum Suit { Spades, Hearts, Diamonds, Clubs, SuitEnd };

    Card( Rank rank, Suit suit )
        : m_rank(rank), m_suit(suit)
    {
        if( rank < Ace || rank > King ) {
            std::stringstream errMsg;
            errMsg << "Card(): rank '" << rank << "' out of range";
            throw std::range_error( errMsg.str() );
        } else if( suit < Spades || suit > Clubs ) {
            std::stringstream errMsg;
            errMsg << "Card(): suit '" << suit << "' out of range";
            throw std::range_error( errMsg.str() );
        }
    }

    Rank rank() const { return m_rank; }
    Suit suit() const { return m_suit; }
    std::string str() const
    {
        std::stringstream ret;
        ret << sRanks[m_rank] << " of " << sSuits[m_suit];
        return ret.str();
    }

    friend std::ostream& operator<<( std::ostream &os, const Card &card )
    {
        return os << card.cRanks[card.m_rank] << card.cSuits[card.m_suit];
    //  return os << card.sRanks[card.m_rank] << " of " << card.sSuits[card.m_suit];
    }

private:
    static const std::string sRanks[14];
    static const std::string sSuits[4];
    static const char cRanks[14];
    static const char cSuits[4];
    Rank m_rank;
    Suit m_suit;
};

const std::string Card::sRanks[14] = {
    "\0", "Ace", "Two", "Three", "Four", "Five", "Six", "Seven",
    "Eight", "Nine", "Ten", "Jack", "Queen", "King"
};
const std::string Card::sSuits[4] = { "Spades", "Hearts", "Diamonds", "Clubs" };
const char Card::cRanks[14] = {
    '\0', 'A', '2', '3', '4', '5', '6', '7',
    '8', '9', 'T', 'J', 'Q', 'K'
};
const char Card::cSuits[4] = { 's', 'h', 'd', 'c' };


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

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


int main()
{
    std::vector<Card> deck;
    for( Card::Suit suit = Card::Spades; suit < Card::SuitEnd; ++suit )
        for( Card::Rank rank = Card::Ace; rank < Card::RankEnd; ++rank ) {
            deck.push_back( Card(rank, suit) );
            std::cout << deck.back() << "\n";
        }

    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Note: The return 0 at the end of main() (in C++ only) is optional. If left out it implies a successful return value. \$\endgroup\$ – Martin York Mar 4 '12 at 6:13
  • \$\begingroup\$ If you want to write a stream manipulator that affects how the card is printed. Then you need to look up xalloc. This allows you to store information in the stream that can then be subsequently used by output stream operator to determine formatting. \$\endgroup\$ – Martin York Mar 4 '12 at 19:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.