Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I would like to know whether I'm going in a correct way or not in building my Poker game. So far, I've implemented card and deck classes and I would like to see your feedback about my work. Feel free to criticize the code of any regard (organization, order, comments ... etc)

card.h

#ifndef CARD_H
#define CARD_H

#include <string>

const int SUIT_MAX(4);
const int RANK_MAX(13);

class Card
{
    friend class Deck; // Deck Class needs to access to Card Class but not vice versa
public:
    explicit Card();
    explicit Card(const int &suit, const int &rank);

    std::string Card2Str() const;

private:
    int generate_suit();
    int generate_rank();
    int get_suit() const;
    int get_rank() const;
    int m_suit;
    int m_rank;
};
#endif

card.cpp

#include <stdlib.h>     /* srand, rand */
#include "card.h"
#include <iostream>


const std::string SUIT[SUIT_MAX]  = {"S", "H", "D", "C"};
const std::string RANK[RANK_MAX]  = {"2","3","4","5","6","7","8","9","10","J","Q","K","A"};


Card::Card()
{
   m_suit = generate_suit(); 
   m_rank = generate_rank();
}

Card::Card(const int &suit, const int &rank) : m_suit(suit), m_rank(rank)
{

}

int Card::generate_suit()
{
    return rand() % (SUIT_MAX-1) + 0;
}

int Card::generate_rank()
{
    return rand() % (RANK_MAX-1) + 0;
}

std::string Card::Card2Str() const
{
    return SUIT[get_suit()] + RANK[get_rank()];
}

int Card::get_suit() const
{
    return m_suit;
}

int Card::get_rank() const
{
    return m_rank;
}

deck.h

#ifndef DECK_H
#define DECK_H

#include <vector>
#include <iostream>
#include <fstream>
#include "card.h"

using namespace std;

class Deck
{ 
public:
      explicit Deck();
      void print_Deck() const;
      void getOneCard();
private:
    std::vector<Card> m_deck;

};

#endif

deck.cpp

#include <iostream>
#include "deck.h"


Deck::Deck()
{
    for (unsigned int i(0); i < SUIT_MAX; ++i)
    {
        for (unsigned int j(0); j < RANK_MAX; ++j)
        {
            Card card(i, j);
            m_deck.push_back(card);
        }
    }
}


void Deck::print_Deck() const
{
    unsigned int count(1);

    for (unsigned int i(0); i < m_deck.size(); ++i)
    {
        std::cout << m_deck[i].Card2Str() << " ";
        if ( count == 13 )
        {
          std::cout << std::endl;
          count = 0;
        }
        ++count;
    }
}

void Deck::getOneCard()
{   
    Card cd(m_deck.back().get_suit(), m_deck.back().get_rank());
    m_deck.pop_back();
    std::cout << cd.Card2Str() << std::endl;
}

main.cpp

#include <iostream>
#include <vector>
#include <stdlib.h>     /* srand, rand */
#include <time.h>       /* time */
#include <string>

#include "card.h"
#include "deck.h"

int main()
{
    srand (time(NULL));

    Deck _deck; 
    _deck.print_Deck();
    _deck.getOneCard();

    std::cout << std::endl;
    _deck.print_Deck();


    std::cout << std::endl;

    return 0;
}
share|improve this question
    
I happen to be personally interested in playing card representations in C++. You may take a look at my latest posted version here for some ideas. –  Jamal Feb 16 at 17:48
    
@Jamal, thank you for the link. –  CroCo Feb 17 at 14:41
add comment

