CODEWITHSUNDEEP
c++
kamwoz
kamwoz

Reputation: 226

Modifying private object properties through method which returns reference

I'm curious if that's proper way of assignement

class Foo { 
    int x_;

    public:
    int & x() {
        return x_;
    }
};

My teacher is making assignement like that: obj.x() = 5; But IMO that's not the proper way of doing it, its not obvious and it would be better to use setter here. Is that violation of clear and clean code ? If we take rule that we should read the code like a book that code is bad, am I right ? Can anyone tell me if am I right ? :)

Upvotes: 0

Views: 241

Answers (4)

Valentin Trinqué
Valentin Trinqué

Reputation: 1072

IMO, this code is not a good practice in terms of evolution. If you need to provide some changes checking, formatting, you have to refactor your class API which can become a problem with time.

  1. Having set_x() would be a way cleaner. Moreover, it will allow you to have checking mechanics in your setter.
  2. a proper getter get_x() or x() could also apply some logic (format, anything...) before returning. In your case, you should return int instead of int& since setter should be used for modification (no direct modification allowed).

And truly speaking, this code doesn't really make sense... it returns a reference on a property making it fully modifiable. Why not having directly a public property then ? And avoid creating an additional method ?

Do you want control or not on your data? If you think so, then you probably want a proper getter and setter. If not, you probably don't need a method, just make it public.

To conclude, I would say you are right, because the way you see it would make it better over the time, prone to non-breaking change, better to read.

As the UNIX philosophy mentions : "Rule of Clarity: Clarity is better than cleverness."

Upvotes: 1

Vlad from Moscow
Vlad from Moscow

Reputation: 311048

In general such a code confuses readers.

obj.x() = 5;

However it is not rare to meet for example the following code

std::vector<int> v = { 1, 0 };
v.back() = 2;

It is a drawback of the C++ language.

In C# this drawback was avoided by introducing properties.

As for this particular example it would be better to use a getter and a setter.

For example

class Foo { 
    int x_;

    public:
    int get_value() const { return x_; }
    void set_value( int value ) { x_ = value; }
};

In this case the interface can be kept while the realization can be changed.

Upvotes: 0

463035818_is_not_an_ai
463035818_is_not_an_ai

Reputation: 122820

In this example, the effect of returning a (non-const) reference is the same as if you made the variable public. Any encapsulation is broken. However, that is not a bad thing by default. A case where this can help a lot is when the variable is part of a complicated structure and you want to provide an easy interface to that variable. For example

class Foo {
    std::vector<std::list<std::pair<int,int>>> values;
    public: 
        int& getFirstAt(int i){
            return values[i].[0].first;
        }
};

Now you have an easy access to the first element of the first element at position i and dont need to write the full expression every time.

Or your class might use some container internally, but what container it is should be a private detail, then instead of exposing the full container, you could expose references to the elements:

class Bar {
    std::vector<int> values;   // vector is private!!
    public:
       int& at(int i){         // accessing elements is public
           return values.at(i);
       }
};

Upvotes: 0

Dietmar K&#252;hl
Dietmar K&#252;hl

Reputation: 153945

Assuming that x() happens to be public (or protected) member the function effectively exposes an implementation: the is an int held somewhere. Whether that is good or bad depends on context and as it stands there is very little context.

For example, if x() were actually spelled operator[](Key key) and part of a container class with subscript operator like std::vector<T> (in which case Key would really be std::size_t) or std::map<Key, Value> the use of returning a [non-const] reference is quite reasonable.

On the other hand, if the advice is to have such functions for essentially all members in a class, it is a rather bad idea as this access essentially allows uncontrolled access to the class's state. Having access functions for all members is generally and indication that there is no abstraction, too: having setters/getters for members tends to be an indication that the class is actually just an aggregate of values and a struct with all public members would likely serve the purpose as well, if not better. Actual abstractions where access to the data matters tend to expose an interface which is independent of its actual representation.

Upvotes: 0

Related Questions