Sign in to follow this  

Review my Uno Game

This topic is 3464 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

For the past week or so I've been working on a text based uno game in which you play against computers. This community has helped me out and for that I owe you guys a big thank you. The rules of Uno can be found at: http://en.wikipedia.org/wiki/UNO_(game) I was looking to have you guys critique the code and point out anything that looks wrong/bad/poor style/etc. I'm also looking to add a GUI using SDL so if you have any pointers on a good way to accomplish that those would be very helpful. Anything you have to say would be greatly appreciated. If there is a better way to post the code let me know. I email a .zip file containing all of the source files and/or a compiled executable if someone would like one. The there are a few basic classes: Game: holds all the game logic, players, deck and pile of cards Player: An interface defining things that a player needs to be able to do Card: a class representing an uno card Deck: a class representing an uno deck VectorHandPlayer: an implementation of the Player interface using a vector to hold the hand HumanPlayer: a subclass of VectorHandPlayer that gets input from the user AIPlayer: a subclass of VectorHandPlayer that makes decisions on its own Card.h
#pragma once
#include <ostream>

const int DRAWTWO = 10;
const int SKIP = 11;
const int REVERSE = 12;
const int MAXCARDNUM = 12;

const int WILDNUM = 13;
const int WILDDRAWFOURNUM = 14;

enum Suit{ NOCOLOR = 0, RED = 1, BLUE = 2, GREEN = 4, YELLOW = 8, WILD = RED|BLUE|GREEN|YELLOW };

class Card
{
public:
	Card(int number, Suit color);
	~Card(void);

	int number()const;
	Suit color()const;
	void setWildColor(Suit color);
	void resetWildColor();

	bool validPlay(const Card* toPlay)const;

	friend std::ostream& operator<<(std::ostream& os, Card crd);

	bool operator<(const Card& rhs)const;

private:
	int _num;
	Suit _col;
};


std::ostream& operator<<(std::ostream& os, Card crd);

Card.cpp
#include "Card.h"
#include "conutil.h"

Card::Card(int number, Suit col):
_num(number),
_col(col)
{
}

Card::~Card()
{
}

int Card::number()const
{
	return _num;
}

Suit Card::color()const
{
	return _col;
}

void Card::setWildColor(Suit color)
{
	if(_col == WILD)
		_col = color;
}
void Card::resetWildColor()
{
	if(_num == WILDNUM || _num == WILDDRAWFOURNUM)
		_col = WILD;
}

bool Card::operator<(const Card& rhs)const
{
	return ((int)_col*1000 + _num) < ((int)rhs._col*1000 + rhs._num);
}

bool Card::validPlay(const Card* toPlay)const
{
		return ((toPlay->_col) & (_col)) || (toPlay->_num == _num) ||
			     toPlay->_num == WILDNUM || toPlay->_num == WILDDRAWFOURNUM;
}

std::ostream& operator<<(std::ostream& os, Card crd)
{
	switch(crd._col)
	{
	case GREEN:
		textcolor(cGREEN);
		break;
	case RED:
		textcolor(cRED);
		break;
	case YELLOW:
		textcolor(cYELLOW);
		break;
	case BLUE:
		textcolor(cBLUE);
		break;
	default:
		textcolor(cWHITE);
	}

	switch(crd._num)
	{
	case WILDNUM:
		os << "WLD";
		break;
	case WILDDRAWFOURNUM:
		os << "DR4";
		break;
	case DRAWTWO:
		os << "DR2";
		break;
	case SKIP:
		os << "SKP";
		break;
	case REVERSE:
		os << "REV";
		break;
	default:
		os << "  " << crd._num;
	}
	textcolor(cLIGHT_GREY);
	
	return os;
}

Deck.h
#pragma once
#include "Card.h"
#include <vector>
#include <list>

class Deck
{
public:
	Deck(void);
	~Deck(void);

	int size()const;
	Card* deal();
	void shuffle();
	void print()const;
	void addCards(std::list<Card*> cards);

private:
	std::vector<Card*> _cards;
};

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

using std::cout;
using std::endl;