4 Answers 4

  • Prefer nullptr to NULL if you're using C++11.

  • <stdlib.h> and time.h are both C libraries. Use the respective C++ libraries <cstdlib> and <ctime>. The rest are okay. You don't need to include <vector> in main.cpp.

  • Since you use a storage container for your deck, you can display it using iterators:

    for (auto iter = m_deck.begin(); iter != m_deck.cend(); ++iter)
    {
        std::cout << *iter << "\n";
    }
    

    If you're using C++11, use a range-based for-loop:

    for (auto& iter : m_deck)
    {
        std::cout << iter << "\n";
    }
    
  • I don't know why you're using randomness to generate a card within the class. The constructor should be given input to determine its rank and suit. A card shouldn't create itself, and you wouldn't want this with an actual card game. Remove the generate functions entirely, and instead allow the constructor to receive arguments to determine the card's value.

  • You also don't need to construct the deck within its constructor. A deck can still start out empty until cards are passed to it. That should be handled in client code, especially if someone doesn't want a 52-card deck (or a certain game uses something different).

  • As an alternative to using std::strings, consider enums for the ranks and suits:

    // make sure the first rank is explicitly set at 1
    // this will make the last rank equal 13
    enum Rank { Ace=1, Two, /* ... */, Queen, King };
    

    enum Suit { Spades, Hearts, Diamonds, Clubs };
    

    With this, you'll no longer need the arrays nor the max constants. Your m_rank and m_suit members should also be one of these enum type (Rank and Suit respectively). The enums and the Card class declaration should all then be contained in a namespace.

    Within your card class, you'll need functions to convert a given enum value to a char value for displaying. You could, for instance, do this with a switch statement, which will serve as a look-up table. This will also make it easier to maintain the ranks and suits.

    std::string getRank(Rank rank)
    {
        switch (rank)
        {
            case Ace: return "A";
            // ...
            default: throw std::logic_error("invalid rank");
        }
    }
    
    // same idea with suits
    

    Also note the default statement. If an invalid rank is passed in, an std::log_error exception will be thrown. Regardless of what works best for error-handling, there should be a default statement here to properly handle it.

  • Rename Card2Str() to CardToString(). The former is just awkward wording. It should also be lowercase as it's a function and not a type.

  • getOneCard() doesn't specify where in the deck the one card is drawn. Since it takes a card from the top of the deck, just call it draw() or deal(). On the other hand, it's void and has no parameters, so it's not returning a card. If this is supposed to just display a top card, then it's ineffective as it's still destroying the top card.

    Better yet, consider having a draw() and a top() (perhaps you just need to peek at the top card). The functionality also looks a bit unclear. Since you're using std::vector, a storage container, make use of its member functions. In this case, use back():

    draw():

    Card Deck::draw()
    {   
        Card cd = m_deck.back();
        m_deck.pop_back();
        return cd;
    }
    

    top():

    // return by const& as this Card should only be read-only
    // also make the function const as no data members are modified
    Card const& Deck::top() const
    {   
        return m_deck.back();
    }
    

    For both of these functions, be sure the caller is first checking for an empty deck.

    Side-note: You can use a different storage container if you, say, need to be able to insert into or draw from somewhere else in the deck besides the top or bottom. For that, you can use an std::list (doubly linked list). If you instead want to insert into or draw from the top and bottom, you can use an std::deque (double-ended queue). Otherwise, stay with what you have.

  • Your deck is missing something: a shuffle function! You could utilize std::random_shuffle from the STL to do it nicely for you:

    void Deck::shuffle()
    {
        std::random_shuffle(m_deck.begin(), m_deck.end());
    }
    

    You should also check for an empty deck prior to shuffling.

share|improve this answer
    
I think it would be very awkward to program if ranks/suits were enums; IMO giving up the arrays for switch statements everywhere would make most things worse, not better. –  Hurkyl Feb 16 at 23:45
    
@Hurkyl: It was based on a suggestion given to me a number of times. Even without the enums, it would make little sense for those members to be ints. With enums, you have a more accurate type. Even with the arrays, you would have to make sure it doesn't go out of bounds. –  Jamal Feb 16 at 23:55
    
@Jamal, thank you so much for this invaluable info. About Card2Str, actually I'm chosen 2 over To because I use Matlab and this shortcut is common in some functions. Also, I totally agree with "Hurkyl" that using enum makes the code a little longer, so arrays would be a better choice with full regard of your opinion. About using iterators, I think it would be a good idea. About shuffling, I think Dealer Class is in charge of shuffling, correct me if I'm wrong. About top, it seems a good function. –  CroCo Feb 17 at 14:36
    
About getOneCard(), I think I need to move this function to Dealer Class, since Deck Class does no action itself. I mean Deck Class should only store cards nothing else. This is my understanding. –  CroCo Feb 17 at 14:43
    
About top(), shouldn't be a function in Dealer Class? –  CroCo Feb 17 at 14:55
show 4 more comments

I come from Java world, but here are just a few thoughts:

  • generate_suit and generate_rank methods should probably not subtract 1 from MAXes and also adding zero at the end is unnecessary. E.g. rand() % (SUIT_MAX-1) is rand() % 3 and this could never be 3, so you will never have generated clubs ♣.
  • Friending Card class by Deck class is not necessary. Just make get_suit and get_rank methods public and/or create a copy constructor of the Card class and/or rewrite getOneCard method like this:

    void Deck::getOneCard()
    {   
        std::cout << m_deck.back().Card2Str() << std::endl;
        m_deck.pop_back();        
    }
    
  • Card::Card() constructor doesn't seem used now, so consider deleting it along with both generate* methods.
share|improve this answer
    
