scrambling a deck of cards (c++)

[Zacariaz]

3 things:
1. The constructor is messy, how could it be done better?
2. The best way to scramble the cards?
3. Any suggestions, corrections etc.

Code: Select all

struct card {
  const char V,S;
  card *Next;
  card(char v, char s):Next(0),V(v),S(s) {}
};

class deck {
  card *Top;
public:
  deck();
  void scramble() {}
};

deck::deck() {
  static const char V[] = "A123456789TJQK";
  static const char S[] = "cdhs";
  card *temp;
  for(char s = 0; s < 4; s++) { // there has got to be a better way!
    for(char v = 0; v < 13; v++) {
      if(s == 0 && v == 0) {
        temp = new card(v,s);
        Top = temp;
      }
      else {
        temp->Next = new card(v,s);
        temp = temp->Next;
      }
    }
  }
}

Thanks

[os64dev]

If a deck has 4 times 13 = 52 cards then for a scramble function you could take two random numbers i and j both between 0 and (4*13) and swap the cards with location i and j and repeat this a process number of times (minimal 52).

Code: Select all

int randomRange(int lo, int hi) {
    return(((rand() * (hi-lo)) / RAND_MAX) + lo);
}

void scramble(deck * d) {
    int shuffles = randomRange(52, 100);
    while(shuffles-- > 0) {
        int i = randomRange(0, 52);
        int j = randomRange(0, 52);

        card tmp = d->cards[i];
        d->cards[i] = d->cards[j];
        d->cards[i] = tmp;
    }
}

so you deck class should have something like:

Code: Select all

class card {
   ...
}

class deck {
   card *   cards;
}

[Solar]

An array of char is equivalent to a string, i.e. you can initialize your arrays like this:

Code: Select all

char V[] = "A23456789TJQK";

Aside from that... excuse me if I slept through the last few card-related threads of yours and missed the point where you explained your reasoning, but the OO design looks seriously wrong to me:

1) A "card" object should represent a card, not a way to store multiple cards. IMHO, whether to use a linked list, STL container or whatever should be the decision of the client code, not the class itself.

2) You let the client (deck) modify the next-pointer of card objects? "Friend" declarations are evil...

3) You are defining arrays S and V private in class card, but pass integers to the card constructor?

4) Always make destructors virtual. Better yet, if you don't have anything to say in your destructor, don't write one, as the default one will do fine.

5) Magic numbers... why are the sets valued 5, 4, 3, 6 respectively? And the way you are handling those arrays means you will be indexing them with yet more magic numbers...

6) You are calling new() in Deck::Deck(), but your destructor does nothing: Memory leak.

7) Why creating the cards on the heap, anyway?

My alternate suggestion (changes in coding style are deliberate and part of the suggestion):

Code: Select all

#include <vector>

class Card {
    public:
        enum Value
        {
            Ace,
            Two,
            Three,
            Four,
            Five,
            Six,
            Seven,
            Eight,
            Nine,
            Ten,
            Jack,
            Queen,
            King,
            MAX_VALUE
        };

        enum Set
        {
            Spades,
            Hearts,
            Diamonds,
            Clubs,
            MAX_SET = 4
        };

        Card( char set, char value ) : mSet( set ), mValue( value ) {}

        static char const Set[] = "SHDC";
        static char const Value[] = "A23456789TJQK";

    private:
        char mSet;
        char mValue;
};

class Deck {
    public:
        Deck();
        void scramble();
    private:
        std::vector< Card > mCards;
};

Deck::Deck()
{
    for ( char s = 0; s != Card::MAX_SET; ++s ) {
        for ( char v = 0; v != Card::MAX_VALUE; ++v ) {
            mCards.push_back( Card( s, v ) );
        }
    }
}

Now I'll spend a couple of minutes on the <algorithm> docs and come back to you with a scramble() implementation.

[Solar]

Code: Select all

#include <algorithm>

