[SOLVED] Removing an element from a map of vectors

Removing an element from a map of vectors

std::map<unsigned long, std::vector<Foo*> > fKeys = myObject->GetFoo();
for (auto it = fKeys.begin(); it != fKeys.end() && !found; ++it)
    for (auto it1 = 0; it1 < (*it).second.size() && !found; ++it)
    {
        if ( /* some condition */)
        {
            found = true;
            delete (*it).second[it1];
            (*it).second[it1] = nullptr;
        }
        else
            ++it1;
    }

After executing this code, I checked and the vector still has the element. I'd expect this element would be gone.

What am I missing?

Solution

There are many problems with your code:

You are operating on a copy of the map that is returned by GetFoo(). Any changes you make to that copy or its elements will not be reflected back in the original map that is inside of myObject. But, you are destroying a Foo object that the original map points at, which means you are leaving behind a dangling pointer that is just asking for trouble later.
You are not removing the found element from the vector, you are merely setting its value to nullptr. To actually remove the element, you need to call std::vector::erase().
Your inner loop is incrementing the wrong iterator. ++it should be ++it1 instead, and get rid of ++it1; in your else, as it doesn't belong there at all.
You can and should use iter->xxx instead of (*iter).xxx.

Try something more like this:

// assuming GetFoo() actually returns a map& reference!
auto &fKeys = myObject->GetFoo();

for( auto it = fKeys.begin(); it != fKeys.end() && !found; ++it ) {
    for( size_t idx = 0; idx < it->second.size() && !found; ++idx )
    {
        if( /* some condition */ )
        {
            found = true;
            delete it->second[idx];
            it->second.erase(it->second.begin() + idx);
        }
    }
}

Which can be simplified a bit by using a range-for loop for the outer loop, and replacing the inner loop with std::find_if(), eg:

// assuming GetFoo() actually returns a map& reference!
for( auto &elem : myObject->GetFoo() ) {
    auto it = std::find_if(elem.second.begin(), elem.second.end(),
                           [](Foo *f) { return /* some condition */; });
    if( it != elem.second.end() )
    {
        delete *it;
        elem.second.erase(it);
        break;
    }
}

Which can be simplified even further by changing your std::vectors to hold std::unique_ptr<Foo> instead of Foo* so you don't have to manually delete anything (you shouldn't be using new/delete if you can help it).

Either way, the kind of logic you are trying to use really should be moved inside of a method of your myObject itself, so it can operate on its internal map instead of the caller operating on a copy of the map. Then you can get rid of the GetFoo() method altogether, eg:

template<typename Callable>
void MyObjectClass::removeFooObject_if(Callable predicate) {
    for( auto &elem : fKeys ) { // <-- or whatever your member is called...
        auto it = std::find_if(elem.second.begin(), elem.second.end(), predicate);
        if( it != elem.second.end() )
        {
            delete *it;
            elem.second.erase(it);
            return;
        }
    }
}

...

myObject->removeFooObject_if(
    [](Foo* foo){ return /* some condition */; });