Deck::Deck(void)
{
	//Construct the deck
	//A deck contains 110 cards consisting of 4 suits
	//Each suit has two cards 1-9, DrawTwo, Skip, Reverse and one 0 card
	//There are also four of each Wild and Wild Draw Four
	_cards.reserve(110);
	for(int i = 0; i <= MAXCARDNUM; i++)
	{
		_cards.push_back( new Card(i, RED) );
		_cards.push_back( new Card(i, GREEN) );
		_cards.push_back( new Card(i, BLUE) );
		_cards.push_back( new Card(i, YELLOW) );
		if(i != 0)
		{
			_cards.push_back( new Card(i, RED) );
			_cards.push_back( new Card(i, GREEN) );
			_cards.push_back( new Card(i, BLUE) );
			_cards.push_back( new Card(i, YELLOW) );
		}
	}
	for(int i = 0; i < 4; i++)
	{
		_cards.push_back( new Card(WILDNUM, WILD) );
		_cards.push_back( new Card(WILDDRAWFOURNUM, WILD) );
	}
}

Deck::~Deck(void)
{
	while(!_cards.empty())
	{
		delete _cards.back();
		_cards.pop_back();
	}
}

int Deck::size()const
{
	return _cards.size();
}

Card* Deck::deal()
{
	if(_cards.empty())
		return NULL;
	Card* ret = _cards.back();
	_cards.pop_back();
	return ret;
}

void Deck::shuffle()
{
	for(int i = 0; i < 400; i++)
	{
		int a = rand() % _cards.size();
		int b = rand() % _cards.size();
		std::swap(_cards[a], _cards[b]);
	}
}

void Deck::print()const
{
	for(int i = 0; i < _cards.size(); i++)
	{
		std::cout << *(_cards[i]) << ' ';
		if( (i+1) % 10 == 0 )
			std::cout << '\n';
	}
	std::cout << std::endl;
}

void Deck::addCards(std::list<Card*> cards)
{
	_cards.insert(_cards.begin(), cards.begin(), cards.end());
}

Game.h
#pragma once
#include <list>
#include "Deck.h"
#include "Player.h"

typedef std::list<Player*> PlayerList;

class Game
{
public:
	Game();
	~Game();

	void addPlayers();
	void playRound();

private:
	Deck* _deck;
	PlayerList _players;
	std::list<Card*> _pile;

	PlayerList::iterator _pos;
	bool _forward;

	void dealHands();
	Card* safeDeal();
	void drawCards(Player* pl, int numCards);
	void advance();
	void reverse();
};

Game.cpp
#include "Game.h"
#include <iostream>
#include <sstream>
#include <cstdlib>
#include <ctime>
#include <cassert>
#include "AIPlayer.h"
#include "HumanPlayer.h"

Game::Game(void):
_forward(true)
{
	_deck = new Deck();
	srand( (unsigned int)time(NULL) );
}

Game::~Game(void)
{
	while(!_pile.empty())
	{
		delete _pile.back();
		_pile.pop_back();
	}
	delete _deck;
	while(!_players.empty())
	{
		delete _players.back();
		_players.pop_back();
	}
}

void Game::addPlayers()
{
	_players.push_back( new HumanPlayer() );
	_players.push_back( new AIPlayer() );
	_players.push_back( new AIPlayer() );
	_players.push_back( new AIPlayer() );

	_pos = _players.begin();
}