void Deck::scramble()
{
    random_shuffle( mCards.begin(), mCards.end() );
}

There is a three-argument version of random_shuffle(), the third argument being a functor. This would allow you to implement your own random number generator.

[AndrewAPrice]

@SOLAR: Great idea. Then you can have functions such as AddDeck and RandomizeDeck.

[Solar]

The three great virtues of a programmer: Lazyness, Impatience, Hubris.

[Zacariaz]

An array of char is equivalent to a string, i.e. you can initialize your arrays like this:

Code: Select all

char V[] = "A23456789TJQK";

of course youre right...

Aside from that... excuse me if I slept through the last few card-related threads of yours and missed the point where you explained your reasoning, but the OO design looks seriously wrong to me:

1) A "card" object should represent a card, not a way to store multiple cards. IMHO, whether to use a linked list, STL container or whatever should be the decision of the client code, not the class itself.

yes, but somewhere i allso wrote that the math was to complex to do what i wanted to do, so instead i would do some trial statistics. For that it would be very nice to have a deck of cards.

2) You let the client (deck) modify the next-pointer of card objects? "Friend" declarations are evil...

how is that any different than a normal linked list?
you have a struct, in this case a class, and you add nodes to it by doing axcactly this, or am i completely wrong here?

3) You are defining arrays S and V private in class card, but pass integers to the card constructor?

the S and V in card are not arrays, thay are only to store value and suit of a single cards.

4) Always make destructors virtual. Better yet, if you don't have anything to say in your destructor, don't write one, as the default one will do fine.

of course...

5) Magic numbers... why are the sets valued 5, 4, 3i, 6 respectively? And the way you are handling those arrays means you will be indexing them with yet more magic numbers...

they are, atleast on my computer, a part of the ascii table representing spades, hearts, diamonds and clubs symbols.
The important thing to understand here is that V[] and S[] in deck in only used to ease outputting hte cards ag.
Ace of spades == As == 0,3
but i dont want to print 0,3 to the screen, As looks so much better so:
std::cout << V[0] << S[3];
outputs As
It is not a good solution, i know, but it was the only opne i could think of.

6) You are calling new() in Deck::Deck(), but your destructor does nothing: Memory leak.

is this simular to point 4.? if not i think it requires further explanation for me to understand it.

7) Why creating the cards on the heap, anyway?

My alternate suggestion (changes in coding style are deliberate and part of the suggestion):

Code: Select all

#include <vector>

class Card {
    public:
        enum Value
        {
            Ace,
            Two,
            Three,
            Four,
            Five,
            Six,
            Seven,
            Eight,
            Nine,
            Ten,
            Jack,
            Queen,
            King,
            MAX_VALUE
        };

        enum Set
        {
            Spades,
            Hearts,
            Diamonds,
            Clubs,
            MAX_SET = 4
        };

        Card( char set, char value ) : mSet( set ), mValue( value ) {}

        static char const Set[] = "SHDC";
        static char const Value[] = "A23456789TJQK";

    private:
        char mSet;
        char mValue;
};

class Deck {
    public:
        Deck();
        void scramble();
    private:
        std::vector< Card > mCards;
};

Deck::Deck()
{
    for ( char s = 0; s != Card::MAX_SET; ++s ) {
        for ( char v = 0; v != Card::MAX_VALUE; ++v ) {
            mCards.push_back( Card( s, v ) );
        }
    }
}

Now I'll spend a couple of minutes on the <algorithm> docs and come back to you with a scramble() implementation.

i appreciate your work, but at this time i dont really knwo what to say, ill get back to you.


I now that i have made so rather messy code here, reason not important, but i dont think it is very hard to understand what im trying to do and i do think this is the right approach for creating a deck of cards, which in this case is the only thing im trying to do.

[Solar]

1) A "card" object should represent a card, not a way to store multiple cards. IMHO, whether to use a linked list, STL container or whatever should be the decision of the client code, not the class itself.

