A Deck of Cards Review

[Ispil]

I just finished with making a simple deck of cards... did I do anything in particular in a strange or unacceptable way, or is this otherwise-good code?

main.cpp

#include <iostream>
#include <ctime>
#include <cstdlib>
#include "Deck.h"

/// Code for a deck of cards using generic deck files; user can input number of cards to be drawn.

int main()
{
    srand(time(NULL));
    Deck A;
    A.Menu();
    return 0;
}

Deck.h

#ifndef DECK_H
#define DECK_H
#include <iostream>

/// The generic deck header (alter for other uses)

class Deck
{
    static const int numSuits = 4;     //a standard deck of cards has four suits
    static const int numCards = 13;    //and 13 card types. Can be changed if you wanted to play Uno or somesuch

    std::string suits[numSuits]= {"hearts","spades","clubs","diamonds"};   //the array of suits
    std::string cards[numCards]= {"ace","two","three","four",
                                  "five","six","seven","eight",
                                  "nine","ten","jack","queen","king"};    //the array pf card values
    bool isdrawn[numCards][numSuits];        //an array to keep track of whether the cards are drawn

    void Printcard(int, int);   //prints the cards drawn
    int getrandom(int);         //produces a random number
    bool Sortdeck();            //determines whether the deck has been entirely drawn or not
    void Checkdeck();           //check whether a card is drawn
    void Shuffle();             //if the whole deck is drawn, set all of isdrawn to false
    void Drawcard(int);         //draw the card, marking it off in isdrawn

    public:
    void Menu();
};

#endif // DECK_H 

Deck.cpp

#include "Deck.h"
#include <iostream>
#include <ctime>
#include <cstdlib>

/// The generic deck files (alter for other uses)

int Deck::getrandom(int n)
{
    return rand() % n;
}

void Deck::Drawcard(int number)
{
    int a,b;

    while(number > 0)
    {
        a = getrandom(numCards);    //get a random card type
        b = getrandom(numSuits);    //and a random suit
        if(!isdrawn[a][b])          //if it's not already drawn
        {
            Printcard(a,b);         //print the value
            isdrawn[a][b]=true;     //and mark it off as true
            number--;               //then go again with one card less
        }
        else                        //if the card was drawn before
            Checkdeck();            //see whether there's any other valid cards left
    }
    return;
}

void Deck::Printcard(int card, int suit)
{
    if(card==0 || card ==7)//if it starts with a vowel (assuming generic deck)
        std::cout << "You drew an " << cards[card] << " of " << suits[suit] << ".\n";
    else                   //otherwise
        std::cout << "You drew a " << cards[card] << " of " << suits[suit] << ".\n";
    return;
}

bool Deck::Sortdeck()
{
    bool a=false;
    for(int n=0; n<numSuits; n++)
    {
        for(int i=0; i<numCards; i++)
        {
           if(!isdrawn[i][n])       //if there's a single card in the deck not drawn
                a=true;             //make a note of it in a
        }
    }
    return a;                       //then return a
}

void Deck::Checkdeck()
{
    if(!Sortdeck())                 //if there are no cards left
    {
        Shuffle();                  //shuffle
    }
    return;
}

void Deck::Shuffle()
{
    std::cout << "Shuffling...\n";
    for(int n=0;n<numSuits;n++)
        for(int i=0;i<numCards;i++)
        {
            isdrawn[i][n]=false;    //sets the isdrawn array to be false
        }
    return;
}

void Deck::Menu()
{
    unsigned int amount = 1;
    while(amount > 0)          //this statement also prevents non-integer input from going through
    {
        std::cout << "How many cards would you like to draw? Type \"0\" to exit.\n";
        std::cin >> amount;    //if it's negative, the loop will just repeat itself
        Drawcard(amount);
    }
    return;
}

[cire]

I wouldn't expect a deck of cards to have a member function Menu.

[Ispil]

It's mainly to get around getting/setting. This is just a generic format- the menu can be removed when applied to other code. That's just as a way to see that it works, really.

[cire]

It's mainly to get around getting/setting. This is just a generic format- the menu can be removed when applied to other code. That's just as a way to see that it works, really.

Except that it doesn't work, really. If you remove the Menu function, the class has no public interface.

The public interface is probably the single most important thing to consider when designing a reusable class.