void Game::playRound()
{
	dealHands();

	std::string winner = "";
	while( winner.empty() )
	{
		//Have the player play a card until they play a valid one
		Player* curPlayer = (*_pos);
		Card* played;
		bool canRenege = true;
		while(true)
		{
			played = curPlayer->playCard(_pile.back(), canRenege);

			//if they want to draw cards
			if(played == NULL)
			{
				int numCards = 0;
				Card* drawn;
				do
				{
					numCards++;
					drawn = safeDeal();
					curPlayer->addCard(drawn);
				} while( !_pile.back()->validPlay(drawn) );

				//TODO: use specific function
				std::cout << curPlayer->name() << " drew " << numCards << " cards" << std::endl;

				canRenege = false;
				played = curPlayer->playCard(_pile.back(), canRenege);
			}

			//Validity check
			if(_pile.back()->validPlay(played))
				break;
			else
			{
				curPlayer->addCard(played);
				std::cout << "LAST PLAY WAS INVALID.  TRY AGAIN" << std::endl;
			}
		}

		//add the card and let people know what card was played
		//reset last card to wild if it was a wild
		_pile.back()->resetWildColor();
		_pile.push_back(played);
		std::cout << curPlayer->name() << " played a " << *played << std::endl;

		//do something based on card
		switch(played->number())
		{
		case SKIP: advance(); break;
		case REVERSE: reverse(); break;
		case DRAWTWO:
			advance();
			drawCards(*_pos, 2);
			break;
		case WILDDRAWFOURNUM:
			advance();
			drawCards(*_pos, 4);
			break;
		}

		//Checks based on number of cards
		if(curPlayer->handSize() == 0) //win
			winner = curPlayer->name();
		else if(curPlayer->handSize() == 1) //uno
			std::cout << curPlayer->name() << " says UNO!" << std::endl;
		else
			std::cout << curPlayer->name() << " has " << curPlayer->handSize() << " cards left" << std::endl;

		//next player
		advance();
		std::cout << std::endl;
	}

	//print the winner
	std::cout << winner << " has won!!" << std::endl;

	//clean up
	while(!_pile.empty())
	{
		delete _pile.back();
		_pile.pop_back();
	}
	for(PlayerList::iterator it = _players.begin(); it != _players.end(); it++)
		(*it)->clearHand();
}

void Game::dealHands()
{
	assert(_pile.empty());

	//new deck for dealing
	delete _deck;
	_deck = new Deck();
	_deck->shuffle();

	//deal hands
	for(PlayerList::iterator it = _players.begin(); it != _players.end(); it++)
	{
		drawCards(*it, 7);
	}

	//first card on the pile
	_pile.push_back(_deck->deal());
}

Card* Game::safeDeal()
{
	if(_deck->size() == 0)
	{
		Card* topCard = _pile.back();
		_pile.pop_back();
		_deck->addCards(_pile);
		_deck->shuffle();
		_pile.clear();
		_pile.push_back(topCard);
	}
	return _deck->deal();
}

void Game::drawCards(Player* pl, int numCards)
{
	for(int i = 0; i < numCards; i++)
		pl->addCard( safeDeal() );
}

void Game::advance()
{
	if(_forward)
	{
		_pos++;
		if(_pos == _players.end())
			_pos = _players.begin();
	}
	else
	{
		if(_pos == _players.begin())
			_pos = _players.end();
		_pos--;
	}
}

void Game::reverse()
{
	_forward = !_forward;
}

Player.h
#pragma once
#include "Card.h"
#include <string>

class Player
{
public:
	Player(std::string name);
	virtual ~Player(void);

	//accessors
	std::string name()const;
	virtual int handSize()const = 0;

	//actions
	virtual void addCard(Card* crd) = 0;
	virtual void clearHand() = 0;
	//returns the card to play or null if the player wants to draw cards
	virtual Card* playCard(const Card* lastCard, bool canRenege = true) = 0;

protected:
	std::string _name;
};

Player.cpp
#include "Player.h"

Player::Player(std::string name):
_name(name)
{
}

Player::~Player()
{
}

std::string Player::name()const
{
	return _name;
}

VectorHandPlayer.h
#pragma once
#include "player.h"
#include "Card.h"
#include <string>
#include <vector>

class VectorHandPlayer :
	public Player
{
public:
	VectorHandPlayer(std::string name);
	virtual ~VectorHandPlayer(void);

	int handSize()const;
	void addCard(Card* crd);
	void clearHand();

	virtual Card* playCard(const Card* lastCard, bool canRenege = true) = 0;

protected:
	std::vector<Card*> _hand;
};

VectorHandPlayer.cpp
#include "VectorHandPlayer.h"

VectorHandPlayer::VectorHandPlayer(std::string name):
Player(name)
{
}

