C++ std::map is this the correct practise

Question

Apologies in advanced if this is the wrong site, please let me know if it is!

I've written a function that checks to see whether a key exists in a particular std::map and wondered if this is a good practise to use, and, whether or not anyone can throw any pointers on improvements.

The std::map allows for multiple data-types to be accepted for the value.

union Variants {

    int asInt;
    char* asStr;


    Variants(int in) { asInt = in; }
    Variants() { asInt = 0;}
    Variants(char* in) { asStr = in; }

    operator int() { return asInt; }
    operator char*() { return asStr; }

};

template<typename T, typename Y>
bool in_map(T value, std::map<T, Y> &map)
{
     if(map.find(value) == map.end()) {
       return false;
   }else{
     return true;
   }
}

And I can then use in main the following:

 std::map<string, Variants> attributes;

 attributes["value1"] = 101;
 attributes["value2"] = "Hello, world";

 if(in_map<std::string, Variants>("value1", attributes))
 {
    std::cout << "Yes, exists!";
 }

Any help or advise would be greatly appreciated. Sorry if this doesn't comply to the rules or standards. Thanks!

Answer 1

Typing

if(in_map<std::string, Variants>("value1", attributes))

seems a bit excessive to me, typing all of that typename syntax makes me want to just use the map.find function instead just out of convenience. However, depending on your compiler, sometimes the template parameters can be interpreted automatically, for example, visual studio will allow this:

if(in_map(std::string("value1"), attributes))

In this case, I had to construct an std::string object to replace the char*, but I've completely removed the template definition from the call, the compiler still figures out what T and Y are based on the parameters given.

However, my recommended advice would be to use #define to define your "function". While it is not really a function, since #define actually just replaces snippets of code directly into the source, it can make things much easier and visually appealing:

#define in_map(value,map) (map.find(value) != map.end())

Then your code to use it would just look like this:

if(in_map("value1", attributes))

You both get the optimization of not using a function call, and the visual appearance like it does in PHP.

Answer 2

ny pointers on improvements.

sure.

template<typename T, typename Y>
bool in_map(T value, const std::map<T, Y> &map)
{
     return map.find(value) != map.end();
}

And I'd place map as 1st parameter (just a preference). Also, because the whole thing fits into single line, you might not even need this function.

You're also throwing away returned iterator, but since you aren't using it, that's not a problem.

Apart from this, does this look ok in terms of coding practise? I.e. Using Union or are there other types I can use such as struct?

Well, using char* doesn't looke like a good idea, because char* implies that you can modify data. char* also implies that this pointer is dynamically allocated and you might want to delete[] that pointer later. And you can't use destructors in unions. If the text cannot be changed, you could use const char*, otherwise you might want to use different datatype. Also see Rule of Three

Next problem - you're trying to place char* and int at the same location. That implies that at some point you're trying to convert pointer to integer. Which is a bad idea, because on 64bit platform pointer might not fit into int, and you'll get only half of it.

Also, if you're trying to store multiple different values in the same variable, you are not indicating which type is being stored anywhere. To do that you would need to enclose union into struct and add field (into struct) that indicates type of stored object. In this case, however, you'll end up reinventing the wheel. So if you're trying to store "universal" type, you might want to look at Boost.Any, Boost.Variant or QVariant. All of those require BIG external libraries, though (either boost or Qt).

Answer 3

The biggest problem I see with your function is that you're throwing away the resulting iterator.

When you're checking if a key exists in a map, most of the time you want to retrieve/use the associated value after that. Using your function in that case forces you to do a double lookup, at the cost of performance. I would just avoid the use of the function altogether, and write the tests directly, keeping the iterator around for later use in order to avoid useless lookups:

auto it = map_object.find("key");
if (it != map_object.end())
    use(it->second);
else
    std::cout << "not found" << std::endl;

Of course if you're just checking whether a key exists and don't care for the associated value then your function is fine (taking into account what others told you in the comments) but I think its use cases are quite limited and not really worth the extra function. You could just do:

if (map_object.find("key") != map_object.end())
    std::cout << "found, but I don't care about the value" << std::endl;