If I want to create a Deck, shuffle it and show the resulting order. Can I do that with your class?

[AbstractionAnon]

I agree with circe that menu doesn't belong in your class. menu is not an attribute of a deck of cards. menu is typically an attribute of a game. What you have in menu should really have been in your main. Had the code been in main, you would have discovered that Drawcard should have been public.

It's mainly to get around getting/setting

It a good to avoid getters and setters. If there's a need for getters and setters, then you probably don't have your public interface correct.

SortDeck is a misnomer. It doesn't do a sort. It's really telling you that you need to shuffle. Also, once you find a card that is not drawn, you should return true immediately. No reason to iterate through the rest of the deck.

If Drawcard is going to be general purpose, you need some way to return a card value and suit. Consider making a Card class. Value and suit are really attributes of a card. Printcard belongs in the Card class. Deck then becomes simply an array of 52 Cards. Drawcard should return a single Card object.

edit to add:
Printcard exposes the implementation of suits and values and does not check for valid values. This is another reason to implement Card as a class and hide the implementation of suits and values within Card.

[IceThatJaw]

Make a card class or struct. It will require extra code but it makes more sense in the end. You can go the easy route and use unsigned int's or create enum's.

enum Rank { ONE, TWO, THREE, ..... };
enum Suit { JACK, QUEEN, KING, ACE };

struct Card {
    Rank rank;
    Suit suit;
};

or

struct Card {
     unsigned int rank;
     unsigned int suit;
};

Your getRandom function is odd. Why do I have to pass in an int to get a random number? Obviously no numbers from a program are truly random but at least make that function create it's own randomness. Try seeding the time every time you call it.

It might be bad practice but you shouldn't have to pass in a number to get a pseudorandom number from an API.

Also, CheckDeck is a useless method. It doesn't do anything unique enough to deserve a call.

[Ispil]

Well, I have re-done the code for this with (most) of the proposed changes (I didn't make an enumeration because it's easier to perform card-based arithmetic (blackjack, as an example of where this is needed) with integer values). However, it appears that by calling the constructor in Drawcard for the structure Card, it is not being "deleted" per-se, albeit it is not dynamic. This result in a program lock whenever the number of cards drawn is equal to 24 (a rather odd number for it to lock at). Assuming that it is the fault in the constructor, could someone tell me what the destructor should look like for the Card structure? I'm not quite sure how to deal with this issue...

Deck.cpp

#include "Deck.h"
#include "Card.h"
#include <iostream>
#include <ctime>
#include <cstdlib>

/// The generic deck files (alter for other uses)

int Deck::getrandom(int n)
{
    return rand() % n;
}

Card Deck::Drawcard()
{
    unsigned int a,b;
    a = getrandom(numCards);
    b = getrandom(numSuits);
    while(isdrawn[a][b])
    {
        if(!AnyCardsLeft())
            Shuffle();
        a = getrandom(numCards);
        b = getrandom(numSuits);
    }
    Card drawnCard(a+1,b+1,cards[a],suits[b]);
    isdrawn[a][b] = true;
    return drawnCard;
}

bool Deck::AnyCardsLeft()
{
    bool a=false;
    for(int n=0; n<numSuits; n++)
    {
        for(int i=0; i<numCards; i++)
        {
           if(!isdrawn[i][n])       //if there's a single card in the deck not drawn
           {
                a=true;             //make a note of it in a
                break;              //then stop checking the rest
           }

        }
    }
    return a;                       //then return a
}

void Deck::Shuffle()
{
    std::cout << "Shuffling...\n";
    for(int n=0;n<numSuits;n++)
        for(int i=0;i<numCards;i++)
        {
            isdrawn[i][n]=false;    //sets the isdrawn array to be false
        }
    return;
}

main()

#include <iostream>
#include <ctime>
#include <cstdlib>
#include "Deck.h"
#include "Card.h"

/// Code for a deck of cards using generic deck files; user can input number of cards to be drawn.

int main()
{
    srand(time(NULL));
    Deck A;
    Card B;
    int CardstoDraw = 1;

    while(CardstoDraw)
    {
        std::cout << "How many cards would you like to draw? Press 0 to exit.\n";
        std::cin >> CardstoDraw;
        for(int i = CardstoDraw; i > 0; i--)
        {
            B = A.Drawcard();
            std::cout << "You drew a(n) " << B.Name << " of " << B.Suit << ".\n";
        }
    }

    return 0;
}