VectorHandPlayer::~VectorHandPlayer(void)
{
	clearHand();
}

int VectorHandPlayer::handSize()const
{
	return _hand.size();
}

void VectorHandPlayer::addCard(Card* crd)
{
	_hand.push_back(crd);
}

void VectorHandPlayer::clearHand()
{
	while(!_hand.empty())
	{
		delete _hand.back();
		_hand.pop_back();
	}
}

HumanPlayer.h
#pragma once
#include "VectorHandPlayer.h"
#include <vector>

class HumanPlayer :
	public VectorHandPlayer
{
public:
	HumanPlayer(void);
	HumanPlayer(std::string name);
	virtual ~HumanPlayer(void);

	virtual Card* playCard(const Card* lastCard, bool canRenege = true);
	void displayMessage(std::string msg)const;

protected:
	static char _lastID;
	bool _dispAdds;

	void sortHand();
	void printHand(bool numbers = false);
	void displayAdds(bool YorN);
};


HumanPlayer.cpp
#include "HumanPlayer.h"
#include <iostream>
#include <algorithm>

char HumanPlayer::_lastID = 'A';

HumanPlayer::HumanPlayer():
VectorHandPlayer(std::string("HumanPlayer") + _lastID++),
_dispAdds(false)
{
}

HumanPlayer::HumanPlayer(std::string name):
VectorHandPlayer(name),
_dispAdds(false)
{
}

HumanPlayer::~HumanPlayer()
{
}

void HumanPlayer::printHand(bool numbers)
{
	if(numbers)
	{
		for(int i = 1; i <= _hand.size(); i++)
		{
			if(i < 10)
				std::cout << "  " << i;
			else
				std::cout << " " << i;
			std::cout << ' ';
		}
		std::cout << std::endl;
	}

	for(std::vector<Card*>::iterator it = _hand.begin(); it != _hand.end(); it++)
		std::cout << *(*it) << ' ';
	std::cout << std::endl;
}

bool compareCards(const Card* a, const Card* b)
{
	return (*a) < (*b);
}

void HumanPlayer::sortHand()
{
	std::sort(_hand.begin(), _hand.end(), compareCards);
}

void HumanPlayer::displayMessage(std::string msg)const
{
	std::cout << msg << std::endl;
}

Card* HumanPlayer::playCard(const Card* lastCard, bool canRenege)
{
	std::cout << _name << ": your turn" << std::endl;
	sortHand();
	printHand(true);
	Card *ret = NULL;
	int num = -1;
	while(!ret)
	{
		while( num < 0 || num > _hand.size() )
		{
			//std::cout << "Enter the number corresponding to the card you want to play or 0 to draw cards until you find one that will play: ";
			std::cout << "<" << *lastCard << "> ";
			while( !(std::cin >> num) )
			{
				std::cin.clear();
				std::cin.ignore(100, '\n');
				std::cout << "Please enter a number from 0 to " << _hand.size() << ": ";
			}
			if(num == 0 && canRenege)
				return NULL;
			--num;
		}
		ret = _hand[num];
	}
	if(ret->color() == WILD)
	{
		int colNum = -1;
		Suit color = NOCOLOR;
		while(color == NOCOLOR)
		{	
			std::cout << "Select which color you'd like your wild to be: " << std::endl;
			std::cout << "1: Red  2: Green  3: Yellow  4: Blue" << std::endl;
			std::cout << "Your choice: ";
			while( !(std::cin >> colNum) )
			{
				std::cin.clear();
				std::cin.ignore(100, '\n');
				std::cout << "Please try again: ";
			}
			switch(colNum)
			{
			case 1: color = RED; break;
			case 2: color = GREEN; break;
			case 3: color = YELLOW; break;
			case 4: color = BLUE; break;
			default: color = NOCOLOR;
			}
		}
		ret->setWildColor(color);
	}
	_hand.erase(_hand.begin() + num);
	return ret;
}


AIPlayer.h
#pragma once
#include "VectorHandPlayer.h"
#include "Card.h"
#include <string>
#include <map>
#include <vector>

typedef std::map<Suit,int> SuitCounts;

