Is std::map + std::tr1::bind + standard algorithms worthwhile?

Question

This is a follow-up to my question from yesterday. I have Scott Meyers' warning about write-only code on my mind. I like the idea in principle of using standard algorithms to access the keys or values of a std::map, but the syntax required is a little baroque IMHO. Let's say I want to dump all the keys of a map to a vector. Given following declarations,

typedef std::map<int, int> MyMap;
MyMap m;
std::vector<int> v;

which code is more maintainable (i.e., potentially less confusing)?

Option #1:

std::transform(m.begin(),
               m.end(),
               std::back_inserter(v),
               std::tr1::bind(&MyMap::value_type::first, _1));

Option #2:

for (MyMap::iterator i = m.begin(); i != m.end(); ++i)
{
    v.push_back(i->first);
}

Option 1 is more standard library-ish but I have to mentally decompose it to understand what's going on. Option 2 seems easier to read at the expense of a possible small runtime penalty. I'm not hurting for CPU time so I'm leaning toward option 2. Does you guys agree? Is there a third option I should consider?

P.S. Over the course of writing this question I concluded that the best way (for my project) to read the keys of a std::map is to store them off in a side container and iterate over that. The maintainability question still stands though.

score 11 · Accepted Answer · answered Dec 17 '08 at 22:18

11

Clarity always beats clever. Do what you can read later.

You're not alone in thinking that the standard code is a little obtuse. The next C++ standard will introduce lambda functions so you can write more legible code with the standard algorithms.

answered Dec 17 '08 at 22:18

Drew Dormann

59,987
13
123
180

score 5 · Answer 2 · answered Dec 17 '08 at 22:18

5

The first is just as readable and maintainable as the second -- if you know what bind does. I've been working with Boost::Bind (essentially identical to std::tr1::bind) long enough that I have no trouble with it.

Once TR1 becomes part of the official standard, you can safely assume that any competent C++ programmer will understand it. Until then, it could pose some difficulty, but I always think of the long-term over the short-term.

answered Dec 17 '08 at 22:18

Head Geek

38,128
22
77
87

I totally agree. A lot of people worry about the "readability" of some idiomatic piece of code, and think maybe it's not readable, but when you think what their intended audience is, it's people who don't know anything about the language you are writing – 1800 INFORMATION Dec 18 '08 at 00:29

score 4 · Answer 3 · answered Dec 17 '08 at 22:24

You forgot using namespace std::tr1::placeholders :P

To be honest, for simple algorithms like this, the latter code is probably easier to maintain. But I actually tend for the former (especially when C++1x gives us lambdas!) because it emphasizes a functional style of programming, which I personally prefer to the imperative style of using a loop.

It's really a different strokes thing; standard algorithms are most useful when they are either complex or generic, and this is neither.

Here's what it will look like with lambdas:

std::transform(m.begin(), m.end(), std::back_insterter(v),
               [](MyMap::value_type pair){ return pair.first; }
              );

And actually, there's another approach, which I would prefer but for its verbosity:

using std::tr1::bind;
using std::tr1::placeholders::_1;
std::for_each(m.begin(), m.end(),
              bind(&std::vector<int>::push_back, v,
                   bind(&MyMap::value_type::first, _1)
                  )
             );

And with lambdas (this is probably by and large the neatest and most explicit of all the options):

std::for_each(m.begin(), m.end(),
              [&v](MyMap::value_type pair){v.push_back(pair.first);}
             );

score 3 · Answer 4 · answered Dec 17 '08 at 21:58

3

I say go for 2)

To improve performance, you could get m.end() out of the loop and reserve the space in the vector.

Can't wait for C++0x and the range-based for loop; that would make your loop even better.

answered Dec 17 '08 at 21:58

Nemanja Trifunovic

24,346
3
50
88

score 1 · Answer 5 · answered Dec 18 '08 at 00:55

1

Go with option #1, see Scott Meyers, Effective STL Item #43, page 181.

answered Dec 18 '08 at 00:55

oz10

153,307
27
93
128

But the whole point of this question is that I'm trying to balance that notion against item #47 (avoid write-only code). – Michael Kristofik Dec 18 '08 at 03:56
@Kristo, if you really had an intimate, practiced knowledge of the STL + TR1 then option #1 would be just as readable to you as option #2. – oz10 Dec 19 '08 at 03:12

score 1 · Answer 6 · answered Dec 18 '08 at 15:39

When I looked at your question yesterday it wasn't the bind (which I use a lot) that forced me to look twice to understand the code but the map::value_type::first which I haven't had occasion to use very often. Whilst I agree that "Clarity always beats clever", familiarity is required before clarity and you're not going to become familiar with styles you don't use...

I'd also say that while option 2 is clearer in terms of comprehending the intended purpose, it would hide a bug more easily (any error in option 1 is more likely to be visible at compile time).

score 1 · Answer 7 · answered Mar 23 '12 at 23:28

I would go for Option #3:

#include <boost/range/adaptor/map.hpp>
#include <boost/range/algorithm_ext/push_back.hpp>

boost::push_back(v, m | boost::adaptors::map_keys);

This has the advantages that it:

is shorter
uses a named function to get the keys
is (potentially) more efficient (because boost::push_back can call reserve() on v)
and does not require the redundant v.begin(), v.end() pair.

Any other way is pure madness.

Is std::map + std::tr1::bind + standard algorithms worthwhile?

7 Answers7