returning reference

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?
Isnt this right:

Code:

---

options &get_options() const { return m_options; }

---

so that I'm allowed to call get_options().set and get functions?

Where should I put the 'const' in this case?

Thanks for help!

Code:

---

options &get_options() const { return m_options; }

---

This is correct - you are not modifying the content of your class.


--
Mats

Why do I get an error with this:

Code:

---

error C2440: 'return' : cannot convert from 'const options' to 'options &'

---

Ah, yes, I suppose if you do return a reference to a member variable, you are allowing the code outside the object to modify the object itself, so although the member function itself isn't doing any changing, it will potentially be modified by the new user of the reference: so either no const at all, or const option &get...() const ... [you can have both].

--
Mats

You have a choice between a getter/setter pair:

Code:

---

const options& get_options() const { return m_options; }
void set_options(const options& new_options) { m_options = new_options; }

---

Or to provide:

Code:

---

const options& get_options() const { return m_options; }
options& get_options() { return m_options; }

---

For the latter choice the non-const version of get_options() will be used unless the object is const.

Quote:

---

Originally Posted by laserlight

You have a choice between a getter/setter pair:

Code:

---

const options& get_options() const { return m_options; }
void set_options(const options& new_options) { m_options = new_options; }

---

Or to provide:

Code:

---

const options& get_options() const { return m_options; }
options& get_options() { return m_options; }

---

For the latter choice the non-const version of get_options() will be used unless the object is const.

---

Which options is better? What do you guys prefer?

The first option seems better to me, but I guess in that case I have to overload = operator for options class? Will it compile after I do this?

Quote:

---

Originally Posted by l2u

The first option seems better to me, but I guess in that case I have to overload = operator for options class? Will it compile after I do this?

---

You'd have to do that anyway even in the second example, because presumably the caller is obtaining a non-const reference in order to assign to it, right?

Quote:

---

The first option seems better to me, but I guess in that case I have to overload = operator for options class?

---

I prefer the first option too, and you do not need to write operator= since the compiler provided version will work correctly. Of course, if the compiler provided version would not work correctly, then you should implement it yourself either way.

I tend to prefer using set/get these days with a little helper class:

Code:

---

        template<typename Type, typename Class> class CUtilityMember
        {
        private:
                Type Data;
                typedef const Type& (Class::*GetFncPointer)();
                typedef void (Class::*SetFncPointer)(const Type& NewData);
                GetFncPointer m_pGet;
                SetFncPointer m_pSet;
                Class* m_pClass;
       
        public:
                CUtilityMember(Class* pClass, GetFncPointer pGet = NULL, SetFncPointer pSet = NULL);
                CUtilityMember(const Type& NewData, Class* pClass, GetFncPointer pGet = NULL,
                        SetFncPointer pSet = NULL);
                CUtilityMember& operator = (const Type& NewData);
                operator Type () const;
        };

        template<typename Type, typename Class> CUtilityMember<Type, Class>::CUtilityMember
                (Class* pClass, GetFncPointer pGet, SetFncPointer pSet): m_pClass(pClass), m_pGet(pGet),
                m_pSet(pSet) { }
        template<typename Type, typename Class> CUtilityMember<Type, Class>::CUtilityMember
                (const Type& NewData, Class* pClass, GetFncPointer pGet, SetFncPointer pSet):
                Data(NewData), m_pClass(pClass), m_pGet(pGet), m_pSet(pSet) { }
        template<typename Type, typename Class> CUtilityMember<Type, Class>&
                CUtilityMember<Type, Class>::operator = (const Type& NewData)
        {
                if (m_pSet)
                        (m_pClass->*m_pSet)(NewData);
                else
                        Data = NewData;
                return *this;
        }
        template<typename Type, typename Class> CUtilityMember<Type, Class>::operator Type () const
        {
                if (m_pGet)
                        return (m_pClass->*m_pGet)();
                else
                        return Data;
        }

---

And to use it:

Code:

---

                CFilter(CString strExt = _T(""), CString strDescription = _T("")): strExt(strExt, this),
                        strDescription(strDescription, this) { }
                CUtilityMember<CString, CFilter> strExt;
                CUtilityMember<CString, CFilter> strDescription;

---

Optionally, if you want to extra things rather than just set/get the value of the class, then you can provide a class function pointer to a get/set function within your own class.
It's not 100% finished yet, but it will allow you to do

Myclass.MyVar = somevalue;
And
sometype somevalue = Myclass.MyVar;

Without sacrificing the encapsulation. You can add more operators as necessary.

Quote:

---

Originally Posted by laserlight

I prefer the first option too, and you do not need to write operator= since the compiler provided version will work correctly. Of course, if the compiler provided version would not work correctly, then you should implement it yourself either way.

---

I still get the error if I define the set function as you said. What am I doing wrong?

Quote:

---

I still get the error if I define the set function as you said. What am I doing wrong?

---

Post the smallest and simplest program that demonstrates the error, and also post the exact error message.

Quote:

---

Originally Posted by laserlight

Post the smallest and simplest program that demonstrates the error, and also post the exact error message.

---

Sorry, my bad - I forgot to put const in front of get_options() function.

I want to make sure about something..

If I have this design:

Code:

---

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

class subclass {
public:
        subclass(options &opt) : m_options(opt) { }
private:
        options &m_options;
};

class someclass {
public:
        someclass() : m_subclass(m_options) { }