class AIPlayer :
	public VectorHandPlayer
{
public:
	AIPlayer(void);
	AIPlayer(std::string name);
	virtual ~AIPlayer(void);

	void displayMessage(std::string)const;

	virtual Card* playCard(const Card* lastCard, bool canRenege = true);

protected:
	template<class InputIterator>
	SuitCounts countBySuit( InputIterator first, InputIterator last );
	template<class T, class InputIterator>
	T highestCount( InputIterator first, InputIterator last);
	template<class InputIterator, class Predicate>
	std::vector<Card*> vectorIf( InputIterator first, InputIterator last, Predicate pred );

private:
	static char _lastID;
};


AIPlayer.cpp
#include "AIPlayer.h"
#include <cstdlib>
#include <iostream>
#include <map>
#include <boost/bind.hpp>
#include <algorithm>

char AIPlayer::_lastID = 'A';

AIPlayer::AIPlayer(void):
VectorHandPlayer(std::string("AIPlayer") + _lastID++)
{
}

AIPlayer::AIPlayer(std::string name):
VectorHandPlayer(name)
{
}

AIPlayer::~AIPlayer(void)
{
}

bool isColor(Card* crd, Suit col)
{
	return crd->color() == col;
}

void AIPlayer::displayMessage(std::string msg)const
{
}

Card* AIPlayer::playCard(const Card* lastCard, bool canRenege)
{
	//Get Playable cards
	std::vector<Card*> playable = vectorIf(_hand.begin(), _hand.end(), boost::bind(&Card::validPlay, lastCard, _1));

	if( playable.size() == 0 )
		return NULL;

	//Find most common color
	SuitCounts counts = countBySuit(playable.begin(), playable.end());
	Suit col = highestCount<Suit>(counts.begin(), counts.end());

	//list all playable cards of that color
	playable = vectorIf(playable.begin(), playable.end(), boost::bind(isColor, _1, col));
	//choose a random one
	Card* ret = playable[rand() % playable.size()];
	//if it's wild, pick a color
	if(ret->color() == WILD)
	{
		counts = countBySuit(_hand.begin(), _hand.end());
		counts[WILD] = 0;
		col = highestCount<Suit>(counts.begin(), counts.end());
		if(col == WILD)
		{
			Suit colors[] = {RED, GREEN, BLUE, YELLOW};
			col = colors[ rand()%4 ];
		}
		ret->setWildColor(col);
	}
	//remove from hand
	_hand.erase( std::find(_hand.begin(), _hand.end(), ret) );
	return ret;
}

template<class InputIterator>
SuitCounts AIPlayer::countBySuit(InputIterator first, InputIterator last)
{
	SuitCounts counts;
	for(InputIterator it = first; it != last; it++)
	{
		++counts[ (*it)->color() ];
	}
	return counts;
}

template<class InputIterator, class Predicate>
std::vector<Card*> AIPlayer::vectorIf( InputIterator first, InputIterator last, Predicate pred )
{
	std::vector<Card*> cards;
	while(first != last)
	{
		if( pred(*first) )
			cards.push_back(*first);
		++first;
	}
	return cards;
}

template<class T, class InputIterator>
T AIPlayer::highestCount(InputIterator first, InputIterator last)
{
	T ret;
	int highest = -1;
	while(first != last)
	{
		if(first->second > highest)
		{
			highest = first->second;
			ret = first->first;
		}
		first++;
	}
	return ret;
}

conutil.h
#include <windows.h>

enum Colors {
	cBLACK = 0,		cDARK_BLUE,		cDARK_GREEN,	cDARK_CYAN,
	cDARK_RED,		cDARK_MAGENTA,	cBROWN,			cLIGHT_GREY,
	cDARK_GREY,		cBLUE,			cGREEN,			cCYAN,
	cRED,			cMAGENTA,		cYELLOW,		cWHITE};

void textcolor(int color)
{
   HANDLE handle= GetStdHandle(STD_OUTPUT_HANDLE);
   SetConsoleTextAttribute( handle, color);
}

Uno.cpp
#include "conio.h"
#include "Deck.h"
#include "Player.h"
#include "Game.h"
#include <iostream>


