Problem with nested class or array overrun.

[J. Campbell]

I'm trying to make a class that represents a deck of cards and a
dealer for use in a card game. I'm fairly novice. The code compiles
(Dev c++(gcc 3.3.1)) without complaint, however it acts strange when I
run it. (In windows), when I double-click or run the prog from the
IDE, it displays the deck, then exits. When I run it from a console
window, if I type the filename eg "cards.exe", it displays the cards
as expected but doesn't pause at the end of the program. However if
I type "cards" without the explicit exe extension, the cards are not
displayed and I get the message:
"This application has requested the Runtime to terminate it in an
unusual way.
Please contact the application's support team for more information."

Any help is greatly appreciated. Joe
_________________________________________
#include <iostream>
#include <stdint.h>
#include <string>

using namespace std;

struct acard{
int suit;
int value;
uint32_t index; // to be used by shuffle function
};

class deck{
private:
string theSuits; // contains suits in order
string theValues; // contains values in order
acard* card; // an array of cards
int numcards;

public:
deck();
~deck();
void show();
// void shuffle();
};

deck::deck():
theSuits("\x05\x04\x03\x06") //c, d, h, s
,theValues("23456789TJKQA")
,numcards(theSuits.length() * theValues.length())
,card(new acard[numcards])

{
int cardnum(0);
for(int i = 0; i < theSuits.length(); ++i){
for(int j = 0; j < (theValues.length()); ++j){
cardnum = ((i * theValues.length()) + j);
card[cardnum].suit = i;
card[cardnum].value = j;
}
}
}

deck::~deck(){
delete [] card;
}

void deck::show(){
for (int i=0; i < numcards; ++i){
cout << theValues[card.value]
<< theSuits[card.suit] << endl;
}
}


int main()
{
deck mydeck;
mydeck.show();

system("pause");
return 0;
}
____________________________________

 

[Victor Bazarov]

I'm trying to make a class that represents a deck of cards and a
dealer for use in a card game. I'm fairly novice. The code compiles
(Dev c++(gcc 3.3.1)) without complaint, however it acts strange when I
run it. (In windows), when I double-click or run the prog from the
IDE, it displays the deck, then exits. When I run it from a console
window, if I type the filename eg "cards.exe", it displays the cards
as expected but doesn't pause at the end of the program. However if
I type "cards" without the explicit exe extension, the cards are not
displayed and I get the message:
"This application has requested the Runtime to terminate it in an
unusual way.
Please contact the application's support team for more information."

Any help is greatly appreciated. Joe

Regarding the message you get from your OS, I think you should ask in the
newsgroup for that OS. Regarding the code, see below.

_________________________________________
#include <iostream>
#include <stdint.h>

No such standard header. Yet.

#include <string>

using namespace std;

struct acard{
int suit;
int value;
uint32_t index; // to be used by shuffle function

No such standard type. Yet. Use at your own risk.

};

class deck{
private:
string theSuits; // contains suits in order
string theValues; // contains values in order
acard* card; // an array of cards
int numcards;

Swap the two lines before above. The explanation is in my next
paragraph.

public:
deck();
~deck();
void show();
// void shuffle();
};

deck::deck():
theSuits("\x05\x04\x03\x06") //c, d, h, s
,theValues("23456789TJKQA")
,numcards(theSuits.length() * theValues.length())
,card(new acard[numcards])

This is very wrong. The initialisation happens in the order of the
declarations, not in the order you wrote in the initialiser list. So,
when you initialise the "card" member, "numcards" still contains trash.
What you allocate is impossible to say. Move the declaration of the
'numcards' member _before_ the 'card' member.

{
int cardnum(0);
for(int i = 0; i < theSuits.length(); ++i){
for(int j = 0; j < (theValues.length()); ++j){
cardnum = ((i * theValues.length()) + j);

You might want to reconsider the use of superfluous parentheses. It
makes the code harder to read.

card[cardnum].suit = i;
card[cardnum].value = j;
}
}
}

deck::~deck(){
delete [] card;
}

void deck::show(){
for (int i=0; i < numcards; ++i){
cout << theValues[card.value]
<< theSuits[card.suit] << endl;
}
}


int main()
{
deck mydeck;
mydeck.show();

system("pause");

The behaviour of 'system' is implementation-defined. There is no way
to predict what it will do. If you simply need to pause your program,
you might want to use getchar().

return 0;
}

Victor

 

[Joe C]

class deck{
private:
string theSuits; // contains suits in order
string theValues; // contains values in order
acard* card; // an array of cards
int numcards;

Swap the two lines before above. The explanation is in my next
paragraph.

public:
deck();
~deck();
void show();
// void shuffle();
};