        const options &get_options() const { return m_options; }
        void set_options(const options& new_options) { m_options = new_options; }
private:
        options m_options;
        subclass m_subclass;
};

---

Now I do:

Code:

---

someclass test;
test.set_options(some_new_options);

---

Can this make a problem since I pass a reference of m_options to subclass object?
I think this shouldnt be a problem since it will only reassign the values or am I wrong?

It won't cause a problem because you're assigning to a non-reference type.
But I still question if this approach is a good one. You're going to have to remake that class for every other class that might use it. And I still feel using = to assign and just the name to get is a better approach (I may be wrong on that).

Why will this give the following error:

Code:

---

class someclass {
public:
        const someobject &get_object(std::string str) const { return m_map[str]; }
        void set_object(std::string str, object obj) { m_map[str] = obj; }
       
private:
        std::map<std::string, someobject> m_map;
};

---

error C2678: binary '[' : no operator found which takes a left-hand operand of type 'const someclass::map' (or there is no acceptable conversion)

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.

Quote:

---

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.

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.

Quote:

---

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?

Yes, it would.
std::map always inserts an object if it does not exist, when using the index operator.
If you only want to find and return an object, use find.
Find returns an iterator which you have to check against yourmap.end() (if it's equal, then the element doesn't exist) because the standard library was so poorly designed.

Quote:

---

Originally Posted by Elysia

Find returns an iterator which you have to check against yourmap.end() (if it's equal, then the element doesn't exist) because the standard library was so poorly designed.

---

I think this is one area where the standard library was well designed instead.

EDIT:
On second thought, if you are referring to the need to compare with map.end(), then perhaps that could have been done differently, e.g., to return a singular iterator that evaluates to false, whereas all non-end iterators evaluate to true. On the other hand, the safe bool idiom was not known at the time these class templates were designed, conversion to void* had its problems, and comparison with end() is consistent with the usual range comparison, so it is still a stretch to say that "the standard library was so poorly designed" in this area.

>> 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.

Quote:

---

Originally Posted by laserlight

I think this is one area where the standard library was well designed instead.

EDIT:
On second thought, if you are referring to the need to compare with map.end(), then perhaps that could have been done differently, e.g., to return a singular iterator that evaluates to false, whereas all non-end iterators evaluate to true. On the other hand, the safe bool idiom was not known at the time these class templates were designed, conversion to void* had its problems, and comparison with end() is consistent with the usual range comparison, so it is still a stretch to say that "the standard library was so poorly designed" in this area.

---

Well, I like to vent my frustration with how it was designed sometimes, so yes... :)
Comparing the iterator to true or false would be a big improvement, but perhaps another solution might be like MFC does it. Take a reference and return true/false.

Quote:

---

Originally Posted by Elysia

... because the standard library was so poorly designed.

---

Next thing you'll be telling a rocket scientist how to get a space shuttle to the moon.
In short, you're not qualified to make such a judgement. nor am I, you could say

I see you've learnt the CRTP.
The code you posted beginning with the following, is an over-engineered solution looking for a problem:

Code:

---

template<typename Type, typename Class> class CUtilityMember
...

---

I knew I would get blamed for that :p
But that's OK, we all have our opinions.
But let's just say it isn't a judgement, it's an opinion. A judgement it is when everyone agrees.

Quote:

---

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.

Quote:

---

Originally Posted by medievalelks

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.

---

Im a little confused about your post.. How else am I supposed to share the options across the different classes in my program? I thought this is the right approach towards that kind of problems in c++.

I think the design I mentioned is good because in case options change, I dont have to change the data in each class separetly (if each class holds the options it needs).

Quote:

---

Originally Posted by l2u

Im a little confused about your post.. How else am I supposed to shared the options across the different classes in my program? I thought this is the right approach towards that kind of problems in c++.

I think the design I mentioned is good because in case options change, I dont have to change the data in each class separetly (if each class holds the options it needs).

---

It's hard to tell without a more concrete example. What are these "options"? Are these program-wide settings that you are managing with the options class? If so, how are they initialized?

The way I've handled this is to create a singleton that manages all program settings, which are read from a text file and stored in a std::map. Clients of the class fetch object values by calling a getString() or getDouble() member function on the singleton, which returns the value of the setting or option.

Quote:

---

Originally Posted by medievalelks

It's hard to tell without a more concrete example. What are these "options"? Are these program-wide settings that you are managing with the options class? If so, how are they initialized?

The way I've handled this is to create a singleton that manages all program settings, which are read from a text file and stored in a std::map. Clients of the class fetch object values by calling a getString() or getDouble() member function on the singleton, which returns the value of the setting or option.

---

I dont like singleton design approach because I want to be more object oriented - to be able to have multi-session program (different options for same tasks).

Quote:

---

Originally Posted by l2u

I dont like singleton design approach because I want to be more object oriented - to be able to have multi-session program (different options for same tasks).

---

What is not OO about singletons?

Anyway, that solution is out since you want multiple instances of the options. In any event, I would hide options behind a queryable interface so you don't have multiple objects fiddling with what is essentially a POD. So instead of checking if options.FullScreen is true in different places, have them call OptionInterface.isFullscreen().

I like my own class member function interface better. It essentially makes it possible to use class members as PODs, yet they are object oriented in sense that you can wrap functionality in function calls. Essentially, it's like set/get.
I posted it on the 1st page.