Sign in to follow this  
sheep19

Book exercise. Is it correct?

Recommended Posts

sheep19    494
Hello. I have finished the chapter on Advanced Classes of Beginning C++ through game programming. The last program of the chapter is this:
#include <iostream>
#include <string>
using namespace std;

class Player
{
public:
	Player( const string& name = "" );
	string GetName() const;
	Player* GetNext() const;
	void SetNext( Player* next );

private:
	string m_Name;
	Player* m_pNext;
};

Player::Player( const string& name ): m_Name( name ), m_pNext( 0 )
{
}

string Player::GetName() const
{
	return m_Name;
}

Player* Player::GetNext() const
{
	return m_pNext;
}

void Player::SetNext(Player* next)
{
	m_pNext = next;
}

class Lobby
{
	friend ostream& operator << ( ostream& os, const Lobby& aLobby );

public:
	Lobby();
	~Lobby();
	void AddPlayer();
	void RemovePlayer();
	void Clear();

private:
	Player* m_pHead;
};

Lobby::Lobby(): m_pHead( 0 )
{
}

Lobby::~Lobby()
{
	Clear();
}

void Lobby::AddPlayer()
{
	//create a new player node
	cout << "Please enter the name of the new player: ";
	string name;
	cin >> name;
	Player* pNewPlayer = new Player( name );

	//if list is empty, make head of list this new player
	if( m_pHead == 0 )
		m_pHead = pNewPlayer;
	
	//otherwise find the end of the list and add the player there
	else
	{
		Player* pIter = m_pHead;
		while( pIter->GetNext() != 0 )
		{
			pIter = pIter->GetNext();
		}
		pIter->SetNext( pNewPlayer );
	}
}

void Lobby::RemovePlayer()
{
	if( m_pHead == 0 )
	{
		cout << "The lobby is empty. No one to remove.\n";
	}
	else
	{
		Player* pTemp = m_pHead;
		m_pHead = m_pHead->GetNext();
		delete pTemp;
	}
}

void Lobby::Clear()
{
	while( m_pHead != 0 )
	{
		RemovePlayer();
	}
}

ostream& operator << ( ostream& os, const Lobby& aLobby )
{
	Player* pIter = aLobby.m_pHead;

	os << "\nHere's who's in the lobby:\n";
	if( pIter == 0 )
		os << "The lobby is empty.\n";
	else
	{
		while( pIter != 0 )
		{
			os << pIter->GetName();
			pIter = pIter->GetNext();
		}
	}

	return os;
}

int main()
{
	Lobby myLobby;
	int choice;

	do
	{
		cout << myLobby;
		cout << "\nGAME LOBBY\n";
		cout << "0 - Exit the program.\n";
		cout << "1 - Add a player to the lobby.\n";
		cout << "2 - Remove a player from the lobby.\n";
		cout << "3 - Clear the lobby.\n";
		cout << "\nEnter choice: ";
		cin >> choice;

		switch( choice )
		{
		case 0: cout << "Good bye.\n"; break;
		case 1: myLobby.AddPlayer(); break;
		case 2: myLobby.RemovePlayer(); break;
		case 3: myLobby.Clear(); break;
		default: cout << "That was not a valid choice.\n";
		}
	} while( choice != 0 );

	system( "PAUSE" );
	return 0;
}



The author says that the Lobby::AddPlayer() function is inefficient because it iterates through all the objects to add one. So the exercise is to make it efficient by adding an m_pTail member function. This is what I've done:
void Lobby::AddPlayer()
{
	//create a new player node
	cout << "Please enter the name of the new player: ";
	string name;
	cin >> name;
	Player* pNewPlayer = new Player( name );

	//if list is empty, make head of list this new player
	if( m_pHead == 0 )
	{
		m_pHead = pNewPlayer;
		m_pTail = pNewPlayer;
	}
	
	//otherwise find the end of the list and add the player there
	else
	{
		m_pTail->SetNext( pNewPlayer );
                m_pTail = pNewPlayer;
	}
}



Is it correct? Thanks for your time.

Share this post


Link to post
Share on other sites
agm_ultimatex    191
Heh, I remember this exercise from the book. I think that looks correct. If you're using VStudio of some sort, can always debug and follow it through. However I found with c++ that the debugger leaps through the vector classes and stuff, making it very annoying to debug.

Share this post


Link to post
Share on other sites
Antheus    2409
Did you update void Lobby::RemovePlayer() to update m_pTail? If not, then if you add, remove and add a player, the application will either crash or produce weird results.

Share this post


Link to post
Share on other sites
sheep19    494
Quote:
Original post by Antheus
Did you update void Lobby::RemovePlayer() to update m_pTail? If not, then if you add, remove and add a player, the application will either crash or produce weird results.


Should I? It's the first person who's being removed, not the last. :)

Correct me if I'm wrong.

Share this post


Link to post
Share on other sites
rip-off    10979
There are some issues. It would appear that it would work correctly (although it would be really nice if the tail pointer was set to 0 if the list is emptied by a call to RemovePlayer, as a C++ programmer you will learn to hate dangling pointers).

Firstly, unless the order of the players in the lobby is important for some reason, you could insert the new player at the start of the list, eliminating the need to hold a reference to the tail.

The more pressing issue is of class responsibility. While I understand that the book might be trying to demonstrate the concepts of a linked list, it is still important to think about them the right way. We can separate the "linked list" from the "things in the list" fairly easily:

class Player
{
public:
Player( const string& name = "" );
string GetName() const;

private:
string m_Name;
};

class PlayerList
{
PlayerList();
~PlayerList();

// both these functions could take an integer parameter
// this value would indicate where to insert or remove
void insert(const Player &);
void remove();

void clear();

private:

struct PlayerNode
{
Player player;
PlayerNode *next;
};

PlayerNode *head;
};

class Lobby
{
// operations
// can't implement operator<< yet

private:
PlayerList players;
};



Each class now has a defined task. The Player only cares about a single players information, it doesn't need to know about (or be responsible for) any other players. The PlayerList class is responsible for holding an arbitrary number of players. It uses the inner structure PlayerNode to represent the links in the list. Finally, the Lobby is an extension of the user interface (I suppose), that prompts the user for information, etc(it could be removed, and main could interact with the PlayerList directly).

While this is more complicated looking, this has benefits that aren't immediately obvious. For example, with your current implementation, anyone who has a reference to a Player object can modify the list the player is in! This is obviously undesirable, it could cause no end of bugs.

The above could be simplified using templates, but that truly is for later. Also, the above does not allow one to iterate over the PlayerList and do something with each player, this is actually relatively easy to implement but I'll leave it out of the interface for the moment.

If you are interested in going beyond the task you have been given, then you could implement the above to the best of your ability. Most of it involves moving code from the lobby into the PlayerList. From there, we could make more suggestions to expand the interface of PlayerList to a fully functional linked list. Writing your own linked list is always an important moment for every aspiring programmer.

Once you have written your own version, you will be able to throw it away and learn the joy that is std::list<>. [smile]

Share this post


Link to post
Share on other sites
Antheus    2409
Quote:
Original post by sheep19
Quote:
Original post by Antheus
Did you update void Lobby::RemovePlayer() to update m_pTail? If not, then if you add, remove and add a player, the application will either crash or produce weird results.


Should I? It's the first person who's being removed, not the last. :)

Correct me if I'm wrong.


It's ok, I wrote too fast. I had a generic linked list remove in mind.

Share this post


Link to post
Share on other sites

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