Creating a list of object pointers

Question

I am attempting to create a list(which is a private memeber of an object) of object pointers ...

So the program I am coding is a simulation between two groups of "Hero"s. Each hero has special powers, physical attributes ect... and a list of targets. The list of targets is a list of pointers to a Hero. When I call the constructor to create a Hero all of the information is initialized with random values with the exception of the list of targets. So far I have created two list's of Hero's, team 1 and team 2. I am trying to create a list of pointers in team 1 which points to the Heros address's in team 2, and vice versa. The series of for loops in main() is the best solution I have but the function Hero::setTarget "pushes_back" the address of the first Hero multiple times. Any advice would be greatly appreciated.

Here what I have so far excluding the Powers.h and Powers.cpp files ...

EDITED : removed nested for loops, but I am still getting the same address passed into my list ....

    #include <iostream>
    #include <list>
    #include <random>
    #include <ctime>

    #include "Hero1.h"
    #include "Powers.h"

    using namespace std;

    int main()
    {
 ///random number generator
    default_random_engine generator(time(NULL));
    uniform_int_distribution<int> numHero(1,10);

///lists of hero's, pointers to a hero, list iterators
    list<Hero> team1;
    list<Hero> team2;
    Hero * hptr1;
    Hero * hptr2;
    list<Hero>::iterator hitr1;
    list<Hero>::iterator hitr2;

/// team 1 is created
    int a =  numHero(generator);
    for(int x=0; x<a; x++)
    {
    hptr1 = new Hero();
    team1.push_back(*hptr1);
    }

/// team 2 is created
    int b = numHero(generator);
    for(int x=0;x<b;x++)
    {
    hptr2 = new Hero();
    team2.push_back(*hptr2);
    }

for(hitr2=team2.begin();hitr2!=team2.end();hitr2++)
{
        hptr1->setTarget(hptr2);
}

for(hitr1=team1.begin();hitr1!=team1.end();hitr1++)
{
    hptr2->setTarget(hptr1);
}

 ///printing results for list of targets
 int w =1;
 cout<<"target address "<<w<<endl;
        hitr1=team1.begin();
        hptr1->displayTarget();

Hero is my Hero.h

#ifndef HERO1_H_INCLUDED
#define HERO1_H_INCLUDED

#include <iostream>
#include <random>
#include <ctime>
#include <list>

#include "Powers.h"


using namespace std;

class Power;

class Hero
{
public:

    Hero();
   // ~Hero();

    void setID();
    void setLoc();
    void setPhy();
    void setPowers();
    void setEquip();
    void setTarget(Hero *h);

    int getID(){return id;}
    int getLoc(int x);
    int getPhy(int x);

    void displayHero();
    void displayTarget();
    void displayPowers();


private:
    int id;
    int location[3];
    int physical [4];
    Power * powPtr;
    Power * equipPtr;
    Hero * targetPtr;
    list<Power> powers;
    list<Power> equipment;
    list<Hero*> target;
    list<Power>::iterator equipItr;
    list<Power>::iterator powItr;
    list<Hero*>::iterator targetItr;

};

My Hero.cpp

#include "Hero1.h"

default_random_engine generator(time(NULL));
uniform_int_distribution<int> distribution(100000,200000);
normal_distribution<double> disto(50,10);
uniform_int_distribution<int> randPow(1,3);

Hero::Hero()
{
    setLoc();
    setID();
    setPhy();
    setPowers();
    setEquip();
}

void Hero::setLoc()
{
    location[0] = 1;
    location[1] = 2;
    location[2] = 3;
}

void Hero::setID()
{
    int a = distribution(generator);
    id = a;
}

void Hero::setPhy()
{
    double a = disto(generator);
    double b = disto(generator);
    double c = disto(generator);
    double d = disto(generator);

    physical[0] = a;
    physical[1] = b;
    physical[2] = c;
    physical[3] = d;

}

void Hero::setPowers()
{
    int a = randPow(generator);
    for(int x=0;x<a;x++)
    {
    powPtr = new Power(getPhy(3));
    powers.push_back(*powPtr);


    }
}


void Hero::setEquip()
{
    int a = randPow(generator);
    for(int x=0;x<a;x++)
    {
    equipPtr = new Power(getPhy(3));
    powers.push_back(*equipPtr);
    }

}
void Hero::setTarget(Hero *h)
{

    target.push_back(h);

}

void Hero::displayTarget()
{
    int x =1;
    for(targetItr=target.begin();targetItr!=target.end();targetItr++)
    {
        cout<<&targetPtr<<endl;
        x++;
    }
    cout<<x<<endl;

}

int Hero::getLoc(int x)
{
    int p;
    p = location[x];
    return p;
}

int Hero::getPhy(int x)
{
    int p;
    p = physical[x];
    return p;
}
void Hero::displayPowers()
{
    cout<<"Number of powers = "<<powers.size()<<endl<<endl;
    for(powItr=powers.begin();powItr!=powers.end();powItr++)
    {
        powPtr->displayEffect();
    }
}

void Hero::displayHero()
{
    cout<<"Id :\t\t\t\t\t"<<id
        <<"\nLocation:\t\t\t\t"<<location[0]<<","<<location[1]<<","<<location[2]
        <<"\nPhysical attributes:\tstrength\t"<<physical[0]<<"\n\t\t\tendurance\t"<<physical[1]
        <<"\n\t\t\tagility\t\t"<<physical[2]<<"\n\t\t\tspeed\t\t"<<physical[3]<<endl<<endl;

        displayPowers();
}

Answer 1

Following code sections from your original program also need modification:

void Hero::displayTarget()
{
    int x = 0;                                  <-- change done here (not important)
    for (targetItr=target.begin(); targetItr!=target.end(); targetItr++)
    {
        cout << *targetItr << endl;              <-- change done here
        x++;
    }
    cout<<x<<endl;
}

NOTE:

1. targetPtr member in Hero class is not initialized in your original code.
2. In displayTarget(), always targetPtr was printed but not updated with every iteration targetItr.

Further, change in the display code in main():

cout<<"target addresses in team 1"<<endl;
        hitr1=team1.begin();
        hptr1 = *hitr1;                        <-- change done here
        cout<<"address of team 1 pointer "<<hptr1<<endl;
        hptr1->displayTarget();
        cout<<endl<<"-------------"<<endl;
        cout<<"target addresses in team 2"<<endl;
        hitr2=team2.begin();
        hptr2 = *hitr2;                        <-- change done here
        cout<<"addreass of team 2 pointer "<<hptr2<<endl;
        hptr2->displayTarget();

Hope this helps you. All the Best.

Answer 2

I agree with the remarks by @TheUndeadFish.

Further, I add following observations:

list<Hero> team1;
list<Hero> team2;

Instead of keeping list of objects of "Hero", it would be suggested to keep list of pointers to "Hero" objects

list<Hero*> team1;
list<Hero*> team2;

The above suggested method is helpful as team1 heros keeps list of team2 heros and vice versa, so this will help in accessing the updated information of each hero as pointer/references help in accessing dynamic change in object data.

Further, the for loops do not have initialization of hptr1 and hptr2 in original code. It can be done as follows:

///lists of hero's, pointers to a hero, list iterators
    list<Hero*> team1;                    <-- change done here
    list<Hero*> team2;                    <-- change done here
    Hero * hptr1;
    Hero * hptr2;
    list<Hero*>::iterator hitr1;          <-- change done here
    list<Hero*>::iterator hitr2;          <-- change done here

/// team 1 is created
    int a =  numHero(generator);
    for(int x=0; x<a; x++)
    {
    hptr1 = new Hero();
    team1.push_back(hptr1);               <-- change done here
    }

/// team 2 is created
    int b = numHero(generator);
    for(int x=0;x<b;x++)
    {
    hptr2 = new Hero();
    team2.push_back(hptr2);               <-- change done here
    }

for(hitr2=team2.begin();hitr2!=team2.end();hitr2++)
{
    hptr2 = *hitr2;                        <-- change done here
    for(hitr1=team1.begin();hitr1!=team1.end();hitr1++)
    {
        hptr1 = *hitr1;                    <-- change done here
        hptr1->setTarget(hptr2);
    }
}

for(hitr1=team1.begin();hitr1!=team1.end();hitr1++)
{
    hptr1 = *hitr1;                        <-- change done here
    for(hitr2=team2.begin();hitr2!=team2.end();hitr2++)
    {
    hptr2 = *hitr2;                        <-- change done here
    hptr2->setTarget(hptr1);
    }
}

Answer 3

I see a couple different problems in your code, but lets start with the one you asked about...

Your setTarget calls are doing the same thing every time because they're being called with pointers that aren't being updated at all within the loops. This is a problem that can come from declaring all of your variables at the top of a function. Variables that outlive their purpose are vulnerable to mistakes like this (or being reused to multiple purposes and then making code harder to understand and maintain). So I highly recommend scoping local variables to exactly where they're needed.

For example:

/// lists of heroes
    list<Hero> team1;
    list<Hero> team2;

/// team 1 is created
    int a = numHero(generator);
    for(int x=0; x<a; x++)
    {
        Hero * hptr1 = new Hero();
        team1.push_back(*hptr1);
    }

/// team 2 is created
    int b = numHero(generator);
    for(int x=0;x<b;x++)
    {
        Hero * hptr2 = new Hero();
        team2.push_back(*hptr2);
    }

for(list<Hero>::iterator hitr2=team2.begin();hitr2!=team2.end();hitr2++)
{

    for(list<Hero>::iterator hitr1=team1.begin();hitr1!=team1.end();hitr1++)
    {
        hptr1->setTarget(hptr2);
    }
}

for(list<Hero>::iterator hitr1=team1.begin();hitr1!=team1.end();hitr1++)
{
    for(list<Hero>::iterator hitr2=team2.begin();hitr2!=team2.end();hitr2++)
    {
        hptr2->setTarget(hptr1);
    }
}

If you did that, then you could get compile errors because hptr1 and hptr2 don't exist where setTarget is being called. And that's exactly what you want - the compiler to call your attention to a mistake.

So then those loops can be corrected as something like:

for(list<Hero>::iterator hitr2=team2.begin();hitr2!=team2.end();hitr2++)
{

    for(list<Hero>::iterator hitr1=team1.begin();hitr1!=team1.end();hitr1++)
    {
        Hero& target = *hitr2;
        hitr1->setTarget(&target);
    }
}

Next, for the second problem: you're leaking memory when filling the lists:

Hero * hptr1 = new Hero();
team1.push_back(*hptr1);

That will dynamically allocate a Hero but then copy the values of it into another Hero that gets created within the list - because that's how push_back works with something like list<Hero>. And then the pointer to the dynamically allocated one is thrown away (or re-used in the code you originally posted), so nothing can ever deallocate that memory.

What you actually want to do is more like:

team1.push_back(Hero());

Answer 4

I think what I have to say is more worthy of being a comment then an answer to this question, however StackOverflow's bizarre rules will not permit me to leave comments. That said, I think you should rename "setTarget" to "addTarget", as "setTarget" implies that there is a single value to the variable that will be overwritten every time you call it, while "addTarget" better communicates that you will be adding the target to a list.

Also, you should consider using std::for_each with a lambda function rather than the explicit loops. I doubt it would make any noticeable difference in terms of performance or correctness, but it would make your program more readable and learning about the STL functions will help you improve your programming skill.