Opinion on my code and a c++ class question

FlipNode · #1 21-Apr-2005, 16:51

I just want you to look at what I have coded and tell me how you think I did and maybe give me some ideas on how to improve this code.

One thing I am thinking is to rename the "Card" class to "Cards" and make a new "Card" class that only defines a single card. Should I do this? If so, then how exactly?

)

I don't want to create a bunch of card objects in the "Cards" class.

Any comments would be great.

Code:

Files
main.cpp
Deck.h
Deck.cpp
Card.h
Card.cpp

- I displayed them in the reverse order then above

CPP / C++ / C Code:

// Developer	: Bruce Hunter
// File			: Card.h
// Purpose		: 

#ifndef CARD_H_
#define CARD_H_

class Card {

public:
	
	Card();									// constructor
	~Card();								// deconstructor
	
	int remaining;							// number of cards remaining/available
	
	// set functions
	int setRemaining( int );				// calculate and set remaing cards

	// get functions
	int getCardNum( int );					// get card number
	int getSuitNum( int );					// get suit number

	int getTotalCards( void ) const;		// get the number of cards in a deck
	int getTotalSuits( void ) const;		// get the number of suits in a deck

	const char *getFaceName( int ) const;	// get card face name
	const char *getSuitName( int ) const;	// get card suit name
	
	int getRemaining( void );				// get the number of remaining cards available
	
private:
	
	const int TOTAL_CARDS;					// constant total number of cards
	const int TOTAL_SUITS;					// constant total number of suits

	const static char *FACE[ 13 ];			// constant array of each different card face name
	const static char *SUIT[ 4 ];			// constant array of each different card suit name

}; // end Card class

#endif

CPP / C++ / C Code:

// Developer	: Bruce Hunter
// File			: Card.cpp
// Purpose		: 

#include "Card.h"

using namespace std;

//const int Card::TOTAL_CARDS = 52;	// total number of cards
//const int Card::TOTAL_SUITS = 4;	// total number of different suits

// constant card names
const char * Card::FACE[ 13 ] = 
				{ 
					"Ace", "King", "Queen", "Jack", "Ten", "Nine", "Eight",
					"Seven", "Six", "Five", "Four", "Three", "Deuce"
				};

// constant suit names
const char * Card::SUIT[ 4 ] = 
				{ 	
					"Spades", "Hearts" , "Diamonds", "Clubs"
				};

// Constructor
Card::Card(): TOTAL_CARDS(52),TOTAL_SUITS(4),remaining(52){}

// Deconstructor
Card::~Card()
{
	// TODO: de-initialize objects here
}

int Card::setRemaining( int value )
{
	// TODO: exception handling needed
	this->remaining = value;
}

int Card::getCardNum( int value )
{
	//TODO: exception handling needed
	return( value % 13 );
}

int Card::getSuitNum( int value )
{
	// TODO: exception handling needed
	return( value / 13 );
}

const char * Card::getFaceName( int value ) const
{
	//TODO: exception handling needed
	return( this->FACE[ value ] );
}

const char * Card::getSuitName( int value ) const
{
	//TODO: exception handling needed
	return( this->SUIT[ value ] );
}

int Card::getTotalCards( void ) const
{
	//TODO: exception handling needed
	return( this->TOTAL_CARDS );
}

int Card::getTotalSuits( void ) const
{
	//TODO: exception handling needed
	return( this->TOTAL_SUITS );
}

int Card::getRemaining( void )
{
	return( this->remaining );
}

CPP / C++ / C Code:

// Developer	: Bruce Hunter
// File			: Deck.h
// Purpose		: 

#ifndef DECK_H_
#define DECK_H_

#include "Card.h"

class Deck {

public:

	Deck();											// constructor
	~Deck();										// deconstructor
	
	void shuffle( void );							// shuffle deck
	void deal( int );								// deal a specific number of cards

	bool hasNext( void );							// check for another card to deal
	void dealNext( void );							// deal the next card in the deck

	void printConsole( int );						// print to console

private:
	
	int deck[ 52 ];									// deck of cards
		
	Card * card;									// create a pointer to our cards

	void initialize( void );						// initialize deck

	// utility set functions
	void setDeckValue( int );						// set a deck value of 0 -> 51, position and value are the same
	void setDeckValue( int, int );					// set a deck value of 0 -> 51, position and value are different

	// utility get functions
	int getDeckValue( int ) const;					// get a deck value

}; // end Deck class

#endif

CPP / C++ / C Code:

// Developer	: Bruce Hunter
// File			: Deck.cpp
// Purpose		: 

