Filter function implementations using copy_if algorithm vs. for loop

Question

I have the following method of some class, which also defines the method isAllowed:

auto filter(const auto& in) { auto ret = decltype(in) {}; for(auto x : in) if(isAllowed(x)) ret.insert(x); return ret; } 

This is a clear case where copy_if could be used instead. I see two alternative versions:

auto filter(const auto& in) { auto ret = decltype(in) {}; copy_if(begin(in), end(in), inserter(ret, end(ret)), [this](auto i) {return this->isAllowed(i);}); return ret; } 

or

auto filter(const auto& in) { auto ret = decltype(in) {}; copy_if(begin(in), end(in), inserter(ret, end(ret)), bind(&A::isAllowed, this, placeholders::_1)); return ret; } 

Both seem much less obvious than the original, so I am inclined to keep the for loop. Is there a strong argument not to? (And is there a better way?)

Otherwise, I feel itchy because cases like this point to the limited usefulness of the algorithm tools, despite what best practices advise.

There are probably good arguments for choosing copy_if that are not being considered properly (performance?). This question is mainly after those, as the others seem more intuitive and easier to get.

Answer 1

I'll respectfully disagree with other answers that suggest using anything but the first option. The only thing the other two std-library-heavy approaches accomplish is to obfuscate the code. Never underestimate the importance of keeping it simple. Complexity is the number one enemy of any piece of software.

The first one is not only more explicit about what is happening, but it is also universal. Any high-level programming language has the concept of a for-loop, so an occasion reader coming from another language background can still clearly see what's going on over there. A JavaScript programmer would have a very hard time figuring out what an inserter or a placeholder is. Even a novice C++ programmer with limited knowledge of the library would have to waste time digging through documentation to understand a simple function that does a conditional copy.

Don't get me wrong here, I'm not making a case against the Standard Library, I'm just pointing out that, like any programming language feature, it can be overused. In the three examples presented by the OP, the second and third clearly are overusing the std lib just for the sake of using library feature XYZ. My opinion.

Answer 2

Your instinct to use a plain old for loop is correct. However, there are a few nits to pick with your code:

auto filter(const auto& in) 

This is not valid C++, last time I checked (2014).

 auto ret = decltype(in) {}; 

This would be better written as decltype(in) ret; to eliminate the move-construction.

 for(auto x : in) 

This should certainly be for (auto&& x : in), to eliminate the copy-construction of x. Also, stylistically, it would be good to wrap each control structure in curly braces.

Putting it all together, we get

template<typename T> auto filter(const T& in) { decltype(in) filtered; for (auto&& x : in) { if (isAllowed(x)) { filtered.insert(x); } } return filtered; } 

Rewriting to use perfect forwarding is left as an exercise for the reader. :)

Answer 3

I would either pick the first or the second, but not the third. That said, you could also provide another overload that takes two iterators instead of a range. It would make the function more powerful since it would allow it to filter parts of collections instead of full collections only:

template<typename InputIterator> auto filter(InputIterator begin, InputIterator end); 

You could also make it take an OutputIterator parameter so that the result can be stored anywhere. But then your function is no more than a thin wrapper around std::copy_if. That said, you would benefit from its full power.

You could also replace std::inserter(ret, end(ret)) by the simpler std::back_inserter(ret) since you only ever add the elements at the end of the collection to be returned.

Answer 4

Personally I'd pick the second over the other two but if you're looking ahead to C++17 then this is one of the situations where the Ranges proposal will really help.