Deck.h

#ifndef DECK_H
#define DECK_H
#include <iostream>
#include "Card.h"

/// The generic deck header (alter for other uses)

class Deck
{
    static const int numSuits = 4;     //a standard deck of cards has four suits
    static const int numCards = 13;    //and 13 card types. Can be changed if you wanted to play Uno or somesuch

    std::string suits[numSuits]= {"hearts","spades","clubs","diamonds"};   //the array of suits
    std::string cards[numCards]= {"ace","two","three","four",
                                  "five","six","seven","eight",
                                  "nine","ten","jack","queen","king"};    //the array pf card values
    bool isdrawn[numCards][numSuits];        //an array to keep track of whether the cards are drawn

    int getrandom(int);         //produces a random number
    bool AnyCardsLeft();        //determines whether the deck has been entirely drawn or not
    void Shuffle();             //if the whole deck is drawn, set all of isdrawn to false

    public:
    Card Drawcard();            //draw the card, marking it off in isdrawn
};
#endif // DECK_H 

Card.h

#ifndef CARD_H
#define CARD_H

/// The generic card header (alter for own use)

struct Card
{
    std::string Name;   //the name of the card
    std::string Suit;   //the suit of the card
    unsigned int Value;          //the numerical value of the card
    unsigned int SuitValue;      //the numerical value of the suit of the card (for array purposes)

    Card operator = (Card A)
    {
        Name = A.Name;
        Suit = A.Suit;
        Value = A.Value;
        SuitValue = A.SuitValue;
        return *this;
    }
    Card(int v, int sv, std::string n, std::string s)
    {
        Value = v;
        SuitValue = sv;
        Name = n;
        Suit = s;
    }
    Card()
    {
        Value = 0;
        SuitValue = 0;
        Name = '\0';
        Suit = '\0';
    }
};

#endif // CARD_H 

EDIT: Did a few minor modifications (threw in some unsigned ints instead of ints where needed). Otherwise, same issue.

EDIT EDIT: Also, the getrandom(int) function requires a value passed to it because it gets a random number from 0 to whatever value is passed to it. If I need a truly-random number, I can just call rand() inside the function.

[Daleth]

Dunno bout your issue. When I ran your code, the program went into an infinite loop after 17 cards were drawn. By infinite loop, I mean the program stopped displaying messages but I could hear my laptop whirring faster as it struggled to complete the loops.

Some random tidbits:
-I don't think you need another function for generating a random number because it is so trivial and is localized to the class. I might understand if the user was entering the max value, but Deck::getrandom is isolated to the member functions.
-You don't really need a bool variable in Deck::AnyCardLeft(). You could replace lines 40 and 41 with return true and line 46 with false, since you only need at least one card to exist for the condition to be true.
-I find your Deck class interesting because it does not actually store any Card objects and depends on indices.

[Ispil]

Yep, that's the same issue- whatever is causing that infinite loop appears to originate with the fact that so many Card structures are created... and I can't figure out how to prevent it due to there being a lack of dynamic variables. As for the suggested changes, I'll add them to my actual code, but don't feel as if they're relevant to the issue at hand to warrant changing the code I posted.

[Daleth]

Did you try making your own Deck constructor to make sure everything was initialized correctly?

[Ispil]

Everything in Deck is initialized in the header. All of the other variables are iterators in for loops, or the random card/suit values which are initialized with every call to Drawcard. I'm confident that it's not an uninitialized-variable issue.

[Daleth]

I would disagree. I made a default constructor for Deck, initializing isdrawn and made a clean build. the program was able to draw more cards before failing. After fixing up the Card::operator=() function to stop making unnecessary copies, i.e. const reference, checking the memory address, and making the return type a reference, the program was able to draw another couple of cards.
I'm not really clear on why this is happening, but check over your code carefully again.

[Ispil]

Actually, just initializing isdrawn as a default constructor fixed the issue, it would appear.

[Ispil]

Alright, now I have the finalized code for the deck of cards. I moved shuffle to public in case someone wants to call it before the deck is empty (poker, et cetera), and did other minor changes. It still works perfectly.

Deck.cpp

#include "Deck.h"
#include "Card.h"
#include <iostream>
#include <ctime>
#include <cstdlib>

/// The generic deck files (alter for other uses)

