problems deleting elements in vector while iterating through?..

[pabloist]

I'm working on the bunny problem from http://www.cplusplus.com/forum/articles/12974/.

My program crashes sometimes when it's deleting bunnies from the vector; it also doesn't delete them consistently (I should see 10 bunnies die by turn 10 given that none of them are mutants, but I don't - I usually see only 1 - 3 or so get deleted.)

Game.cpp

#include <iostream>
#include <vector>
#include <algorithm>
#include <fstream>

#include "Bunny.h"
#include "Game.h"

#include "../../custom_files/rangedRandom.h"
#include "../../custom_files/itos.h"

//------------------- STATIC VARS
int Game::bunniesBorn   = 0;
int Game::bunniesDied   = 0;
int Game::numMassKills  = 0;
int Game::numStarters   = 10;
int Game::populationLim = 1000;
//-------------------------------

void Game::Play()   // this calls a single round
{
    turn++;
    Update();
    cout << "Turn: " << turn << ", "
         << "Bunnies Remaning: " << bunnyList.size() << endl;
}

bool Game::GameOver()
{
    if (bunnyList.empty())
        return true;
    else
        return false;
}

void Game::SaveData()
{
    ofstream save ("datalog.txt", ios::app);
    if (save.is_open())
    {
        save << "--------------------------------------------" << endl;
        save << "TURN NUMBER " << turn << endl;
        save << "List of bunnies:" << endl;
        for (vector<Bunny>::iterator iter = bunnyList.begin(); iter != bunnyList.end(); iter++)
            save << iter->title << endl;
        save << "\nTotal Bunnies:  " << bunnyList.size() << endl;
        save << "Total Died:     " << bunniesDied       << endl;
        save << "Total Born:     " << bunniesBorn       << endl;
        save << "--------------------------------------------" << endl;

        save.close();
    }
    else
        cout << "Failed to save data." << endl;
}

void Game::Update()
{
    vector<Bunny>::iterator iter = bunnyList.begin();

    Bunny sampleRMVB(NULL);   // this isn't an actual bunny being created, it's just a placeholder.
    int numRMVB = 0;
    bool dadExists = false;


    // first, see if there is a father.
    while (!dadExists && iter != bunnyList.end()) // as long as we haven't found a father AND we havent reached the end of the list.
    {
        if (iter->age >= 2 && iter->gender == "male")
            dadExists = true;
        iter++;
    }

    // now, check for mutants.
    for (iter = bunnyList.begin(); iter != bunnyList.end();
        iter++)
    {
        if (iter->isRMVB)
        {
            sampleRMVB = *iter;
            numRMVB++;
        }
    }

    // now, age all bunnies, kill ones that are too old, mutate if necessary, and give birth to new ones.
    for (iter = bunnyList.begin(); iter != bunnyList.end();
        iter++)
    {
        iter->age++;    // age all bunnies.

        iter->title = "Bunny " + iter->name + " (" + itos(iter->age) + ", " + iter->gender + ", " + iter->color + ")";  // update the title of each bunny.


        if (iter->age >= 2 && iter->gender == "female" && !iter->isRMVB)    // if the bunny is a girl over 2 and is NOT radioactive
        {
            if (dadExists)  // if there is a father present
            {
                bunniesBorn++;
                bunnyList.push_back(iter->BirthBunny(*iter)); // add a new bunny to the list!
            }
        }
        if (numRMVB > 0)
        {
            if (!iter->isRMVB)  // if the current is not an RMVB.
            {
                sampleRMVB.ConverttoRMVB(*iter);
                numRMVB--;
            }
        }
        if (iter->tooOld())
        {
            bunniesDied++;
            cout << iter->name << " died!" << endl;
            system("PAUSE");

            if (!bunnyList.empty())
                iter = bunnyList.erase(iter);   // kill the bunny
        }
        if (static_cast<int>(bunnyList.size()) >= randomInRange(populationLim, bunnyList.size()))    // if there are too many bunnies in the population....
        {
            int amount;

            if (bunnyList.size()-2 > populationLim)
                amount = randomInRange(populationLim, bunnyList.size()-2);    // kill anywhere from populationLim to n-2 rabbits
            else
                amount = populationLim;

            bunniesDied += amount;

            random_shuffle(bunnyList.begin(), bunnyList.end());
            bunnyList.erase(bunnyList.begin(), bunnyList.begin()+amount);

            numMassKills++;
            cout << "MASS BUNNY DEATH! " << amount
                 << " BUNNIES KILLED." << endl;
        }
   }
}

Bunny.cpp

#include <iostream>
#include <vector>

#include "../../custom_files/rangedRandom.h"
#include "Bunny.h"

//-------------STATIC CONST
const int Bunny::deathage_normal = 10;
const int Bunny::deathage_mutant = 25;
//-------------------------


bool Bunny::operator< (const Bunny& compare) const // basis is age.
{
    if (age < compare.age)
        return true;
    else
        return false;
}

void Bunny::Age()
{
    age++;
}

bool Bunny::tooOld()
{
    if (age >= deathage_normal && !isRMVB)
        return true;
    else if (age >= deathage_mutant && isRMVB)
        return true;
    else
        return false;
}

Bunny Bunny::BirthBunny(const Bunny& female)
{
    Bunny newBaby;
    newBaby.color = female.color;
    return newBaby;
}

void Bunny::ConverttoRMVB(Bunny& target)
{
    if (this->isRMVB)
        target.isRMVB = true;
}

main.cpp

#include <iostream>
#include <cstdlib>
#include <ctime>

#include "../../custom_files/rangedRandom.h"
#include "Game.h"
#include "Bunny.h"

using namespace std;

int main()
{
    srand(time(0));
    Game game;

    for (int i=0; i<50; i++)
    {
        game.Play();
        if (game.GameOver())
            break;

        i++;
        game.SaveData();
        cout << "Saving data to a file." << endl;
        system("PAUSE"); // use this to look at console output each turn.
    }

    cout << "You lasted for " << game.turn << " turns!" << endl;
    return 0;
}

[ne555]

Can you just put the relevant part?
I see this in Game::Update(), maybe is something else too:

for (iter = bunnyList.begin(); iter != bunnyList.end(); iter++){
//...
        if (iter->tooOld())
        {
                //...
                iter = bunnyList.erase(iter);   // kill the bunny
        }
}

So you are always incrementing the iterator. But if you kill a bunny, when you refresh it, the iterator already points to the next element.
You're missing when there are two consecutive that must be killed

[Galik]

Game.cpp line 99 will invalidate your iterator.

Also I think you need to age the bunnies *after* doing all the other tests.

Also +1 ne555. You should not be doing iter++ on those occasions when you kill a bunny. Only when the bunny survives.

[pabloist]

Sorry about posting all the code, I wasn't completely sure if that was the only problem spot.

Thanks for pointing that out! How should I handle this, though? I tried adding iter--; to it to move back an element (which should be okay, because that part of Game::Update is at the end of the loop, right?), but it's still crashing.

[pabloist]

Thanks galik, didn't notice that either. Again, how can I deal with this?

[Galik]

I think you could remove the increment from the for loop:

    for (iter = bunnyList.begin(); iter != bunnyList.end(); /* leave empty */)

And then conditionally increment on your age test:

        if (iter->tooOld())
        {
            bunniesDied++;
            cout << iter->name << " died!" << endl;
            system("PAUSE");

            if (!bunnyList.empty())
                iter = bunnyList.erase(iter);   // kill the bunny
        }
        else
        {
                ++iter;
        }

[Galik]

For adding bunnies I would be tempted to add the new bunnies to a separate vector and append that to the main vector after the loop ends.

[pabloist]

! Holy crap, thanks, it works now!

I removed the increment from the for() loop, and moved to only increment if the bunnies are able to age. Thanks a lot :D