tags:

views:

204

answers:

2
+1  Q: 

Union with map?

I am trying to Union two sets with map. I have two sets and would like to combine them into a third one. I get an error for this code in the push_back. Is there a way to do this?

map<char, vector<char> > numbers;
map<char, vector<char> >::iterator it;
numbers['E'].push_back('a');//set1
numbers['E'].push_back('b');
numbers['E'].push_back('c');
numbers['G'].push_back('d');//set2
numbers['G'].push_back('e');


void Create::Union(char set1, char set2, char set3)
{
    for (it = numbers.begin(); it != numbers.end(); ++it)
    {
     numbers[set3].push_back(it->second);
    }
}
+3  A: 

I think you might want:

void Create::Union(char set1, char set2, char set3)
{
    vector<char> &target = numbers[set3];
    for (it = numbers.begin(); it != numbers.end(); ++it)
    {
        if (&it->second == &target)
            continue; // Don't insert into ourselves
        target.insert(target.end(), it->second.begin(), it->second.end());
    }
}

push_back was trying to add the item->second vector itself to the target vector; this way explicitly copies the contents only.

bdonlan
What happens when the iterator reaches numbers[set3], and you try to insert it into itself?
Steve Jessop
(Genuine question, btw, I don't actually know the answer off-hand. I have a suspicion that it's "bad things", because the vector could resize midway through). +1 for the general idea, on the assumption either I'm wrong or you'll sort it out.
Steve Jessop
@onebyone, good point. Modifying the vector invalidates the .begin() and .end() iterators, and so it's undefined behavior
bdonlan
This code gives me both sets twice when the third is printed
Jack Null
@onebyone, gah, yes. Right then, continue bug fixed. Anyway, this was intended to be a fixed version of the code in the question, and I'll leave it at that :)
bdonlan
Yes, I'm unsure myself whether the OP wants to take the union of two sets, or whether he wants to take the union of all the sets, but there just so happen to be two in the example given. My answer does the former, yours the latter, so Jack take your pick :-)
Steve Jessop
Btw, sorry, I deleted my comment about return/continue, which makes yours look weird. I was rewriting it, deleted the old one, but you got there first. Oops.
Steve Jessop
+10  A: 

numbers is a load of vectors, keyed by character. So it->second is a vector. You can't push_back a vector into a vector of char.

You should be iterating over numbers[set1] and numbers[set2], not iterating over numbers. Or as bdonlan says, you could insert a range, although he's taking a union of everything in numbers, not just set1 and set2.

Also: where's item defined? Do you mean it?

Also, note that push_back doesn't check whether the value is in the vector already. So once you get the details of this general approach sorted out, your example case will work and the union of 'E' and 'G' will be a vector containing 'a','b','c','d','e'. But if you took the union of 'a','b','c' with 'c','d','e' you'd get 'a','b','c','c','d','e', which probably isn't what you want from a union.

Assuming your vectors are always going to be sorted, you could instead use the standard algorithm set_union:

#include <algorithm>
#include <iterator>

...

numbers[set3].clear();
std::set_union(numbers[set1].begin(), numbers[set1].end(),
               numbers[set2].begin(), numbers[set2].end(),
               std::back_inserter(numbers[set3]));

If you want to take the union of everything in numbers, I would probably go with either:

vector<char> sofar;
map<char, vector<char> >::iterator it;
for (it = numbers.begin(); it != numbers.end(); ++it) {
    // new, empty vector
    vector<char> target;
    // merge everything so far with the next item from the map,
    // putting the results in target
    set_union(sofar.begin(), sofar.end(),
              it->second.begin(), it->second.end(),
              back_inserter(target));
    // the result is the new "everything so far"
    // note that this operation is very fast. It doesn't have to
    // copy any of the contents of the vector, just exchange some pointers.
    swap(target, sofar);
}
// replace numbers[set3] with the final result
swap(numbers[set3], sofar);

Or:

set<char> sofar;
map<char, vector<char> >::iterator it;
for (it = numbers.begin(); it != numbers.end(); ++it) {
    // let std::set remove the duplicates for us
    sofar.insert(it->second.begin(), it->second.end());
}
// replace numbers[set3] with the final result
numbers[set3].clear();
numbers[set3].insert(numbers[set3].end(), sofar.begin(), sofar.end());

This is less code and might be faster, or might thrash the memory allocator too much. Not sure which is better, and for small collections performance almost certainly doesn't matter at all.

The version with set also doesn't require the vectors to be sorted, although it's faster if they are.

Steve Jessop
+1 Much cleaner code using available STL functions. I've seen many programmers jump on writing their own implementation without investigating what's already available in STL.
Kei