yes, but somewhere i allso wrote that the math was to complex to do what i wanted to do, so instead i would do some trial statistics. For that it would be very nice to have a deck of cards.

This is about "thinking object-oriented". A class defines the characteristics of something. A card in real life has a value, and is of a set. A card does not have a pointer to another card. So, making a "class card" having such a pointer breaks the abstraction, and you should spend some time thinking about if you could do without (which you can, as I have pointed out in my code).

2) You let the client (deck) modify the next-pointer of card objects? "Friend" declarations are evil...

how is that any different than a normal linked list?

Again, this has to do with using the programming language "right". C++ is very flexible in that you can implement things in almost every way you like, but some are better suited to the language than others.

you have a struct, in this case a class, and you add nodes to it by doing axcactly this, or am i completely wrong here?

Correct, if you're doing a linked list in C. But a) this is C++, so handling of member pointers should be done by member functions, not "friends", and b) I seriously doubt a linked list is called for here. As long as you can "get away" using standard containers like <vector>, do so.

5) why are the sets valued 5, 4, 3i, 6 respectively?

they are, atleast on my computer, a part of the ascii table representing spades, hearts, diamonds and clubs symbols.

Ah, I see. Add this as a comment.

The important thing to understand here is that V[] and S[] in deck in only used to ease outputting hte cards ag.
Ace of spades == As == 0,3
but i dont want to print 0,3 to the screen, As looks so much better so:
std::cout << V[0] << S[3];
outputs As
It is not a good solution, i know, but it was the only opne i could think of.

Here we come to the point where "friend" declarations actually make sense:

Code: Select all

class Card {
    ...
    friend ostream& operator<<( ostream& os, const Card& card );
};
    
ostream& operator<<( ostream& os, const Card& card )
{
    os << "[" << Card::Set[ card.mSet ] << Card::Set[ card.mValue ] << "]";
    return os;
}

6) You are calling new() in Deck::Deck(), but your destructor does nothing: Memory leak.

is this simular to point 4.? if not i think it requires further explanation for me to understand it.

A "normal" variable declaration (without "new") means the variable is constructed on the stack. Once the stack frame is deleted - because the block in which the variable was declared is ended - the variable gets auto-destroyed, i.e. its destructor called.

If you declare a variable using "new", the variable is constructed on the heap instead, and will remain there untill you explicitly call delete() for it.

Now, your deck() constructor repeatedly calls "new card()", creating 52 cards on the heap. The ~deck() destructor does nothing, i.e. the deck gets deleted, but the 52 cards remain in memory.

Since you now no longer have a "deck" object that knows where those card objects are located, you can no longer delete() those cards, and have lost 52 * sizeof( card ) in memory. Do this repeatedly, and your memory will "leak" away.

The correct action would have been to go through the lists of your cards, in ~deck(), and calling delete() for every one of it.

i dont think it is very hard to understand what im trying to do...

The "what" (deck of cards with the option of printing their values in some intelligible manner) is not in question, but the "how" (linked list handling).

...and i do think this is the right approach for creating a deck of cards, which in this case is the only thing im trying to do.

Your approach was not "wrong" per se, it just did not make efficient use of the language and was needlessly complicated, making things difficult for you. Now, if you tell me you want to program this for the sake of learning to handle linked lists, my alternative suggestion would have looked different.

[Zacariaz]

Wow, this was enlightening, i dont think i have any comments other than i just gonna scrap it all and start from scratch.

Thanks for the help

[Zacariaz]

What about this then?

Code: Select all

#include <vector>

struct card_t {
  char V,S;
  card_t(char v,char s):V(v),S(s) {}
};

class deck {
  std::vector<card_t> Deck;
public:
  deck() {
    static const char Value[] = "A23456789TJQK";
    static const char Suit[] = "cdhs";
    for(int s = 0; s < 4; s++) 
      for(int v = 0; v < 13; v++)
        Deck.push_back(card_t(v,s));
  }
  void shuffle() {}
};

