Thread: returning reference

std::map<>:: operator[] is non-const, so you can't call it from inside a const function. Use find() instead.

Note that you will have problems if the key does not exist.

One option is to make the map mutable, which would allow you to leave the other code the way it is. This only makes sense if you want to allow users to specify an invalid key and you'd rather return a new default constructed someobject than throw any kind of error. Making the map mutable unfortunately no longer allows the compiler to error out when you accidentally modify it, but if the behavior I described is what you want then that might be the best solution.

You could also just throw an exception on an invalid key or document the function to say that invalid keys cause undefined behavior. Those options make more sense if you don't expect the user to ever specify an invalid key.

Someone mentioned once on this forum that is totally okay to have that kind of functions and just require from the user to check is_key_in_map() before calling the get_object() function so I thought it would be okay to make it this way and always do the checking first.

Originally Posted by l2u

Someone mentioned once on this forum that is totally okay to have that kind of functions and just require from the user to check is_key_in_map() before calling the get_object() function so I thought it would be okay to make it this way and always do the checking first.

It's not okay because the operator[] itself isn't const. Even if you verify the key is in the map, the non-constness doesn't magically go away. Again... Use find()

>> Someone mentioned once on this forum that is totally okay to have that kind of functions and
>> just require from the user to check is_key_in_map() before calling the get_object() function so
>> I thought it would be okay to make it this way and always do the checking first.

Sure it's ok. As I said, in that case you could just throw an exception on an invalid key or document the function to say that invalid keys cause undefined behavior. You still should make a decision on how to handle when find() doesn't find the key. If you just dereference the result of find, then make sure you document that invalid keys are not allowed and undefined behavior will result.

Or you can define the behavior by checking the result of find() against end() and throwing an exception, although it sounds like you don't want to add that complexity. If not, then add an assert that verifies the result of find is not end().

BTW, I'm assuming you understood that if you don't make the map mutable (which you shouldn't because it doesn't fit your use case) then you have to use find().

I dont want to allow users to specify an invalid key (this shouldnt happen).

So the only way is to go with find() (as I understood).. Do you think it would be better in this case to check is the key exists and throw exception if it doesnt?

I think this is something similar to:

Code:

someclass {
public:
  std::string get_name(int n) { return m_names[n]; }
private:
  std::vector<std::string> m_names;
};

Should I check with all that kind of functions if the name exists (and throw exception in other case) although users shouldnt be allowed do get() before doing the check first.

Originally Posted by Elysia

You should check before the user does a get, I think.
Also note that operator [] inserts an element if it doesn't exist, so if the user did specify an invalid key, it would insert a new key instead of complaining. So yes, I think find and throwing an error might be a good approach.

Im not sure this would add an element if it doesnt exist:

Code:

const someobject &get_object(std::string str) const { return m_map[str]; }

So before return I should make checking anyway and throw an exception?

>> Should I check with all that kind of functions if the name exists

Again, that's up to you. If you know the calling functions will be smart and not use an invalid key, then just assert in debug mode if find() returns end(). In Release mode you would just use the result of find directly and if the user uses a key that doesn't exist then it is their fault.

Throwing an exception is a good choice, but it has performance implications. If you don't have other checks in place in your code then using an exception is a good idea.

Originally Posted by l2u

Hello

If I have the following data structure:

Code:

class options {
public:
	void set_value(int v) { m_value = v; }
	int get_value() { return m_value; }
private:
	int m_value;
};

class someclass {
public:
	options &get_options() { return m_options; }
private:
	options m_options;
};

What is the right way to return m_options by reference?

Don't do it. Why do you want to return a reference to your private data? If your object is really a POD, make it a struct.

I'd also revisit the design. If you have a bunch of classes passing around and operating on a data struct, you may want encapsulate that data in an object with real behavior.