deck::deck():
theSuits("\x05\x04\x03\x06") //c, d, h, s
,theValues("23456789TJKQA")
,numcards(theSuits.length() * theValues.length())
,card(new acard[numcards])

This is very wrong. The initialisation happens in the order of the
declarations, not in the order you wrote in the initialiser list. So,
when you initialise the "card" member, "numcards" still contains trash.
What you allocate is impossible to say. Move the declaration of the
'numcards' member _before_ the 'card' member.

Victor

Thanks Victor. What a critical mistake! I confess ignorance. I thought,
as you guessed, that the order was as-per the initialiser list...

I included the compiler info to cover bases, since I couldn't figure it out
what was going wrong...and the OS-specific call, because...well...I won't
try to justify it...

Thanks again for the real help...this detail will carry forward. I wish I
could pay you back. Your helping karma is golden.

Joe

 

[Joe C]

One detail I forgot to ask. Suppose I have a class where the initialization
order is critical, and that the order alternates between public and private.
Does it become necessarry to do something like this?:

class myclass{
public:
int ImFirst;
private:
int ImSecond;
public:
int ImThird;
private:
int ImFourth;
};

Thanks again.

 

[Victor Bazarov]

One detail I forgot to ask. Suppose I have a class where the initialization
order is critical, and that the order alternates between public and private.
Does it become necessarry to do something like this?:

class myclass{
public:
int ImFirst;
private:
int ImSecond;
public:
int ImThird;
private:
int ImFourth;
};

I've never seen anything like this, but I suppose if the requirement is so
unusual, so should be the syntax.

Let me just note here that if the order of initialisation is important to
the point when you have to introduce some weirdness to the class, you may
need to rethink your design, reconsider the reasons according to which the
order of initialisation has to be specific. And also, if you ever need
the data members to be public, you should also think twice. I cannot
remember ever having to have both private and public data in the same
class. The data are either all public or all private (and much more often
the latter than the former).

Victor

 

[Old Wolf]

I'm trying to make a class that represents a deck of cards and a
dealer for use in a card game. I'm fairly novice.

First lesson... don't manually manage memory when there's an easier way.

#include <iostream>
#include <stdint.h>
#include <string>

using namespace std;

struct acard{
int suit;
int value;
uint32_t index; // to be used by shuffle function
};

class deck{
private:
string theSuits; // contains suits in order
string theValues; // contains values in order

Are the suits and values ever going to change during a game? If not,
then use:
string const theSuits;
etc.

Are they specific to this deck? If all decks will have the same suit
and values then you should either make these items static, or remove
them from the class entirely. Also it is considered good design
these days to have code that populates the deck, not actually as a
member function of the deck, ie. when you create a new deck, it
starts off blank and the constructor does as little as possible;
and then you have a free function that takes a deck as reference
parameter and adds all the cards to it.

acard* card; // an array of cards
int numcards;

Replace these two with:

public:
deck();
~deck();
void show();
// void shuffle();

Use std::random_shuffle() instead of writing your own shuffle. Again
it would be good design for this to not be a member function.

};

deck::deck():
theSuits("\x05\x04\x03\x06") //c, d, h, s
,theValues("23456789TJKQA")

,numcards(theSuits.length() * theValues.length())
,card(new acard[numcards])

,acard(theSuits.length() * theValues.length())

deck::~deck(){
delete [] card;
}

You don't need this function at all now. The rest of the program
stays the same

If you stuck with your pointer approach, then you would also have to
add a copy-constructor and an assignment operator.

 

[Victor Bazarov]

First lesson... don't manually manage memory when there's an easier way.







Are the suits and values ever going to change during a game? If not,
then use:
string const theSuits;
etc.

Are they specific to this deck? If all decks will have the same suit
and values then you should either make these items static, or remove
them from the class entirely. Also it is considered good design
these days to have code that populates the deck, not actually as a
member function of the deck, ie. when you create a new deck, it
starts off blank and the constructor does as little as possible;
and then you have a free function that takes a deck as reference
parameter and adds all the cards to it.




Replace these two with:



Use std::random_shuffle() instead of writing your own shuffle. Again
it would be good design for this to not be a member function.

This is not needed if 'numcards' is not there any more, right?

,card(new acard[numcards])

,acard(theSuits.length() * theValues.length())

Actually

, card(theSuits.length() * theValues.length())

deck::~deck(){
delete [] card;
}

You don't need this function at all now. The rest of the program
stays the same

If you stuck with your pointer approach, then you would also have to
add a copy-constructor and an assignment operator.

How true...

V

 

[Old Wolf]

,card(new acard[numcards])

,acard(theSuits.length() * theValues.length())

Actually
, card(theSuits.length() * theValues.length())

Right. All the more reason to use a better naming convention than
"acard" as a typename and "card" as an acard.