30 replies to this topic

[Ubermeowmix]

vector<Animal> myAnimals; // globally in main

// I then fill myAnimals with a for loop that reads back perfectly and shows names and health properly.
// e.g. Animal newAnimal(animalsNames[i], 10 + i); // animalsNames is just a string array
// myAnimals.push_back(newAnimal);

AttackAnimals(2);

void AttackAnimals(int dam)
{
for(int i = 0; i < myAnimals.size(); i++)
{
myAnimals[i].SetHealth(dam);
};

for ( vector<Animal>::iterator iter = myAnimals.begin(); iter != myAnimals.end(); /* empty */ )
{
if ( iter->Check4Death() )
{
cout << iter->GetName() << " has died!\n";
iter = myAnimals.erase( iter );
} else {
iter ++;
}
}
};

// the following are the functions from the class

void Animal::SetHealth(int minus)
{
*m_pHealth -= minus;
}

bool Animal::Check4Death()
{
if(*m_pHealth <= 0)
return true;
else
return false;
};

Edited by Ubermeowmix, 26 July 2012 - 11:04 AM.

[FredOrJoeBloggs]

Especially the redundant nulling of the pointers, its good practice, but might have performance issues in games.

Oh in what sense is that a bad thing, as this is definitely games coding!

It depends on what your compiler does.
If it compiles the code as is, then it will waste a few instructions.
If the compiler optimises the code, it might well remove the code.
If you want speed remove redundent code before you compile.

However, I can't see why you are using dynamic memory for this data, why not just let the class members be a string and an int? I apreciate you may have simplified it for the question, in which case ignore this comment.

