views:

1390

answers:

6

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.

+3  A: 

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.

Nemanja Trifunovic
+7  A: 

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.

Shmoopty
+5  A: 

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.

Head Geek
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
+4  A: 

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);}
             );
coppro
+1  A: 

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

ceretullis
But the whole point of this question is that I'm trying to balance that notion against item #47 (avoid write-only code).
Kristo
@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.
ceretullis
+1  A: 

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

Patrick