Card Deck::Drawcard()
{
    unsigned int a = rand() % numCards;         //get a random card as a
    unsigned int b = rand() % numSuits;         //and suit as b
    while(isdrawn[a][b])                        //while the picked card is already drawn
    {
        if(!AnyCardsLeft())                     //if there are no cards left to draw
            Shuffle();                          //shuffle
        a = rand() % numCards;                  //then get a new card
        b = rand() % numSuits;                  //and suit
    }
    Card drawnCard(a+1,b+1,cards[a],suits[b]);  //create a card object for the chosen card
    isdrawn[a][b] = true;                       //mark it off in isdrawn
    return drawnCard;                           //and return the card
}

bool Deck::AnyCardsLeft()
{
    for(int n=0; n<numSuits; n++)
        for(int i=0; i<numCards; i++)
           if(!isdrawn[i][n])       //if there's a single card in the deck not drawn
                return true;        //then exit, returning true
    return false;                   //if not, return false
}

void Deck::Shuffle()
{
    std::cout << "Shuffling...\n";
    for(int n=0;n<numSuits;n++)
        for(int i=0;i<numCards;i++)
            isdrawn[i][n]=false;    //sets the isdrawn array to be false
    return;
}

Deck::Deck()                        //default constructor to prevent issues with isdrawn
{
    for(int n=0;n<numSuits;n++)
        for(int i=0;i<numCards;i++)
            isdrawn[i][n]=false;    //sets the isdrawn array to be false
}

main()

#include <iostream>
#include <ctime>
#include <cstdlib>
#include "Deck.h"
#include "Card.h"

/// Code for a deck of cards using generic deck files; user can input number of cards to be drawn.

int main()
{
    srand(time(NULL));
    Deck A;
    Card B;
    int CardstoDraw = 1;

    while(CardstoDraw)
    {
        std::cout << "How many cards would you like to draw? Press 0 to exit.\n";
        std::cin >> CardstoDraw;
        for(int i = CardstoDraw; i > 0; i--)
        {
            B = A.Drawcard();
            std::cout << "You drew a(n) " << B.Name << " of " << B.Suit << ".\n";
        }
    }

    return 0;
}

Deck.h

#ifndef DECK_H
#define DECK_H
#include <iostream>
#include "Card.h"

/// The generic deck header (alter for other uses)

using std::string;

class Deck
{
    static const int numSuits = 4;     //a standard deck of cards has four suits
    static const int numCards = 13;    //and 13 card types. Can be changed if you wanted to play Uno or somesuch

    string suits[numSuits]= {"hearts","spades","clubs","diamonds"};   //the array of suits
    string cards[numCards]= {"ace","two","three","four",
                                  "five","six","seven","eight",
                                  "nine","ten","jack","queen","king"};    //the array pf card values
    bool isdrawn[numCards][numSuits];        //an array to keep track of whether the cards are drawn

    bool AnyCardsLeft();        //determines whether the deck has been entirely drawn or not

    public:
    void Shuffle();             //if the whole deck is drawn, set all of isdrawn to false
    Card Drawcard();            //draw the card, marking it off in isdrawn
    Deck();                //constructor
};
#endif // DECK_H 

Card.h

#ifndef CARD_H
#define CARD_H

/// The generic card header (alter for own use)

using std::string;

struct Card
{
    string Name;            //the name of the card
    string Suit;            //the suit of the card
    unsigned int Value;          //the numerical value of the card
    unsigned int SuitValue;      //the numerical value of the suit of the card (for array purposes)

    Card operator = (Card A)                            //in the case of assignment between two, take the values of the right and assign to the left
    {
        Name = A.Name;
        Suit = A.Suit;
        Value = A.Value;
        SuitValue = A.SuitValue;
        return *this;
    }
    Card(int v, int sv, string n, string s)   //constructor in case of creating a card with pre-defiend values(Drawcard)
    {
        Name = n;
        Suit = s;
        Value = v;
        SuitValue = sv;
    }
    Card()                                              //default constructor, sets values to 0 and strings to terminating character
    {
        Name = '\0';
        Suit = '\0';
        Value = 0;
        SuitValue = 0;
    }
};

#endif // CARD_H 

EDIT: I fixed any comment-related issues (typos, bad spacing) in my copy of the code, but those are so minor that they won't be reflected here.