Is this a constructor problem?

[kutuup]

Hey all

I have a dynamic array of pointers to instances of a class ("Player").

The constructor for the Player class looks like this:

Player::Player(void)
{
    this->leftChild = 0;
    this->rightChild = 0;

    srand (time(NULL));
    this->id = rand() % 1000;


}

The dynamic array of pointers looks like this:

Tree* PlayerTree = new Tree; //declaring a tree here for use later
Player** players;
players = new Player* [5];

Player* p1 = new Player(1);
players[0] = p1;
Player* p2 = new Player(2);
players[1] = p2;
Player* p3 = new Player(3);
players[2] = p3;
Player* p4 = new Player(4);
players[3] = p4;
Player* p5 = new Player(5);
players[4] = p5;

Then I insert the instances of Player from the array into the tree I declared above using a member function of my Tree class:

PlayerTree->Insert(players[1]);
PlayerTree->Insert(players[0]);
PlayerTree->Insert(players[3]);
PlayerTree->Insert(players[2]);
PlayerTree->Insert(players[4]);

I then call a function of Tree which displays the "id" data member of each node in order (the "id" should be random based on the constructor).

However, I get this:

Player ID: 500 (random number)
Player ID: 500
Player ID: 500
Player ID: 500
Player ID: 500 (all the SAME random number!)

Why might this be? Surely the constructor is called each time a new instance is created, and in the constructor I re-seed the rand() function so it should re-seed each time it is called.

I should point out that the Player class is a child of a class called GameObject whose constructor is blank (default). Could this be a factor?

Thanks for any help!

James

[StuXYZ]

First welcome to Daniweb, second please use code tags! It make life easier for all of us.

The problem is possible with the use of srand(time(NULL)); . What you are doing is re-initializing the random number generator with a number and that number is from the time. HOWEVER, time(NULL) normally only gives an answer to the nearest millisecond, I guess that you are reinitializing the random number generator with the same seed number each so the first random number is always the same.

To cure the problem put srand(time(NULL)); or as easier for debugging,
write something like srand(53234231); but put in just after int main() and nowhere else.

Note: 53234231 is not special, it can be absolutely any number (except zero )

Hope that helps

[kutuup]

Ah, that's sorted it! Thanks!

But now the display doesn't show the instances in order like it did before.

If I change the constructor to accept an integer then assign the id data member that number then pass a different number each instance, they display in order even if I input them in a random order.

for example

Player* p1 = new Player(0);
	players[0] = p1;
	Player* p2 = new Player(3);
	players[1] = p2;
	Player* p3 = new Player(1);
	players[2] = p3;
	Player* p4 = new Player(7);
	players[3] = p4;
	Player* p5 = new Player(9);
	players[4] = p5;

(sorry if i messed up the code tags.)

If i do that they are put in the right order when they output.

Why might this be?

[StuXYZ]

Firstly you didn't mess up the code tags, thanks for using them.

Second, without all of your code I cannot tell, but you are putting the Player pointers into a tree. That is most likely ordered, and hence the output is ordered.

Can I just add, if you do this

std::cout<<*p1<<std::endl;
std::cout<<*p2<<std::endl;
// etc...

do you get the right order e.g. 0,3,1 etc ? If so then it is that your tree is ordered. [Did you write the tree ??]

[kutuup]

The code for the tree was provided by my tutor and look like this:

#pragma once
#include "Player.h"

class Tree
{

private:
	
public:
	Player* root;
	Tree(void);
	~Tree(void);

	void DisplayInOrder(Player* localRoot)
	{
		        if (localRoot != 0) {
                DisplayInOrder(localRoot->leftChild);
				localRoot->Display();
                DisplayInOrder(localRoot->rightChild);
				}
	}

	Player* Find(const Player* key)
	{
		            Player * current = root;
            while (current != key) {
				if (key < current)
                    current = current->leftChild;
                else
                    current = current->rightChild;
                if (current == 0)
				{
				    cout << endl << "Player Not Found!" << endl << endl;
                    return 0;

				}
            }
			cout << endl << endl << "Found Player! ";
			current->Display();
			cout << endl << endl;
            return current;
	}

	void Insert(Player* insertedPlayer)
	{

		if(root == 0)
	    {
		     root = insertedPlayer;
	     }
	     else
	     {
		     Player * current = root;
		     Player * parent;
		     while(true)
		     {
			     parent = current;
				 if(insertedPlayer < current)
			      {
				     current = current->leftChild;
					 if(current == 0)
					     {
						     parent->leftChild = insertedPlayer;
						     return;
					     }
			     }
			      else
			     {
			       current = current->rightChild;
				   if(current == 0)
				     {
							     parent->rightChild = insertedPlayer;
							     return;
				     }
			
			     }
		     }
	     }

	}

};

I have overloaded the < and != operators for the Player class and set them to compare instanced based on their "id" data member which is randomly generated in the constructor.

I can't see why they output in the order they were put in rather than in order of id since the displayInOrder function seems to be fine.

[StuXYZ]

Well that is pretty strange!

The obvious thing to to write a tree container that contains the objects in the tree, either as pointers or directly [sometimes both].

Here, we have class Tree, that has a single pointer to Player.

Then let us figure out if anything actually works:

First off is the find method, as fast as I can see it assumes that the tree is ordered by numerical comparison of the pointer values. So overloading the < and != operators for Player has no impact.

// Your previous post
PlayerTree->Insert(players[1]);
PlayerTree->Insert(players[0]);
PlayerTree->Insert(players[3]);
PlayerTree->Insert(players[2]);
PlayerTree->Insert(players[4]);

Now assuming that you have pointers that increase as you got from players[0] -> [5]

You then call a display order that DOESN'T use the root player, but whatever item you give, but let us assume that you put in players[1]. Your tree after the inserts looks like this

4
          / 
        3     
      /   \
    1       2
      \
        0

All other entries should be null ptr.

Then the display should be 0 : 1 : 2 : 3 : 4 . If you don't believe me,
the step through the tree from the diagram. : Node 1, has a left, so go to 0, ok so it doesn't have a left, display 0 , no right, so go up to 1 again, display etc..

Does this help?

[mike_2000_17]

If your tutor told you that overloading the < operator for the Player class would make the tree ordered with respect to that comparison's logic, then he made a mistake in his implementation of the Tree class. This implementation of the Tree class compares the values of the pointers, not the objects they point to. This is either a mistake on his part or you have misunderstood how the ordering in the Tree should occur (if the Tree code is correct, it means that they ought to be ordered by the pointer value, not by value).

I don't understand way your tutor would provide a Tree implementation when there is a standard implementation of a sorted list, it is called std::set. It could be used as follows:

bool comparePlayers(const Player* p1, const Player* p2) {
  if(!p1) return false;
  if(!p2) return true; //relegate all NULL pointers to the end, if they exist (unlikely).
  return *p1 < *p2; //compare by value.
};

typedef std::set<Player*, bool (*)(const Player*,const Player*)> Tree;

int main() {
  //.. .as before...
  Tree PlayerTree(&comparePlayers);
  PlayerTree.Insert(players[1]);
  PlayerTree.Insert(players[0]);
  PlayerTree.Insert(players[3]);
  PlayerTree.Insert(players[2]);
  PlayerTree.Insert(players[4]);
  for(Tree::iterator it=PlayerTree.begin(); it!= PlayerTree.end(); ++it)
    (*it)->Display();
  //...
};