return element in a map

Hello

Whats the right way to return reference to element in a map (in one line):

Code:

---

class someclass {
public:
  element &get_element(std::string key) { return *(m_map.find(key)); }

private:
  std::map<std::string, element> m_map;
};

---

Thanks for help!

That would be:

Code:

---

class someclass {
public:
  element& get_element(const std::string& key) { return m_map[key]; }

private:
  std::map<std::string, element> m_map;
};

---

Of course, if key does not exist then it would be added to the map. If you do not want this behaviour, then another option is:

Code:

---

class someclass {
public:
  element& get_element(const std::string& key)
  {
          std::map<std::string, element>:iterator iter = m_map.find(key);
          if (iter == m_map.end())
          {
      throw std::out_of_range("Invalid key: " + key);
          }
          return *iter;
  }
private:
  std::map<std::string, element> m_map;
};

---

Yet another option is to stick to what you have now, but state that if someone attempts to use a key that does not exist, the behaviour is undefined.

And yet another variation is:

Code:

---

class someclass {
public:
  element &get_element(std::string key)
  {
    assert(m_map.find(key) != m_map.end());
    return m_map.find(key)->second;
  }

private:
  std::map<std::string, element> m_map;
};

---

In laserlight's post there is an ill-placed return keyword that needs to be removed to compile correctly:

Code:

---

          std::map<std::string, element>:iterator iter = return m_map.find(key);

---

--
Mats

Quote:

---

Originally Posted by Daved

And yet another variation is:

Code:

---

class someclass {
public:
  element &get_element(std::string key)
  {
    assert(m_map.find(key) != m_map.end());
    return *(m_map.find(key));
  }

private:
  std::map<std::string, element> m_map;
};

---

---

An exception is preferable here. Assertions should be fired for programmer errors, not usage errors. They are also compiled out of release builds, usually.

That is why the assert is an option. Based on previous threads from the OP, it is entirely likely that an invalid key is a programmer error and can only happen due to programming error. It is one step better than the "undefined behavior" solution by itself. The fact that the assert is compiled out of release builds is a positive, because that situation should never occur and so there should be no need to waste time checking the iterator against end().

Why will this produce the following error:

Code:

---

class someclass {
public:
  element &get_element(std::string key) {
    assert(m_map.find(key) != m_map.end());
    return *(m_map.find(key));
  }

private:
  std::map<std::string, element> m_map;
};

---

Code:

---

error C2440: 'return' : cannot convert from 'std::pair<_Ty1,_Ty2>' to 'element &'

---

Thanks again!

Oh yes, we overlooked the fact that a map iterator is a pair consisting of the key and the value. Since you only want the value, you should return m_map.find(key)->second (or in my example, iter->second).

Quote:

---

Originally Posted by Daved

That is why the assert is an option. Based on previous threads from the OP, it is entirely likely that an invalid key is a programmer error and can only happen due to programming error.

---

Well, OK, I guess I don't recall any related threads, but this sure looked like a situation where recovery was possible.

l2u's had threads about this over the last couple weeks you can search for if you're curious about the back story. It doesn't really matter for the point at hand though, since we're just discussing potential solutions rather than specifically identifying the "correct" one.

I think the assert is a valid solution even if recovery is possible. Look at vector's operator[] and at() functions. Both present valid solutions to the problem. One uses undefined behavior (potentially with an assert or debug-only error of some kind). The other uses an exception. You use each in different circumstances depending on what you need.

Personally, I would normally prefer a defined solution despite potential performance costs because I'd rather have the code work in all cases than prematurely optimize. However, the implementation cost of catching the out_of_range error in an appropriate place may be significant, and so leaving the behavior undefined might be more appropriate for a situation that should never happen.

With:

Code:

---

return *(m_map.find(key)->second);

---

I get error:

Code:

---

error C2440: 'return' : cannot convert from 'std::string' to 'element &'

---

Element is the key, and std::string is what you store. iterator->first pointers to the key, and iterator->second points to the actual stored value.
You need to return std::string, because that's what you're storing in the map.

hmm... but the original post gives m_map as:

Code:

---

std::map<std::string, element> m_map;

---

Clearly, std::string is the key type, element is the mapped type.

But somewhat that cannot be it if iterator->second is element.
Anyway, the best way, I think, is just to post the code. This should clear any confusion.

Quote:

---

Originally Posted by laserlight

hmm... but the original post gives m_map as:

Code:

---

std::map<std::string, element> m_map;

---

Clearly, std::string is the key type, element is the mapped type.

---

I think I know what the problem is.. Class element has * operator overloaded and it returns a string (when I do *element).. So what the solution would be in that case so that I can leave the overloaded operator on the element class?

Thanks!

Quote:

---

I think I know what the problem is.. Class element has * operator overloaded and it returns a string (when I do *element).. So what the solution would be in that case so that I can leave the overloaded operator on the element class?

---

Oh, yes, now it makes sense. I thought something looked wrong.

Basically, I noted that a fix is to return m_map.find(key)->second. You did not do that, but returned *(m_map.find(key)->second) instead, which is incorrect.