Hi, I'm reading the c++ book Beginning C++ Game Programming by Michael Dawson and one of the exercises is to make a copy constructor for a game Lobby object which has a linked list of Player objects on the heap. It's supposed to create a new memory address for all the copy's data members so that the list pointers don't point to the same instances as the original.
I created a constructor that seems to work, but I think it might more complicated than needed.
Here's the Lobby class:
class Lobby
{
friend ostream& operator<<(ostream& os, const Lobby& aLobby);
public:
Lobby(): m_pHead(0) {}
~Lobby() {Clear();}
Lobby(const Lobby& l);
void AddPlayer();
void RemovePlayer();
void Clear();
private:
Player* m_pHead; //First Player on list
};
I haven't read the code in detail, but it's about the length I expect for copying a linked list. The last time I wrote something like that was about 15 years ago (Thank you standard C++ library!) and I seem to remember that using double pointers simplified a lot of the logic in this type of code.
How else do you think it'd be possible to do something like you did that's simplier? The method you've chosen looks good to me, and is probably the best method for this job
I'm not convinced your copy constructor currently works. It appears to create copies of each of the members in the original list, but the pointer in the head node is still pointing at the original list. The copied nodes are leaked.
You seem to be getting lost in your own variable names. pHeap is created, assigned and then never used for anything.
It's also weird that the code in front of your loop is exactly the same as the one inside the loop. Also, where are you setting up the next pointers? From what little code I can see, all your nodes next pointers are either pointing to the original list or to random garbage.
Your test should be to print out both lists with the players name and the address. If your code is working, all the names must be the same, but the addresses must be different.
If you don't mind a bit of headache, you can usually avoid all the "is this the head?" or "is the head NULL?" special cases by using pointer to pointer (ie. pointer to next pointer instead of using next pointer directly, so your head elements turns into just the first "next" element). But that isn't something to worry about until stuff is actually working in the first place.
Doesn't that create a new address for the head node?
[/quote]
You are allocating nodes, but you are not linking them correctly. Note that code is copying the Player instance, which implicitly copies the "next" pointer. Thus, after that line the head->next of both Lobbies point to the same sublist. Since head->next isn't subsequently modified, the loop following effectively just leaks a bunch of memory.
If not, how would I test it?
[/quote]
Trienco's suggestion of printing both the names and addresses of the Player instances is a good one, you can visualise the list that way.
Consider:
Lobby::Lobby(const Lobby& other)
{
if(other.head)
{
this->head = new Player(other.head->GetName());
Player *previous = this->head;
for(Player *current = other.head->next; current ; current = current->next)
{
Player *copy = new Player(current->GetName());
previous->SetNext(copy);
previous = copy;
}
}
else
{
this->head = nullptr;
}
}
I haven't tested this, but I believe it should work. I think it is slightly cleaner because it initialises as opposed to copies, and avoids copying pointers from the source list into the new list (even temporarily).
You could also go with the pointer to a pointer, but I find that more difficult to reason about.
Finally, neither is exception safe, these implementations will leak memory if one of the Player allocations or constructors throws an exception. But I won't go into too much detail in "For Beginners" on this point.