So if i want to store floating point numbers and strings they can just be basic type def calls and that will save on CPU power will it?, what would I need to use the dynamic memory for? just the initial instantiation of the class (or Animal) into a vector (as in larspensjo's example), and then delete it if the object dies in game for instance.

Dynamic memory is useful if you don't know who many of the objects you are going to have. If you know how many objects of a certain type you will have at maximum AND the amount of memory that uses doesn't hinder the running of the program then use stack memory. However since you are using a std::vector you are already using dynamically allocated memory (al be it managed in a library for you). The objects that you are placing into the vector can have dynamic memory, but they could also be 'non-dynamic'.
If performance is an issue then write your code such that you can replace the way the data is stored without affecting the rest of the code and then bench mark the two options by performing so many thousand operations that will excersize the code.
[source lang="cpp"]#include <afx.h>#include <string>#include <vector>class CAnimalsD{ // Animal class using dynamic memorypublic: typedef std::vector<CAnimalsD> VECTOR;public: CAnimalsD(const std::string& name = "", const int& health = 2) : m_pName(new std::string(name)) , m_pHealth(new int(health)) {} CAnimalsD(const CAnimalsD& other) : m_pName(new std::string(*other.m_pName)) , m_pHealth(new int(*other.m_pHealth)) {} ~CAnimalsD() { if (m_pName) delete m_pName; if (m_pHealth) delete m_pHealth; }private: std::string* m_pName; int* m_pHealth;};class CAnimalsS{ // Animal class using static memorypublic: typedef std::vector<CAnimalsS> VECTOR;public: CAnimalsS(const std::string& name = "", const int& health = 2) : m_Name(name) , m_Health(health) {} CAnimalsS(const CAnimalsS& other) : m_Name(other.m_Name) , m_Health(other.m_Health) {} ~CAnimalsS(){}private: std::string m_Name; int m_Health;};// Gash test program.int main (void){ static const int constTestCycles = 10000; CAnimalsD::VECTOR animalsD; CAnimalsS::VECTOR animalsS; const DWORD startTime = GetTickCount(); for (int i = 0; i < constTestCycles; ++i) { animalsS.push_back(CAnimalsS("FRED", i)); } animalsS.clear(); const DWORD midTime = GetTickCount(); for (int i = 0; i < constTestCycles; ++i) { animalsD.push_back(CAnimalsD("FRED", i)); } animalsD.clear(); const DWORD endTime = GetTickCount(); printf("dynamic test = %d\n", midTime - startTime); printf("static test = %d\n", endTime - midTime); printf("press enter to exit\n"); return fgetc(stdin);}[/source]

[Mussi]

[source lang="cpp"]void AttackAnimals(int dam){for(int i = 0; i < myCritters.size(); i++){myAnimals[i].SetHealth(dam);};[/source]

The funtion is called AttackAnimals, but your iterator ranges from 0 to the size of myCritters, shouldn't that be myAnimals( this is dangerous because you might go out of bounds if myCritters.size() > myAnimals.size() )? Also, your SetHealth() method isn't very descriptive of what it does, it doesn't actually set the health to a value, but subtracts that value from it, consider renaming it to takeDamage() or something like that.

bool Animal::Check4Death()
{
if(*m_pHealth <= 0)
return true;
else
return false;
};

You can reduce this to:

bool Animal::Check4Death()
{
return *m_pHealth <= 0;
};

Looks a lot cleaner.

Now as to your problem, your m_pHealth member is a pointer, I think that could be it. Why is this a pointer? Are you sure your animals aren't sharing this pointer?

* used quotes for code because source tags don't seem to function very well

Edited by Mussi, 26 July 2012 - 06:52 AM.

[Ubermeowmix]

bool Animal::Check4Death()
{
return *m_pHealth <= 0;
};

That is cleaner code, I didn't realize that was a legal declaration.

[Ubermeowmix]

void AttackAnimals(int dam)
{
for(int i = 0; i < myCritters.size(); i++)
{
myAnimals[i].SetHealth(dam);
};

that was just a typo on my part.

You've lost me with that code FredOrJoeBloggs, it doesn't seem to work either. Not really sure about templates yet, but i'll put it aside to tinker with later.

[Ubermeowmix]

Oh I've figured out why the following was evaluating to all die.

for ( vector<Animal>::iterator iter = myAnimals.begin(); iter != myAnimals.end(); /* empty */ )
{
if ( iter->Check4Death() )
{
cout << iter->GetName() << " has died!\n";
iter = myAnimals.erase( iter );
} else {
iter ++;
}
}
};

it's because the constructor call was creating pointers to the member functions and killing the code, now i've swapped it all back to being normal variables the code it working fully. Thanks for all the help guys, the answers were in there. They were just dotted around a bit.

[iMalc]

You don't need to NULL them out.

Indeed, and to get this point across, NULLing out the pointers in the destructor after freeing the memory they pointed to is like using white-out over your text on a piece of paper just before you put it through a shredder.
If you're about to shred it, then there's no point in using white-out.

[larspensjo]

If you're about to shred it, then there's no point in using white-out.

I use this technique now and then. In principle, you are perfectly correct. In practice, this technique helps you find cases where you are using dangling pointers. I tend to make those mistakes myself less and less with the years (though it still happens), but for beginners I definitely recommend it. Yes, it costs some performance, but the usual rule applies: don't optimize until you have measurements.

[dmatter]

In principle, you are perfectly correct. In practice, this technique helps you find cases where you are using dangling pointers.

Not sure I buy that argument really, how would you manage to use the pointers after the destructor? It's only really relevant if you call delete on a pointer where subsequent code still has access to that pointer - such a scenario, whilst easy to concoct, should be almost always avoided. I feel it's better to abide by that rule than to bother nulling pointers after delete, it'll save you from more more bugs overall.

[larspensjo]

Not sure I buy that argument really, how would you manage to use the pointers after the destructor? It's only really relevant if you call delete on a pointer where subsequent code still has access to that pointer - such a scenario, whilst easy to concoct, should be almost always avoided.

I agree, of course. Maybe I wasn't clear in my description, I don't advocate it as a "design pattern". What I meant is that it helps you find bugs, while not being something you should rely on.

[Brother Bob]

Not sure I buy that argument really, how would you manage to use the pointers after the destructor? It's only really relevant if you call delete on a pointer where subsequent code still has access to that pointer - such a scenario, whilst easy to concoct, should be almost always avoided.

I agree, of course. Maybe I wasn't clear in my description, I don't advocate it as a "design pattern". What I meant is that it helps you find bugs, while not being something you should rely on.

It also hides bugs.

Deleting a null pointer is legal and guaranteed by the language to be a no-op. So you if you're double-deleting a pointer in some code path, you have hidden that bug and you won't discover it. At least pick a non-null pointer if you want to clear a pointer to some easy-to-spot value so that it at least is likely to crash or do something to get your attention.