6
\$\begingroup\$

Previous review of this project:

Deck of cards program using a card value map

Based on the previous feedback I've received, my program could become more flexible (it formerly created a standard 52-card deck by itself). Now, the user can add Cards to a Deck, and Card is no longer hidden. I like this approach better (also easier to work with), but I'm sure it can still be improved. Regarding its use, I have a few questions:

  1. Although accessors aren't always recommended, would they still be necessary for allowing the driver to know each Card value/rank/suit (unless they're maintained elsewhere)? For instance, Blackjack requires card numerical values for determining a bust.

  2. Is the exception-handling in deal() effective (when called on an empty Deck), although it'll still need to return a Card? This is assuming that the user will not always call empty() before calling deal().

  3. Is this particular approach too flexible? If so, what could be done instead?

Deck.h

#ifndef DECK_H
#define DECK_H

#include <iostream>
#include <cstdint>
#include <vector>

class Card
{
private:
    unsigned value;
    char rank;
    char suit;

public:
    Card(unsigned v, char r, char s) : value(v), rank(r), suit(s) {}
    friend std::ostream& operator<<(std::ostream &out, const Card &card)
        {return out << '[' << card.rank << card.suit << ']';}
};

class Deck
{
private:
    std::vector<Card> cards;
    int topCardPos;

public:
    Deck();
    void addCard(unsigned value, char rank, char suit);
    void shuffle();
    Card deal();
    std::size_t size() const {return topCardPos+1;}
    bool empty() const {return topCardPos == -1;}
    friend std::ostream& operator<<(std::ostream &out, const Deck &deck);
};

#endif

Deck.cpp

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

Deck::Deck() : topCardPos(-1) {}

void Deck::shuffle()
{
    topCardPos = cards.size()-1;
    std::random_shuffle(cards.begin(), cards.end());
}

void Deck::addCard(unsigned value, char rank, char suit)
{
    cards.push_back(Card(value, rank, suit));
    topCardPos++;
}

Card Deck::deal()
{
    try
    {
        topCardPos--;
        return cards.at(topCardPos+1);
    }
    catch (const std::out_of_range &oor)
    {
        std::cerr << "\nDECK DEAL ERROR: " << oor.what() << "\n";
        Card blankCard(0, '*', '*');
        return blankCard;
    }
}

std::ostream& operator<<(std::ostream &out, const Deck &deck)
{
    for (unsigned i = 0; i < deck.size(); ++i)
    {
        out << "\n" << deck.cards[i];
    }

    return out;
}

(Possible driver)

Deck deck;

std::array<char, 13> RANKS = {'A','2','3','4','5','6','7','8','9','T','J','Q','K'};
std::array<char, 4> SUITS = {'H','D','C','S'};