int main(int argc, char* argv[])
{
	std::cout << "When asked to play your hand will be displayed with numbers above each card.\n"
		"The last card played will be displayed inbetween <> like: <  9>\n"
		"Enter the number corresponding to the card you wish to play or 0 to renege\n"
		"and draw cards from the deck until you get one that will play." << std::endl;
	Game game;

	game.addPlayers();
	game.playRound();

	std::cout << "\n\nPRESS ANY KEY TO QUIT" << std::endl;
	getch();
	return 0;
}



Share this post


Link to post
Share on other sites
The first thing I notice is that you are using #pragma once in your header files. I don't know if this could be a potential problem, but the general practice is more like:
#ifndef THISHEADER_H
#define THISHEADER_H

//header code goes here
#endif


I would also add that beginning variable names with an underscore hinders readability. It's much easier to use mVarName for member variables.

And yes, it is a lot of code to look over, so I stopped after only part. Generally, it looks good to me. Nice use of classes to organize everything.

Share this post


Link to post
Share on other sites
Quote:
Original post by bschneid
The first thing I notice is that you are using #pragma once in your header files. I don't know if this could be a potential problem, but the general practice is more like:
#ifndef THISHEADER_H
#define THISHEADER_H

//header code goes here
#endif



#pragma once is a compiler specific include guard (aka non-standard). It is preferable to macro include guards except when you require portability to compilers which do not support this compiler directive.

IMHO, since every "respectable" compiler supports #pragma once, I think it should always be used over macros.

This can cause confusion when porting though. This is because the standard dictates that when a compiler encounters a #pragma directive it doesn't understand it should silently ignore it.

Share this post


Link to post
Share on other sites
Seeing as this really is a lot of code to look over, I only read random parts.

Deck: The destructor here is completely unnecessary (I think). When the vector goes out of scope, it calls the destructor of what it contains for you. For a pointer, this would be the delete keyword. So, what you do manually, should be done for you.

The time when you should be concerned is when you have a vector of pointers to objects that have pointers to objects on the heap. In other words: A vector with pointers to objects that need to deallocate space. In that case, you'll want to call the object's destructor, THEN let the vector get rid of the pointers.

The only there thing that annoyed me is that you sometimes, when a function takes no arguments, write (void) in replace of (). There's really no difference except that your compiler probably ignores the keyword void.

Everything else looks good, but yeah, it's a lot of code to look over. My advice is, if you want help like this in the future, be confident/write code you KNOW is good and only ask about the new stuff/things you aren't sure about.

Share this post


Link to post
Share on other sites
About the fn(void) vs fn() when you add a new class using Visual C++ it will auto create the constructors and destructors for you. When it does that it writes Class(void) and ~Class(void). I hate it, but I guess I missed a few.

Regarding the destructor in deck. I didn't know that vectors would do that for you. I always assumed that if I didn't do that it would leak. I'll look into that.

Thanks a bunch for all your input!

Share this post


Link to post
Share on other sites
You are right. A vector definitely won't free the memory that you have allocated. What you don't need to do is pop_backing the items one by one.

However, since you needed to write a destructor, this means that you should provide a suitable copy constructor and copy assignment operator (or disable them by declaring them private). Otherwise user code might try to make a copy of deck, meaning that there would be now two vector<Card*> pointing to the same cards, and both Decks will try to free the same memory.

But are you sure you need to allocate the cards dynamically and store pointers instead of Cards themselves? Then you would need neither destructor nor copy constructor/assignment operator.

Another thing I noticed in the Deck class was the implementation of shuffle. I'm quite sure that random_shuffle will do the same job quicker and possibly better (apart from requiring less typing).

Share this post


Link to post
Share on other sites
Quote:
Original post by fpsgamer
IMHO, since every "respectable" compiler supports #pragma once, I think it should always be used over macros.


It's not Microsoft-specific? News to me. Anyway, I prefer using standard language (and preprocessor) features on principle. In any case, it doesn't hurt to use both: compilers not supporting the pragma will just skip that line, and compilers supporting it will still get the potential benefits (not re-opening the file to check for include guards) without breaking anything.

