Why is my vector not returning the last stored value?

Question

I'm trying to store two different values with the same key in an unordered_map. To do so I used a vector to hold the values. I want to recall a previously store value if the index is specified when I call with store->get() or else it returns just the latest value. When I run my test code, it always returns the first value store in the vector. When I debug I do see a second value stored in the vector at set(). But when the times come to get(), the vector only contain the first element. How can that be?

using namespace std;
int main() {
    class store
    {
        std::unordered_map<string, std::vector<string>> container;

    public:
        int set(string key, string value)
        {
            //find if the key exist
            if (container.find(key) != container.end())
            {
                //if yes insert at last position and return last index
                auto vec = container[key];
                vec.push_back(value);

                return vec.size() - 1;
            }
            //if not insert in vector and return index 0
            else
            {
                std::vector<string> tmpVec;
                tmpVec.push_back(value);
                container[key] = tmpVec;
                return 0;
            }
        };
        string get(string key, int timestamp = -1)
        {
            if (container.find(key) != container.end())
            {
                auto vec = container[key];

                if (timestamp >= 0)
                {
                    return vec[timestamp];
                }
                else
                {
                    return vec[vec.size() - 1];
                }
            }
            return "";
        };
    };

    store* store1 = new store();

    auto timestamp1 = store1->set("foo", "bar");
    store1->set("foo", "hello");

    cout << "value at timestamp1: " << store1->get("foo", timestamp1) << endl;;
    cout << "last value:" << store1->get("foo") << endl; //expected hello

    return 0;
}

Do you come from a language where you must use `new` to create objects (like Java or C#)? In C++ you don't have to do that. In fact, it's recommended *not* to use `new` if it can be avoided, which is can here: Just define `store1` as a non-pointer object. As in `store store1;` — Some programmer dude, Oct 26 '20 at 23:38
How can this be? For he IS the Kwisatz Haderach! But seriously, add in the missing headers. The more someone has to change to reproduce the error, the more likely they are to accidentally fix the problem or insert a new one. Other than that, looks like a good question. — user4581301, Oct 26 '20 at 23:39

score 3 · Accepted Answer · answered Oct 26 '20 at 23:40

The problem is in the set function where you do this:

auto vec = container[key];

Here you create a copy of the vector, and only add the value to the copy. The original vector remains unmodified.

You can use references to get a reference to the original vector instead:

auto& vec = container[key];
//  ^
// Note ampersand here, meaning vec is a reference

In fact the set function can be much simplified if you remember that the std::unordered_map overloaded operator for [] will create an element if it doesn't exist, which means you don't need to find the element first:

int set(string key, string value)
{
    // Use existing element from map if it exists,
    // or create a new if it doesn't
    container[key].push_back(value);

    // The vector is guaranteed to exist at this point,
    // and contain at least one element
    return container[key].size() - 1;
}

Thanks dude. Can't believe I got burned by the reference ... shame on me. I was coming from a time where declaring a new object both ways were ok. Is there a reason why we should avoid declaring object with new()? — David hehe, Oct 26 '20 at 23:50
@Davidhehe Some reading on that: [Why should C++ programmers minimize use of 'new'?](https://stackoverflow.com/questions/6500313/why-should-c-programmers-minimize-use-of-new) — user4581301, Oct 26 '20 at 23:53

steeps · Answer 2 · 2020-10-26T23:46:32.413

Your problem is in these two lines:

auto vec = container[key];
vec.push_back(value);

When you make the assignment to vec, you create a new copy of the vector in question. Change the above two lines to

container[key].push_back(value);

and you should get the intended result. Additionally, do not use using namespace std;! Either fully qualify (e.g. std::cout) or use only what you need (using std::cout;). You may also want to consider std::unordered_multimap for your use case.

Why is my vector not returning the last stored value?

2 Answers2