#include <iostream>		// basic input output stream
#include <iomanip>		// input output manipulation
#include <algorithm>	// random_shuffle()

#include "Deck.h"

using namespace std;

// Constructor
Deck::Deck()
{
	card = new Card();

	this->initialize();
}

// Deconstructor
Deck::~Deck()
{
	delete card;
}

// Function: initialize
// Purpose: 
void Deck::initialize( void )
{
	for( int position = 0; position < card->getTotalCards(); position++ )
		this->setDeckValue( position );
}

// Function: shuffle
// Purpose: shuffle the deck in a randomly
void Deck::shuffle( void )
{
	// TODO: this way of shuffling is not random enough
	random_shuffle( this->deck, this->deck+52 );
	card->setRemaining( card->getTotalCards() ); 	// reset the remaining cards on each shuffle
}

void Deck::deal( int dealNum )
{
	// TODO: exception handling needed
	
	if( this->hasNext() )
	{
		for( int position = 0; position < dealNum; position++ )
			this->dealNext();
	}
	else
	{
		// TODO: exception handling here
		// for now i will just do cout some shit
		cout << "Error: no more cards to deal" << endl;
	}
}

void Deck::dealNext( void )
{
	if( this->hasNext() )
	{
		int position = ( card->getTotalCards() - card->getRemaining() );
		this->printConsole( position );
		card->setRemaining( card->getRemaining() - 1 );
	}
	else
	{
		// TODO: exception handling here
		// for now i will just do cout some shit
		cout << "Error: no more cards to deal" << endl;
	}
}

bool Deck::hasNext( void )
{
	bool available = false;
	
	if( card->getRemaining() != 0 )
		available = true;
	
	return( available );
}

void Deck::printConsole( int deckPosition )
{
	cout << setw( 5 ) << setiosflags( ios::right )
		 << card->getFaceName( card->getCardNum( this->getDeckValue( deckPosition ) ) ) << " of "

		 << setw( 8 ) << setiosflags( ios::left )
		 << card->getSuitName( card->getSuitNum( this->getDeckValue( deckPosition ) ) )

		 << ( (deckPosition + 1) % 4 == 0 ? '\n' : '\t' );
}

void Deck::setDeckValue( int value )
{
	// TODO: exception handling needed
	this->deck[ value ] = value;
}

void Deck::setDeckValue( int position, int value )
{
	// TODO: exception handling needed
	this->deck[ position ] = value;
}

int Deck::getDeckValue( int position ) const
{
	// TODO: exception handling needed
	return( this->deck[ position ] );
}

CPP / C++ / C Code:

// Developer: Bruce Hunter
// File		: main.cpp
// Purpose	: test the Deck class

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

/*
	TO COMPILE: # g++ -o main main.cpp Deck.cpp Card.cpp
*/

using namespace std;

int main( int argc, char *argv[] ) 
{
	Deck * deck = new Deck();
	
	deck->shuffle();	// shuffle the deck

	deck->deal(3);		// deal 3 cards from the deck
	deck->deal(2);		// deal 2 more cards from the deck
	
	deck->dealNext();	// should be the 6 card down in the deck

	// 3 + 2 + 1 + 47 = 53 , which is more then we have to deal
	deck->deal(47);		// this should cause an error to be printed
	
	deck->dealNext();	// lets try to deal one more card just to see, it should give an error

	cout << endl;

	// if we shuffle again then we should be able to deal again without an error
	deck->shuffle();
	deck->deal(52);		// deal all cards in deck that we just shuffled
	
	return(0);
}

FlipNode · #2 21-Apr-2005, 21:09

i changed the shuffle function. This makes the shuffle random each time the program is run.
Before, I forgot to seed the random generator

CPP / C++ / C Code:

// Function: shuffle
// Purpose: shuffle the deck in a randomly
void Deck::shuffle( void )
{
	// TODO: this way of shuffling is not random enough
	
	srand(time(0));	// seed the random number generator
	
	// shuffle the deck random times
	for( int process = 0; process <= rand()%17; process++ )
		random_shuffle( this->deck, this->deck+52 );

	card->setRemaining( card->getTotalCards() ); 	// reset the remaining cards on each shuffle
}

WaltP · #3 21-Apr-2005, 23:23

First, don't put the srand() call in the shuffle routine. Put it near the beginning of main(). It only needs to be called once.

If you have a method called getRemaining() that returns the value of remaining, make the value private. It doesn't need to be public.

CPP / C++ / C Code:

int Card::setRemaining( int value )
{
  // TODO: exception handling needed
  this->remaining = value;
}

