GIDForums  

Go Back   GIDForums > Computer Programming Forums > C++ Forum
User Name
Password
Register FAQ Members List Calendar Search Today's Posts Mark Forums Read

 
 
Thread Tools Search this Thread Rate Thread
  #1  
Old 21-Apr-2005, 17:51
FlipNode FlipNode is offline
New Member
 
Join Date: Mar 2005
Posts: 14
FlipNode is on a distinguished road

Opinion on my code and a c++ class question


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);
}
  #2  
Old 21-Apr-2005, 22:09
FlipNode FlipNode is offline
New Member
 
Join Date: Mar 2005
Posts: 14
FlipNode is on a distinguished road

I changed this shuffle function


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
}
  #3  
Old 22-Apr-2005, 00:23
WaltP's Avatar
WaltP WaltP is offline
Outstanding Member
 
Join Date: Feb 2004
Location: Midwest US
Posts: 3,335
WaltP is a name known to allWaltP is a name known to allWaltP is a name known to allWaltP is a name known to allWaltP is a name known to allWaltP is a name known to all
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:
  1. Split deck in near half (+/- random*5 cards)
  2. 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... :-)
__________________

During the election they said Obama could only be elected when pigs fly. Well, we currently have an epidemic of Swine Flu. Coincidence?
  #4  
Old 22-Apr-2005, 21:06
FlipNode FlipNode is offline
New Member
 
Join Date: Mar 2005
Posts: 14
FlipNode is on a distinguished road
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!
  #5  
Old 22-Apr-2005, 23:11
WaltP's Avatar
WaltP WaltP is offline
Outstanding Member
 
Join Date: Feb 2004
Location: Midwest US
Posts: 3,335
WaltP is a name known to allWaltP is a name known to allWaltP is a name known to allWaltP is a name known to allWaltP is a name known to allWaltP is a name known to all
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.
__________________

During the election they said Obama could only be elected when pigs fly. Well, we currently have an epidemic of Swine Flu. Coincidence?
  #6  
Old 23-Apr-2005, 11:22
FlipNode FlipNode is offline
New Member
 
Join Date: Mar 2005
Posts: 14
FlipNode is on a distinguished road
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?
  #7  
Old 23-Apr-2005, 16:06
WaltP's Avatar
WaltP WaltP is offline
Outstanding Member
 
Join Date: Feb 2004
Location: Midwest US
Posts: 3,335
WaltP is a name known to allWaltP is a name known to allWaltP is a name known to allWaltP is a name known to allWaltP is a name known to allWaltP is a name known to all
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.
__________________

During the election they said Obama could only be elected when pigs fly. Well, we currently have an epidemic of Swine Flu. Coincidence?
  #8  
Old 07-Feb-2006, 09:15
rwehrli
 
Posts: n/a

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


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!
Last edited by JdS : 07-Feb-2006 at 09:32. Reason: Please insert your C code between [c] & [/c] tags
 
 

Recent GIDBlogOnce again, no time for hobbies by crystalattice

Thread Tools Search this Thread
Search this Thread:

Advanced Search
Rate This Thread
Rate This Thread:

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

vB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Forum Jump

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

Network Sites: GIDNetwork · GIDWebHosts · GIDSearch · Learning Journal by J de Silva, The

All times are GMT -6. The time now is 05:30.


vBulletin, Copyright © 2000 - 2009, Jelsoft Enterprises Ltd.