2

I use a vector of shared pointers to contain some game characters called customer.

typedef std::shared_ptr<Customer> customer;
std::vector<customer> customers;

customers.push_back(customer(new Customer()));

for(int i = 0; i < customers.size(); i++)
{
    if(customers[i]->hasLeftScreen())
    {
        if(!customers[i]->itemRecieved())
            outOfStocks++;
        // Kill Character Here
    }       
}

I have used vectors to hold objects before so am used to calling erase on the vector and passing in the iterator. My question is there a way of deleting a the pointer from the vector in the above code snippet? I was hoping not to use an iterator here to simplify the code. I also need to delete the pointer because I was the customer to be removed from the game once it has left the screen.

Many thanks

ildjarn
  • 62,044
  • 9
  • 127
  • 211
Chris
  • 191
  • 1
  • 13

3 Answers3

3

Consider using an iterator, which frankly will be much easier to deal with. I'm not sure of your aversion to them, but see below:

std::vector<customer>::iterator it = customers.begin();
while (it != customers.end())
{
    if(it->hasLeftScreen())
    {
        if(!it->itemRecieved())
            outOfStocks++;
        it = customers.erase(it);
        continue;
    }
    ++it;
}

This will remove the shared pointer instance from the vector. If the instance is the last reference to the shared pointer it will also release the associated memory of said Customer, firing its destructor, etc... (somewhat the point of using smart shared pointers in the first place, and props for using smart pointers, by the way).

WhozCraig
  • 65,258
  • 11
  • 75
  • 141
  • Thank you for this. I don't really have an aversion to using an iterator. I was just because at the time, a for loop was quicker to type. I'm getting myself confused as this is the first time using shared pointers and I am still trying to get used to the way they work as opposed to just using a plain old vector storing objects. Thanks for the help. – Chris Mar 11 '13 at 17:04
  • @Chris No problem. Spend some time with iterators, and study the different types (a good expansion can [be found here](http://en.cppreference.com/w/cpp/iterator)). Being fluent with them is going to pay of huge as you work more and more with standard containers and algorithms. – WhozCraig Mar 11 '13 at 17:09
  • Brilliant, thanks for the link. Always looking to expand my C++ knowledge! – Chris Mar 11 '13 at 17:13
2

You should always use iterators; it's a C++ idiom. This would change the code to...

for(auto i = customers.begin(); i != customers.end(); ++i)
{
    if((*i)->hasLeftScreen())
    {
        if(!(*i)->itemRecieved())
            outOfStocks++;
        // Kill Character Here
    }       
}

Now, it is clear, we use the erase-remove idiom instead.

int outOfStocks = 0;
auto it = std::remove_if(customer.begin(), customers.end(), [&](Customer const& i) {
    if(i->hasLeftScreen()) {
        if(!i->itemRecieved()) {
            outOfStocks++;
        }
        return true;
    }
    return false;
}
std::erase(it, customers.end());
Alex Chamberlain
  • 4,147
  • 2
  • 22
  • 49
0

You can also take advantage of "iterator arithmetic":

        // Kill Character Here
        customers.erase(customers.begin() + i);

... but that has a problem that customers.size() and the current index will get invalidated as the container will shrink.

Also, you don't need to explicitly delete the customer you're removing, because the smart pointer will take care of that.

ulidtko
  • 14,740
  • 10
  • 56
  • 88