for (unsigned rank = 0; rank < RANKS.size(); ++rank)
{
    unsigned value = (rank < 10) ? rank+1 : 10;

    for (unsigned suit = 0; suit < SUITS.size(); ++suit)
    {
        deck.addCard(value, RANKS[rank], SUITS[suit]);
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ value seems redundant. it could just be rank<10?rank:10 \$\endgroup\$ – Ray Tayek Jun 19 '13 at 4:23
  • \$\begingroup\$ Ah, a ternary statement. Haven't thought of that. Thanks! \$\endgroup\$ – Jamal Jun 19 '13 at 4:29
7
\$\begingroup\$

1: Yes, your Card class should expose its rank and suit through a public interface. (See the previous thread and @user225770's answer for value.) It should at least give read-only access, but write access may not be necessary. (It is probably better to create a copy than to alter a card.)

The simplest indication of this is your output stream operator overload. The fact that it needs access to these members is a good indication that other client code (i.e. code using this class) will need access as well.

2: It depends, but in either case it should be modified a bit. Exceptions are used for exceptional circumstances, scenarios that you don't expect to run into a lot. If it's rare that your deck runs out of cards, then throwing an exception is fine. However, you shouldn't throw an exception for your own function. Let the exception propagate to the calling code, and let them decide how to handle the exception.

However, if it's a common occurrence that your deck runs out of cards, you should use another approach. What immediately comes to mind is to simply reduce the chance of this happening by checking empty() before calling deal(). You can verify this behavior by adding an assert at the beginning of the function.

(What you are technically doing is defining a precondition for your function. In short, this means that your function requires and assumes that the deck is not empty when deal() is called.)

3: No, I don't think this solution is over-engineered. However, a more interesting question is "How can I tell if I have over-engineered my solution?". Why, thank you for asking! This leads me to my next, and perhaps most important point:

Start doing unit tests. You are writing good code. The best way for you to improve now is by doing test-driven development. By writing unit tests, several of your questions will answer itself. For example: Should my class expose this member through its interface? Unit tests will answer this question for you by letting you know what you need access to to test and use your code in unit tests.

Test-driven development will let you know if your code is over-engineered. You write a test that works as a requirement to your code. Do you need to do X to make the test pass? Then implement it. Don't you? Then You Ain't Gonna Need It (yet).

\$\endgroup\$
  • \$\begingroup\$ The chances of my deck running out of cards should be unlikely, but I also want to properly handle such a situation anyway. I can still do some other kind of check within the function, and I know I will always call empty() in my own program (unless of course I'm testing the class). I also forgot to mention, regarding the previous review, that I could also create a function that creates a classic 52-card deck. However, I'll be back at square one with this approach since I still haven't been able to do it free of underlying problems. \$\endgroup\$ – Jamal Jun 19 '13 at 17:23
  • \$\begingroup\$ In that case, your deal() function should throw an exception when used incorrectly. std::vector::at() will already do this for you, so deal() can simply be return cards.at(topCardPos--);. Then, depending on your error handling strategy, you could either handle the exception in client code, or you can let it propagate to main() and terminate the program. \$\endgroup\$ – Lstor Jun 19 '13 at 23:21
  • \$\begingroup\$ For some reason, Visual C++ can't catch my exception. I have to install Eclipse for my class anyway, so I'll try it on there. \$\endgroup\$ – Jamal Jun 19 '13 at 23:28
  • \$\begingroup\$ What do you mean? If the exception is propagates out of main(), the program should be terminated. A somewhat better approach would be to have a try/catch in main() or another entry-level function where you catch the exception and terminate gracefully. \$\endgroup\$ – Lstor Jun 19 '13 at 23:43
  • \$\begingroup\$ I really don't know exactly what's going on with my exception-handling. I'm just going to leave it out for now. \$\endgroup\$ – Jamal Jun 20 '13 at 2:21
5
\$\begingroup\$

I would personnally not associate a card to its rank. I know a number of card games, and rank of card is a very variable thing.

As so, I think the rank of a card is not a property of the Card class, but should be elsewhere, associated to a Game kind of class.

Else, I think that Lstor answer is really good. Unit tests are a fundamatenal piece of code, go for it :)

\$\endgroup\$
  • 1
    \$\begingroup\$ +1, and welcome to CodeReview.StackExchange! I fully agree regarding rank (as I pointed out in the previous thread). \$\endgroup\$ – Lstor Jun 19 '13 at 12:58
  • \$\begingroup\$ I'm not sure I quite understand (I don't know a whole lot of card games anyway). If rank shouldn't be a member of Card, then how would it be able to know what to output for rank? Please show me some example code if it will help explain it. \$\endgroup\$ – Jamal Jun 19 '13 at 18:00
  • 1
    \$\begingroup\$ I have confused rank and value. I think that value should not be part of the class, whereas rank should. Create a map, function mapping or something else in order to achieve this. The most natural choice would be to have a Game class with a member function along the lines of Value valueToRank(Card const&);. \$\endgroup\$ – Lstor Jun 19 '13 at 23:17
  • \$\begingroup\$ Ah, okay. That makes more sense. :-) I'll take out the value data member and put such an std::map in my driver for now. \$\endgroup\$ – Jamal Jun 20 '13 at 2:24

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.