That, of course,

Quote:
is because the standard dictates that when a compiler encounters a #pragma directive it doesn't understand it should silently ignore it.


:D

Quote:
Original post by visitor
You are right. A vector definitely won't free the memory that you have allocated. What you don't need to do is pop_backing the items one by one.


@OP: vectors (standard library containers, in general) free the memory they allocate (i.e. for the elements); you free the memory you allocate (i.e. the pointed-at stuff pointed at by the elements, when they are pointers, if you allocated that stuff). However, you first strongly consider if you need to store elements by pointer in the first place. Why do you think you are doing this as it stands?

Share this post


Link to post
Share on other sites
I think a good way to clean up your code (and remove potential future headaches) would be to boost::shared_ptr for your dynamically allocated objects. At the very least it will remove the need for that ugly "iterate, pop, delete" stuff in the destructor.

Share this post


Link to post
Share on other sites
Quote:
Original post by starruler
About the fn(void) vs fn() when you add a new class using Visual C++ it will auto create the constructors and destructors for you. When it does that it writes Class(void) and ~Class(void). I hate it, but I guess I missed a few.

Regarding the destructor in deck. I didn't know that vectors would do that for you. I always assumed that if I didn't do that it would leak. I'll look into that.

Thanks a bunch for all your input!


Forgot to say you have to call your_vector.clear();

clear() WILL call the destructor of whatever is the problem, and if instead of having your vector point to objects, you could have a pointer to your vector of objects on the heap!--thus bypassing having to do anything in that constructor.

Share this post


Link to post
Share on other sites
So you think I should refactor to use Card instead of Card* everywhere. I used a pointer because I figured that constructing a card every time that I passed it to a function or returned it from a function was silly. Is that something that I shouldn't worry about? The Card class is very lightweight and the constructor is lightweight as well.

Share this post


Link to post
Share on other sites
Just because a varible is on the stack does not mean you need to copy it in order to pass it to a varible...


std::vector<Card> Cards;


void DoSomething(Card* c);
void DoSomethingElse(Card &c);

...
DoSomething(&Cards[0]);//pass a pointer to the card
DoSomethingElse(Cards[0]);//pass the card by refrence



In neither case is a copy of the card made.

I asked about returning before and apperntly most optimising compilers will cut out the extra copies when they see that each time the origenal gets deleted (eg when it goes out of scope at the function end, having returned a "copy")

EDIT: Topic about additional copies being made

Share this post


Link to post
Share on other sites
I had thought about using references, I don't remember what my rationale was for not using them. Refactor is going to be a pretty big undertaking, but thankfully a lot of the code is in one place.

EDIT: Now I remember. If I return by reference from my Deck::deal() function then that card becomes invalid as it's removed from the vector of cards in Deck and then deleted. So that's one function that needs to return a copy. Basically any function that is return a card needs to return a copy. When I pass a card as an argument I can use a const reference to say a copy. Does that sound about right?

[Edited by - starruler on June 24, 2008 6:05:36 PM]

Share this post


Link to post
Share on other sites
Man, I'm running into a host of problems. For one, I used a return value of NULL to indicate that a player wanted to draw cards. Well, I can't do that if I'm not returning a pointer. I guess I could create a card that had some specific value related to drawing more. That seems just plain wrong though. A card is not to be used like flag, it's a representation of a card.

I think the solution to my problem lies in something that I was planning to do somewhere down the road. I was going to re-write the game using the model-view-controller pattern. Then a player would get the message that it was their turn from the game. They would then send a message back to the game, either the card they wanted to play, or a message stating they want to draw cards.

Is there an easier way?

This brings up a bunch of questions that I have. Like what parts of my program should be models? I don't know if a card should be a model because it doesn't really need a controller. A player seems to be a good choice for a model because it needs a view and a controller.

EDIT: On a side note, std::vector.clear does not delete pointers. Re: http://msdn.microsoft.com/en-us/library/fs5a18ce(VS.80).aspx

Share this post


Link to post
Share on other sites

This topic is 3464 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this