doesn't return an int. If it's not supposed to, make it a void method.

I did a cursory overview and that's all I saw. Just a couple very minor things...

Since you didn't include random_shuffle(), I can't comment on how you are doing it. But if you wish to replicate a real shuffle, "random" is not proper. Here's a couple thoughts...

Shuffle deck into newdeck...

Riffle Shuffle:

Split deck in near half (+/- random*5 cards)
While cards left in either half:
1. Calculate a random value from 1 to 4 (adjust at will)
2. Move that many cards from the bottom of 1st half to bottom of newdeck
3. Repeat a & b for second half

Overhand Shuffle:

While cards are left in deck
1. Calculate a random value from 1 to 10 (adjust at will)
2. Move that many cards from the top of deck to bottom of newdeck

And don't forget a cut function... :-)

FlipNode · #4 22-Apr-2005, 20:06

random_shuffle (); is part of the standard lib of c++. I belive the <algorithm> include.

I have made a lot of changes and update to the Poker lib I am writing from what you have seen. I have re-written the shuffle function without random_shuffle() use. Basically, the new function that I wrote is my way of replacing the random_shuffle() use.

Here is an example. Keep in mind the entire lib has changed from what I have posted above. You will get the idea though.

CPP / C++ / C Code:

// Function: shuffle
// Purpose: a basic random shuffle
void Deck::shuffle( void )
{
	int currentValue;	// hold current deck value
	int r_Position;		// hold random position
	int r_Value;		// hold random position value

	for ( int position = 0; position < card->getTotalCards(); position++ )
	{
		// save the current deck positions value
		currentValue = this->getDeckValue( position );
		// generate a random position in the deck
		r_Position = ( rand() % card->getTotalCards() );
		// save the random positions value
		r_Value = this->getDeckValue( r_Position );

		// do the deck value swap
		this->setDeckValue( position, r_Value );		
		this->setDeckValue( r_Position, currentValue ); 
	}
	// reset the remaining cards on each shuffle
	card->setRemaining( card->getTotalCards() );
}

I will be adding a cut deck function and a hand over hand function, which you will see in the next posted version of the lib. Also, I have move srand(time(0)) to the constructor. Ony run it once, correct?

1) What is Riffle the deck? Code example, I'm a little confuzed by the +/- symbol you gave.
2)What do you mean by making a newDeck? Do you mean shuffle deck1 and move it to a new array?

Please explain.

)

Thanks for your comments and help!

WaltP · #5 22-Apr-2005, 22:11

Welcome to my 1000th post!!

I'll try to make it worthwhile...

Quote:

Originally Posted by FlipNode

I will be adding a cut deck function and a hand over hand function, which you will see in the next posted version of the lib. Also, I have move srand(time(0)) to the constructor. Ony run it once, correct?

Constructor? Like for a deck? Won't that run every time you instantiate a deck? I was thinking in the main() function during your initialization....

Quote:

Originally Posted by FlipNode

1) What is Riffle the deck? Code example, I'm a little confuzed by the +/- symbol you gave.

+/-: ± or plus or minus. In this case, split the deck so one half has from 21-31 cards -- an uneven cut. Riffle Shuffle, as well as others, are here (and here). What I gave are the basic instructions you can convert into code.

Quote:

Originally Posted by FlipNode

2)What do you mean by making a newDeck? Do you mean shuffle deck1 and move it to a new array?

When shuffling a deck digitally, it's easiest to have a source and destination array for the shuffle. newDeck is the temporary place to hold the shuffled cards. They are moved back into Deck just before returning.

FlipNode · #6 23-Apr-2005, 10:22

Quote:

Constructor? Like for a deck? Won't that run every time you instantiate a deck? I was thinking in the main() function during your initialization....

Okay, so.. Everytime someone uses the lib, they have to know to seed the random generator. Why don't I just use a check to see if srand() is seeded already.

Possibly, there is a function along side srand() that does this?
Or
I can check create a boolean isSeeded and run a check each time the constructor is called.

One last thing. It doesn't hurt to run srand() over and over.
Doesn't it just re-seeds the generator with a different time?

Oh, and most likly you won't instantiate more then one deck. right?

WaltP · #7 23-Apr-2005, 15:06

Quote:

Originally Posted by FlipNode

Okay, so.. Everytime someone uses the lib, they have to know to seed the random generator. Why don't I just use a check to see if srand() is seeded already.

Didn't know you were doing a lib. I thought you were writing a program. Yes, they would have to know, or you can have an init function to set up stuff that only needs to be set up once.

Quote:

