C++ unordered_map with custom key resulting in null values

Question

I'm using a std::unordered_map with keys of Currency and values with a double of the currency price. Currency is a custom class that I made. Here is one version that I have tried:

#ifndef CURRENCY_H
#define CURRENCY_H

#include "Nameable.h"
#include <boost/uuid/uuid.hpp>
#include <boost/uuid/uuid_generators.hpp>
#include <boost/uuid/uuid_io.hpp>
#include <boost/functional/hash.hpp>
#include "BigDecimal.h"
#include <iostream>

/**
 * Represents a single currency. Can be used as keys in a map and as a general
 * identifier for determining what unit a value of money is.
 * @param name
 */
class Currency: public Nameable {
public:
    Currency(std::string name) throw(NameAlreadyTakenException);
    Currency(const Currency& orig);
    virtual ~Currency();
    virtual std::string getName();
    virtual void setName(std::string name) throw(NameAlreadyTakenException);
    inline bool operator==(const Currency& key) const {
        return this->id == key.id;
    }

    // A custom hasher that I tried using.
    struct currencyHasher
        {
        std::size_t operator()(const Currency& k) const
        {
            return boost::hash<boost::uuids::uuid>()(k.id);
        }
    };
    boost::uuids::uuid id;
private:

};
// A template specialization for Currency. 
namespace std {
    template <>
    struct hash<Currency> {
        std::size_t operator()(const Currency& k) const {
            cout<< boost::hash<boost::uuids::uuid>()(k.id)<<"\n";
            return boost::hash<boost::uuids::uuid>()(k.id);
        }
    };
}
#endif  /* CURRENCY_H */

And here is the implementation:

#include "Currency.h"

Currency::Currency(std::string name) throw(NameAlreadyTakenException) {
    this->setName(name);
    this->id = boost::uuids::random_generator()();
}

Currency::Currency(const Currency& orig) {

}

Currency::~Currency() {
}

std::string Currency::getName() {
    return this->name;
}

void Currency::setName(std::string name) throw(NameAlreadyTakenException) {
    this->name = name;
}

I tried making Currency key-compatible by implementing both suggestions given by the answer to: C++ unordered_map using a custom class type as the key. As you can see I've overridden the operator== as well as providing a custom hasher as well as specialize the template.

Despite all of this, the keys seem to be losing the values. By this I mean doubles, floats and ints get turned into 0 and string get turned into empty strings. Of course, it causes other problems with anything else that I use as a value. For example:

Currency dollar("Dollar")
std::unordered_map<Currency,int,Currency::currencyHasher> currenMap;
currenMap[dollar]=1337;
std::cout<<currenMap[dollar]<<"\n";

The output of this in the console is 0. Utilizing the template specialization doesn't work either:

std::unordered_map<Currency,int> currenMap;
currenMap[dollar]=1337;
std::cout<<currenMap[dollar]<<"\n";

produces a 0 as well...

Could the fact that Currency is a subclass of Nameable be causing problems? I'm using the boost::uuid as the hash (utilizing boost::hash<boost::uuids::uuid> to convert the id into a size_t) I'm not sure what I'm missing, I thank you for your help.

Cue: http://stackoverflow.com/questions/10334688/how-dangerous-is-it-to-compare-floating-point-values/10335601#10335601 — sehe, Oct 28 '14 at 22:00
@sehe I understand that equality should not be used for floats and doubles. But as far as I know, I'm not using equality for floats and doubles. Unless boost::uuids are represented by them. — Kammeot, Oct 28 '14 at 22:04
agreed. I just responded with the omen, after seeing "Currency" :) You already have the answer. Here self-contained: http://coliru.stacked-crooked.com/a/50e9f64070a7bafc — sehe, Oct 28 '14 at 22:14
Quite distinct from your question, but FWIW assigning uuids to each currency at runtime is ostensibly unnecessarily expensive and cumbersome practice - are you using the uuid across processes/systems? If not, a `std::vector` (or `Current*` or smart pointer if you're really going to have derived classes) and `std::unordered_map` for mapping currency names to vector indices (or you could go straight to `Currency*`s) would perform better and use less memory use. — Tony Delroy, Oct 29 '14 at 15:39
@TonyD The currency names can change, so I needed something that could be hashable so that when the name changes, the currency will still hash to the right value. My plan is to store all the currencies in an unordered_map with their associated value. I don't understand how having an unordered_map of indices for a vector of currencies is less cumbersome, but I can see the memory advantage, seeming UUIDs take up 16 bytes. Would using an int (only 4 bytes) for the id be a better option? — Kammeot, Oct 29 '14 at 19:07
`currency names can change` - then to use the currency name as a look-up key (which I'm guessing you'll need to do sometimes) you'll need an associative container keyed on names wherein you'll erase old-name & insert new-name... that associative container might as well be linking efficiently to the `Currency` objects. The simplest and most efficient ways are by storing an array index or pointer, rather than storing a uuid that then requires an additional `unordered_map` look-up. I just don't see the uuids add any value here (they're normally only used for cross-process uniqueness). — Tony Delroy, Oct 30 '14 at 02:07
@TonyD Sounds reasonable. I realized that uuids were overkill once I saw that they represented 2^128 unique ids! I still want to use the Currency object as a key to the currency value, so instead of a uuid being used for a hash, I'm using an `unsigned long int`, which should reduce the memory footprint by 75%. All entities are going to be holding a pointer or a reference to the currency object that belongs to them. Whenever they carry out a transaction, it's through the money exchange, which calculates the new value of the currency. As part of that, it uses the map as described. — Kammeot, Oct 30 '14 at 04:40
Fair enough - good luck! You'll probably want to avoid overriding `setName` if that's circumventing the `Nameable` checks that potentially throw `NameAlreadyTakenException` - otherwise - seems your code's creating new `Currency` objects with the same name and a newly generated `uuid`. (Separately, try to get in the habit of passing strings/containers by `const` reference unless there's a reason not to - considerably faster). — Tony Delroy, Oct 30 '14 at 05:02

score 2 · Accepted Answer · answered Oct 28 '14 at 22:02

2

The issue is with the copy constructor:

Currency::Currency(const Currency& orig) {

}

When you copy a Currency, you get a default-constructed id. When you insert a Currency into the map, it gets copied, and that copy will have a different id than the original. Thus this:

currenMap[dollar]=1337;

is effectively adding {Currency(), 1337} into the map. So when you are looking up the one with whatever id gets created for dollar, it won't be there. It's not that the value gets "zeroed out"... it's that you get a default-constructed value back.

Fixing your copy constructor should fix the problem.

answered Oct 28 '14 at 22:02

Barry

286,269
29
621
977

`Currency(const Currency &orig) = default; virtual ~Currency() = default;` wouldl seem okay. RuleOfZero – sehe Oct 28 '14 at 22:11
Thank you I fixed my problem by setting the copy constructor to default. `Currency(const Currency& orig) = default;` – Kammeot Oct 28 '14 at 22:23

C++ unordered_map with custom key resulting in null values

1 Answers1