Using map.erase(it); is more efficient because it doesn't have to do a lookup with the key to find the node that should be deleted.
Your code has a problem because the iterator that you pass to erase will be invalidated so you can't keep using it as if nothing has happened. The erase function returns an iterator to the element after the removed element so you could use this to update your iterator it = map.erase(it); but then you need to make sure not to increment the iterator it++ after you removed an element because otherwise you will miss to check the element after the removed element.
1 2 3 4 5 6 7 8 9 10 11
for(std::map<int, std::string>::iterator it = map.begin(); it != map.end();)
{
if((it->second) == itemToRemove)
{
it = map.erase(it);
}
else
{
it++;
}
}
Note that if you know that the values are unique (so only one element can be removed) you could instead just break out of the loop after you have removed the element.
1 2 3 4 5 6 7 8
for(std::map<int, std::string>::iterator it = map.begin(); it != map.end(); it++)
{
if((it->second) == itemToRemove)
{
map.erase(it);
break;
}
}