Originally Posted by FlipNode

Possibly, there is a function along side srand() that does this?

Not that I'm aware.

Quote:

Originally Posted by FlipNode

Or
I can check create a boolean isSeeded and run a check each time the constructor is called.

Yes you could. I'd consider it overkill, but it's not a really bad solution.

Quote:

Originally Posted by FlipNode

One last thing. It doesn't hurt to run srand() over and over.
Doesn't it just re-seeds the generator with a different time?

This is true. It won't hurt anything.

Quote:

Originally Posted by FlipNode

Oh, and most likly you won't instantiate more then one deck. right?

Depends on the game. Double Solitare would require two decks. There are other games that require more than one deck. And I'm not talking about two decks put together as one but two separate decks. Not many, but they do exist.

#8 07-Feb-2006, 08:15

I realize that this thread is now quite dated, but there are many things "wrong" with this class design:

Card should, at most, be an abstract base class. (Of course, my implementation--for convenience--below is just a base with no virtuality at all!) In C++ we will define a Card class that can represent ANY kind of card, or else our code destroys the promise of reusability. With that in mind, one my start thinking of all of the kinds of cards.

Greeting cards, 3x5 cards, flash cards and yes, playing cards...but also any kind of card, otherwise "Card" really isn't a Card except in a specific case.

PlayingCard : public Card {}; ...might be something you want to consider. However, you may find that you don't need anything more than PlayingCard for your quick and dirty exploits into learning how to write what appears to be a game played only with playing cards.

A PlayingCard class may have the enumerations of suit and face value, but storing them only as strings is probably not the best idea. It is far easier to store them as integers 1-13.

Your public interfaces expose operations that are not "Card" but, as you indicate, "Cards" like. Your Card class tries to be a deck of cards.

Your Deck class isn't. PlayingCardDeck is nothing more than a container and perhaps some rules for managing and manipulating the individual Card(s) stored in it. A well-written PCD would contain a std::vector<class PlayingCard> and expose the operations ::shuffle(); ...etc. I don't know where you really need "getRemaining()" but that is a poor naming convention. getRemaining what? While it may seem TOTALLY obvious to you at design time, who is going to use your library and wonder what it means? Even if everyone in the entire world can readily figure it out, what does it cost you to be specific? In fact, ::cardsLeft() is more descriptive and uses fewer letters, though I do like the idea of starting your operation name with a verb. You can't say getRemainingCards because it sounds like it is going to return a reference to the "rest" of the cards held inside of the deck. PlayingCardDeck::countOfCardsRemaining()? ...kind of ugly, but clear. I'd ask why you even need to know how many cards are remaining. It isn't like you can deal out more than you've got in the deck.

Also, instead of Card initializing itself to some value of one unique card in the 52-card deck, your PlayingCardDeck should act as a Factory to instantiate and store the necessary cards that it knows must be in an entire PCD. Then you have a PlayingCard that is little more than a place holder for a face value and a suit and perhaps some operations to telegraph that information. EG:

CPP / C++ / C Code:

#include <iostream>
#include <string>
#include <stdexcept>
#include <exception>
#include <vector>
#include <algorithm>
#include <ctime>
#include <cstdlib>
using namespace std;

typedef std::vector<int> deck;

//class PlayingCardDeck; // forward reference

class Card
{
public:
	Card() : m_type(0) {}
	
	Card( const int type )
	{
		m_type = type;
	}
	const int getType() const { return m_type; }
protected:
	int m_type;
};

class PlayingCard : public Card
{
public:
	const static char SPADES 	= 0x10;
	const static char HEARTS 	= 0x20;
	const static char CLUBS 	= 0x30;
	const static char DIAMONDS 	= 0x40;

//friend PlayingCardDeck;
	PlayingCard() :
	Card( 0 /* enum type { GAME=0, GREETING=1, ... } */ )
	{
		m_value = 0x00; // zero "initialized" PCard
	}
	PlayingCard( char value, char suit ) :
	Card( 0 /* enum type { GAME=0, GREETING=1, ... } */ ),
	m_value( 0 )
	{
		if( value < 1 || value > 13 )
		{
			throw new out_of_range( "value must be between 1 and 13" );
		}
		switch( suit )
		{
			case SPADES:
			case HEARTS:
			case CLUBS:
			case DIAMONDS:
				break;
			default:
				throw new out_of_range( "suit must be one of Spades, Hearts, Diamonds or Clubs" );
		}
		m_value = value | suit;
	}
	const char getValue() const
	{
		return m_value;
	}
	const char getFace() const
	{
		char face = m_value;
		return face &= ~( SPADES|HEARTS|CLUBS|DIAMONDS); // mask off the value from the suit
	}
	const char getSuit() const
	{
		return m_value - getFace();
	}
	const std::string getSuitName()
	{
		std::string suit;
		switch( getSuit() )
		{
			case SPADES:
				suit = "Spades";				
				break;
			case HEARTS:
				suit = "Hearts";
				break;
			case CLUBS:
				suit = "Clubs";
				break;
			case DIAMONDS:
				suit = "Diamonds";
				break;
			default:
				suit = "none";
		}
		return suit;
	}
	
protected:
	char m_value;
};