thanks for the feedback. For the rand(), it is a good point and I'll keep this in mind. About Card::Card(), basically I've implemented that because in case the user wants to see a mere card without considering Deck or any other class. It is just an extra info I guess, so I guess there is no harm of keeping it with full regard of your feedback. About friend Class, I'm not certain but it seems to me it should be there because Card should have its own data private, keeping everything tight and secure. –  CroCo Feb 16 at 16:57
1  
@CroCo I think friending is not necessary here, so it violates Card's encapsulation without a purpose. Even if it was necessary for some other case, why would you want to hide get_suit and get_rank methods of Card when it already indirectly exposes them via the Card2Str method (metaphorically speaking)? If public audience already knows what Card I am (through Card2Str method), why hide my suit or rank getters? –  Maros Feb 16 at 17:12
    
I'm convinced now. Thank you so much. –  CroCo Feb 17 at 14:40
add comment

The Card constructor is unnecessarily complicated:

explicit Card(const int &suit, const int &rank)

Why not just take ints?

explicit Card(int suit, int rank)

There's not much that can be done with a Card, except to convert it into a string. What about comparing with another card for equality? How about comparing with another card by rank? That's impossible unless you stringify it and parse the string. The fact that Deck needs to be a friend is a red flag.


The Card class has a Card2Str() method. That's awkward, since the "Card" part of the name is redundant. In contrast, Deck has a print_Deck() method. Why the inconsistency in capitalization and in functionality? Neither method is idiomatic C++, which would provide an operator<< for std::ostream&. The ostream approach is also more flexible, since it is not constrained to write its output to std::cout. (You could write to a std::stringstream, for example, to obtain a string.)


Deck::getOneCard() is not useful, since it returns void and prints its result to std::cout instead. You want to return a Card, or more properly, a const Card& — and let the caller decide what to do with the card. The name is inappropriate, since "get…" has a connotation of having no side effect; drawCard() would be better. And if you can draw a card from the deck, there had better be a way to put it back at the bottom of the deck.

share|improve this answer
    
As for the very last point, I think it was you or someone else who told me that the user shouldn't have to worry about returning cards like that (although std::deque works with that). Other methods may include having two storage containers (what I have now) or having an iterator keeping track of the top of the deck. –  Jamal Feb 18 at 2:08
    
@200_success, what's wrong with passing by a constant reference? About overloading cout, it is a good idea. I will put this in mind. Even though I've implemented getOneCard() for only testing, I believe the Dealer Class is in charge of drawing cards. I'm picturing Deck Class as a bunch of cards nothing else which reflects the reality. –  CroCo Feb 18 at 23:09
    
Passing const int& will work too. It's just that int is more straightforward, so why not simplify? –  200_success Feb 18 at 23:28
1  
What's the point of a Deck that's just a vector<Card> with a print method? You might as well get rid of the Deck class and have the Dealer hold a vector<Card>. At that point, you've just renamed Deck to Dealer. At the end of the day, you're still missing some functionality to manipulate that collection of cards. –  200_success Feb 18 at 23:31
    
@200_success: Precisely. There are some things that Deck should and shouldn't do. Drawing and shuffling, for instance, should be available so that a player can use them on the deck. A dealer is just someone who uses the deck, but is not the deck. –  Jamal Feb 19 at 4:08
show 2 more comments

This is probably overkill for a poker program, but if you wanted to reuse your Deck class for card games other than poker you'll need to decouple it from your Card class.

In general, a Deck of cards is similar for all card games regardless of what type of cards are being used. Decks can be shuffled, cards can be drawn, etc.

Cards however can be poker cards, pinochle cards, memory game cards, UNO cards, or many more, not all have suits or ranks, though they all may have some way to print or display whatever values they do contain.

With that in mind, if you plan on making other card games besides poker, consider making an ICard abstract interface with some type of pure virtual display or print method, and having your Deck class depend on the interface instead of the Card class directly. Then you'll be able to perform Deck actions on any type of card for any type of card game you can dream up in the future. It will also mean that Deck should not be friends with any concrete Card class.

This added complexity though is not necessary if you only plan to make a poker game.

share|improve this answer
    
Now that's my kind of approach! A big +1 for you! –  Simon André Forsberg Feb 18 at 17:25
1  
@YoungJohn, I don't want to make so complicated. What I'm trying to do is building only one type of Poker games. This allows me to see how the procedure flows, after that I'll update the game to be more flexible. I know this is not good approach, however I think it is good for beginners. Thanks for the advice though. –  CroCo Feb 18 at 23:16
    
Don't even worry about it, it is a common trade-off to choose between making code less complex vs more generic. Your version is less complex and if that is enough for your application then it is absolutely the correct thing to do. –  YoungJohn Feb 18 at 23:30
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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