tags:

views:

117

answers:

3

I have a std::list of Bananas, and I want to get rid of the bad ones. Is there any relatively simple way to perform the following pseudocode?

foreach(Banana banana in bananaList)
{
    if(banana.isBad()) bananaList.remove(banana);
}

(Making a transition from C# and Java to C++ has been a rocky road.)

+7  A: 
bananaList.remove_if(std::mem_fun_ref(&Banana::isBad));

Note that you should probably be using std::vector instead of std::list though -- vector performs better in 99.9% of cases, and it's easier to work with.

EDIT: If you were using vectors, vectors don't have a remove_if member function, so you'd have to use the plain remove_if in namespace std:

bananaVector.erase(
    std::remove_if(bananaVector.begin(), bananaVector.end(), std::mem_fun_ref(&Banana::isBad)), 
    bananaVector.end());
Billy ONeal
Really? I thought that if I was removing lots of things from the middle of the list then `std::list` was the way to go.
John Berryman
@John: `std::list` makes the removal itself fast (O(1)), but finding the right spot relatively slow (O(N), usually a higher constant than vector).
Jerry Coffin
@John: Due to better cache locality, in practice `std::vector` often performs better where, in theory, `std::list` does. You'll have to measure.
sbi
@Martin York: For `std::remove_if`, you would be correct. However, for `std::list<t>::remove_if` the member function takes care of that for you.
Billy ONeal
I can't quote the article, but I read something in Dr. Dobbs or the C++UJ where they did some massive tests on list, vector, set, etc..., and they found that almost every time vector was faster. Even when the vector was being resized or removes and inserts were happening in the middle, it was still much faster, thanks to the rules of cache locality.
mos
You really have to be careful when saying that X always performs better than Y (not saying anybody did). You need to understand to costs of the operations. For example, if the copy constructor needed to move elements down in a vector is expensive, then a list could perform better. In my own code, I've seen nice performance boosts by switching to vector. Transient sets/maps can be very expensive to create and delete.
Torlack
@Torlack: That's why I said "99.9%" of all cases. Because in 99% of cases it's true. Even in cases where there is an expensive constructor involved (i.e. `std::vector<std::wstring>`) vector usually performs better than list.
Billy ONeal
@Billy ONeal: That is why I said "not saying anybody did". And I don't consider std::wstring constructor that expensive. I was referring to much more complex objects.
Torlack
+1  A: 

You'd typically do something like:

list.erase(std::remove_if(list.begin(), list.end(), std::mem_fun(Banana::isBad)), list.end());

Edit: Thanks to remove_if being implemented as a member function of std::list, Billy ONeal's answer is probably the better way to do the job as described, though this would be easier to convert when/if you decide to use a vector, deque, etc., which, as already discussed in comments, is probably a good thing to do.

Jerry Coffin
+1 for pointing at my answer :P
Billy ONeal
A: 

You can use a homebrew code like

for(list<...>::iterator it=bananas.begin(); end=bananas.end(); it!=end;) {
  if(... decide ...) {
    it=bananas.erase(it);
  } else
    ++it;
}

or, you can use the list::remove_if method, or std::remove_if function (which is usable with a vector, too).

jpalecek
One should always prefer algorithms to explicit loops.
Billy ONeal