int main( int argc, char* argv[] )
{
	deck v;
	for( int i = 1; i <= 13; i++ )
	{
		for( int j = PlayingCard::SPADES; j <= PlayingCard::DIAMONDS; j+= 0x10 )
		{
			try
			{
				PlayingCard c( i, j );
			 	v.push_back( (int)c.getValue() ); // add card to vector here
				cout << (int)c.getFace() << " " << c.getSuitName() << " " << (int)c.getValue() << endl;
			}
			catch( exception ex )
			{
				cout << ex.what() << endl;
			}
		}
	}
	sort( v.begin(), v.end() );	
	deck::iterator it;
	for( it = v.begin(); it != v.end(); it++ )
	{
		cout << (int)(*it) << endl;
	}
	srand( time(0) );
	random_shuffle( v.begin(), v.end() );
	for( it = v.begin(); it != v.end(); it++ )
	{
		cout << (int)(*it) << endl;
	}
	
	return 0;
}

It is easy enough to mask off the bits of the face value and the suit value...

Also, you can comment in the friendship with the PCD (once you've written it) so that it can manipulate the card value directly, if somehow needed, though it is probably better not to use friendship for this purpose. You definitely do not want a public mutator for the value. Once a true PlayingCard instance exists, it is extremely unlikely that you'd ever want to change the face value of it. The main function is a quick and dirty example of what the factory operation in the PCD might do, albeit, you would push_back the PC (or just the value, as I've done) onto the PCD's internal vector upon instantiation. Then the PCD could random_shuffle (or other shuffle implementation, as seen fit) and be ready to cut.

Output of execution of the goofy bit of code would look something like:

Code:

1 Spades 17
1 Hearts 33
1 Clubs 49
1 Diamonds 65
2 Spades 18
2 Hearts 34
2 Clubs 50
2 Diamonds 66
3 Spades 19
3 Hearts 35
3 Clubs 51
3 Diamonds 67
4 Spades 20
4 Hearts 36
4 Clubs 52
4 Diamonds 68
5 Spades 21
5 Hearts 37
5 Clubs 53
5 Diamonds 69
6 Spades 22
6 Hearts 38
6 Clubs 54
6 Diamonds 70
7 Spades 23
7 Hearts 39
7 Clubs 55
7 Diamonds 71
8 Spades 24
8 Hearts 40
8 Clubs 56
8 Diamonds 72
9 Spades 25
9 Hearts 41
9 Clubs 57
9 Diamonds 73
10 Spades 26
10 Hearts 42
10 Clubs 58
10 Diamonds 74
11 Spades 27
11 Hearts 43
11 Clubs 59
11 Diamonds 75
12 Spades 28
12 Hearts 44
12 Clubs 60
12 Diamonds 76
13 Spades 29
13 Hearts 45
13 Clubs 61
13 Diamonds 77
...(spewed vector contents from sort/random_shuffle)

Of course, this was a nasty hack done in a few minutes and is not likely very representative of "truly good" design. There are probably a number of important constraints that I've taken the liberty of avoiding in this "work."

For example: You could, obviously, overload a ctor so that it takes an m_value.

HTH...

Take Care.

Rob!

Thread Tools	Search this Thread
Show Printable Version Email this Page	Search this Thread: Advanced Search
Rate This Thread
Rate This Thread:

Similar Threads
Thread	Thread Starter	Forum	Replies	Last Post
Guidelines for posting requests for help - UPDATED!	WaltP	C++ Forum	1	22-Aug-2008 00:07
Introduction to .NET	LuciWiz	.NET Forum	5	09-Aug-2007 04:53
Problem with int mixed with char,...	leitz	C++ Forum	17	07-Dec-2004 20:56
Help! Some basal questions about MFC	xutingnjupt	MS Visual C++ / MFC Forum	1	05-Dec-2004 03:38

Opinion on my code and a c++ class question

I changed this shuffle function

Re: Opinion on my code and a c++ class question