tags:

views:

294

answers:

3

I have a map and I want to find the minimum value (right hand side) in the map. Right now here is how I did it

bool compare(std::pair<std::string ,int> i, pair<std::string, int> j) {
  return i.second < j.second;
}
////////////////////////////////////////////////////
std::map<std::string, int> mymap;

mymap["key1"] = 50;
mymap["key2"] = 20;
mymap["key3"] = 100;

std::pair<char, int> min = *min_element(mymap.begin(), mymap.end(), compare); 
std::cout << "min " << min.second<< " " << std::endl;

This works fine and I'm able to get the minimum value the problem is when I put this code inside my class it doesn't seem to work

int MyClass::getMin(std::map<std::string, int> mymap) {
  std::pair<std::string, int> min = *min_element(mymap.begin(), mymap.end(), 
                                                 (*this).compare);
                                                 //error probably due to this

  return min.second; 
}

bool MyClass::compare(
    std::pair<std::string, int> i, std::pair<std::string, int> j) { 
  return i.second < j.second; 
}

Also is there a better solution not involving to writing the additional compare function

+5  A: 

You have a few options. The "best" way to do this is with a functor, this is guaranteed to be the fastest to call:

typedef std::pair<std::string, int> MyPairType;
struct CompareSecond
{
    bool operator()(const MyPairType& left, const MyPairType& right) const
    {
        return left.second < right.second;
    }
};



int MyClass::getMin(std::map<std::string, int> mymap) 
{
  std::pair<std::string, int> min 
      = *min_element(mymap.begin(), mymap.end(), CompareSecond());
  return min.second; 
}

(You can also nest the CompareSecond class inside MyClass.

With the code you have now, you can easily modify it to work, however. Just make the function static and use the correct syntax:

static bool 
MyClass::compare(std::pair<std::string, int> i, std::pair<std::string, int> j) 
{ 
  return i.second < j.second; 
}

int MyClass::getMin(std::map<std::string, int> mymap) 
{
  std::pair<std::string, int> min = *min_element(mymap.begin(), mymap.end(), 
                                                 &MyClass::compare);
  return min.second; 
}
rlbond
I hit the format button for you. You need to specify the template arguments to the functor or pass an unused argument to a factory. Or template the `operator()` instead of the whole class… that's the best way, yeah :vP .
Potatoswatter
I agree, `operator()` should be templated in best practice. However, I think the code should illustrate the point. What do you mean by "specify the template arguments"? Do you mean derive from `binary_function`? I don't believe that is required for `min_element`...
rlbond
+2  A: 

The problem is that this:

bool MyClass::compare

Requires an instance of the class to be called on. That is, you can't just call MyClass::compare, but you need someInstance.compare. However, min_element needs the former.

The easy solution is to make it static:

static bool MyClass::compare

// ...

min_element(mymap.begin(), mymap.end(), &MyClass::compare);

This no longer requires an instance to be called on, and your code will be fine. You can make it more general with a functor, though:

struct compare2nd
{
    template <typename T>
    bool operator()(const T& pLhs, const T& pRhs)
    {
        return pLhs.second < pRhs.second;
    }
};

min_element(mymap.begin(), mymap.end(), compare2nd());

All this does is grab the second from each pair and grab them, works with any pair. It could be made for general, but that's a bit too much.

If you need to look up by value enough, I recommend you use Boost's Bimap. It's a bi-directional map, so both the key and value can be used to lookup. You would simply get the front of the value-key map.

Lastly, you can always just keep track of the minimum element going into your map. Every time you insert a new value, check if it's lower than your current value (and that should be probably be a pointer to a map pair, start it as null), and if it's lower, point to the new lowest. Requesting the lowest becomes as simple as dereferencing a pointer.

GMan
+2  A: 

I actually have another question: if you need to regularly obtain the minimum of the right-hand side values are you sure than a map is the best structure ?

I would suggest using Boost.MultiIndex in general for these problems of multiple ways of indexing the same set of objects... but if you only need this "reverse-mapping" bit Boost.Bimap might prove easier.

This way you won't have a linear search when looking for the minimum :)

Matthieu M.