9
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ I'd take the first one any day of the week. It is simple and straightforward, so much that even people that don't know C++ or the STD lib can still understand. What is the point of a "best practice" if it only makes code more complicated anyway? \$\endgroup\$ – glampert Mar 14 '15 at 2:41
  • \$\begingroup\$ @glampert, your comment if the best answer I have seen so far. Make it an answer (perhaps detailing that this is valid in this particular case, not as a general rule) and I will select it \$\endgroup\$ – ricab Sep 21 '15 at 17:28
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks, I tend to agree with you. I select this answer until I see good arguments for the other cases that I may still be unaware of. \$\endgroup\$ – ricab Sep 22 '15 at 8:25
2
\$\begingroup\$

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. :)

\$\endgroup\$
  • 1
    \$\begingroup\$ Concerning "not valid C++": true, but likely coming in C++17, part of concepts TS (I just added a tag to the post). Concerning declaration and initialization of ret: there is no move construction, the default ctor is called (and no assignment, only initialization). Concerning the declaration of x in the for: OK, but that would also have to be done on the other possibilities to be fair, and I thought I should keep it as simple as possible for the purpose of the question. Concerning the quotes, that is debatable. \$\endgroup\$ – ricab Aug 19 '15 at 15:49
  • \$\begingroup\$ I meant "braces" instead of "quotes". Cheers \$\endgroup\$ – ricab Aug 19 '15 at 16:50
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ That would make a lot of sense for generalizing, although it would be copy_if as you say. But generalizing is not always the goal, otherwise we'd stick with a Turing Machine and write ones and zeros for each different application. In this case I want to take advantage of specific use-case knowledge so that I can write bar = filter(foo) instead of calling copy_if (or writing a for loop) each time. There is no loss of generality exactly because copy_if (and loops) are still there. The gain is in less code duplication for the cases that are fully served by the form in the original post. \$\endgroup\$ – ricab Aug 19 '15 at 16:49
  • \$\begingroup\$ @ricab That's why I was talking of adding overloads. You don't have to remove the simple version, but you can still add more powerful ones. \$\endgroup\$ – Morwenn Aug 19 '15 at 17:02
  • \$\begingroup\$ If I don't expect I will need them, that would be over-engineering, especially when there is so little difference to copy_if (as you rightly pointed out) \$\endgroup\$ – ricab Aug 20 '15 at 9:44
  • 1
    \$\begingroup\$ @ricab Well, there's still the benefit of abstracting isAllowed away, but I get your point. \$\endgroup\$ – Morwenn Aug 20 '15 at 9:46
  • \$\begingroup\$ True, but why not generalizing that part either... I mean, your approach would be better if I wanted to provide something in a library for other people's unknown uses, it is just not the case here. Still, something as general as copy_if has the disadvantage of being cumbersome to use, as this question hints at. It may just leave you choosing the even more general (but also more standard and easier to read) for loop. There must be something I am not considering properly and I was hoping to see more defenses of copy_if \$\endgroup\$ – ricab Aug 20 '15 at 9:57
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.