only one thing really annoys me... i cant use const in card_t is i want to use card_t() with the vector... How can you, well... make card_T read-only?

[Solar]

What about this then?

Declaring Value[] and Set[] within deck's constructor means they are not visible outside the constructor. Even if you place them outside the constructor and the class definition itself, it would still be deck::Value and deck::Set, which strikes me as odd. That is why I placed them in class Card in my example (Card::Value and Card::Set).

How can you, well... make card_T read-only?

By making V and S private, providing an accessor member function if need be:

Code: Select all

/* Sorry for insisting with the naming... you will find this is a
   very common notation style. */
class Card
{
    public:
        Card( char const value, char const set ) : mValue( value ), mSet( set );
        char value() const { return mValue; }
        char set() const { return mSet; }
    private:
        char mValue;
        char mSet;
};

[Zacariaz]

What about this then?

Declaring Value[] and Set[] within deck's constructor means they are not visible outside the constructor. Even if you place them outside the constructor and the class definition itself, it would still be deck::Value and deck::Set, which strikes me as odd. That is why I placed them in class Card in my example (Card::Value and Card::Set).

i never really understod the use of "static" and originally i didnt think they would be visible outside the constructor, but i got the impression from your code that they would.
So, how to initilize them the smart way outside the constructor?
I mean i can declare a variable, but not initialize it outside the constructor, as far as i know.

wether placing them in card_t ot deck i dont think is so important, its more a matter of how youre gonna use them, allthough from a a oop point of wiev i see the point in placing them in card_t.

How can you, well... make card_T read-only?

By making V and S private!

yes... but i still need access to card_t::V and card_t::S from deck, so that would mean making them friends and then they would still not be read-only.

So, the alternative is to make functions in card_t for just about everything and only let deck be handling the order of the cards, and not having anything to do with what they actually contain. (this would probably be the right way to go however)

I have never done much OOP programming so excuse me if im asking some stupid questions. I have allready learned alot and i hope to learn alot more.
Thanks again

[AJ]

Have a look again at Solar's last example. mValue and mSet only need to be accessed by the Card class. Deck accesses them via the value() and set() functions, effectively making them read only.

Cheers,
Adam

[Zacariaz]

So, i arrive at this, missing allmost everything that i needed:

Code: Select all

#include <vector>

class card_t {
  char Value,Suit;
public:
  card_t(char v,char s):Value(v),Suit(s) {}
  char getv() {return Value;}
  char gets() {return Suit;}
};

class deck {
  std::vector<card_t> Deck;
public:
  deck() {
    for(int s = 0; s < 4; s++) 
      for(int v = 0; v < 13; v++)
        Deck.push_back(card_t(v,s));
  }
  void shuffle() {}
};

it is inconvinient to use and i have to make a dedicated function in card_t to convert Value and Suit to their correct letter, numbers, whatever.
This:

Code: Select all

#include <vector>

class card_t {
  char Value,Suit;
  static char const V[] = "A23456789TJQK";
  static char const S[] = "cdhs";
public:
  card_t(char v,char s):Value(v),Suit(s) {}
  char getv() {return V[Value];}
  char gets() {return S[Suit];}
  char get() {return Value<<2|Suit}
};

class deck {
  std::vector<card_t> Deck;
public:
  deck() {
    for(int s = 0; s < 4; s++) 
      for(int v = 0; v < 13; v++)
        Deck.push_back(card_t(v,s));
  }
  void shuffle() {}
};

is more like i would want it to be, but due to the fact that i cannot initialize V[] and S[] outside the constructor... well, im confused.

[Zacariaz]

Have a look again at Solar's last example. mValue and mSet only need to be accessed by the Card class. Deck accesses them via the value() and set() functions, effectively making them read only.

Cheers,
Adam

yes i got that, but it wasnt exactly what i wanted at the time, thanks for pointing it out though.