Is returning a reference for accessor idiomatic?

Question

In C++, it is possible to create an accessor which returns a reference to a private field.

class Cls {
    private:
        int _attr;
    public:
        int& attr() { return _attr; }
};

such that the attribute can be accessed as such:

// set
c.attr() = 4;

// get
cout << c.attr() << endl;

Is this style of accessor idiomatic/good practice? Would an average C++ programmer be surprised seeing this style of accessor? (Hint: I was surprised the first time I saw this, but kind of liked the style)

Answer 1

Suppose you needed guarantees:

template <int Base>
class Digit {
  private:
    int attr_; // leading underscore names are reserved!
  public:
    int attr() const { return attr_; }
    void attr(int i) { attr_ = i % Base; } // !
};

In that case, you can't return a reference, since there's no way to guarantee that the user won't modify it in a way that you don't want. This is the main reason for using a setter - you can (now or at a later date) filter the input to make sure it's acceptable. With a reference you have to blindly accept what the user assigns to your member. At that point, why not just make it public?

Answer 2

No, that code would be unsurprising, as long as you also provide a const overload:

class Cls {
    int _attr;
public:
    int& attr() { return _attr; }
    int const& attr() const { return _attr; }
};

However, I would consider the following idiomatic, for the reasons mentioned by Chris Lutz and Mark B:

class Cls {
    int _attr;
public:
    int const& attr() const { return _attr; }
    void attr(int i) { _attr = i; }
};

Answer 3

In general, for non-trivial classes (for example ones that can be defined simply as a struct) providing accessors/mutators to specific attributes is probably a code smell. Generally, classes should tend to keep their internal state just that: Internal. If you provide a non-const reference to internal state then all of a sudden you have no control over class invariants at all. This will not only make debugging significantly harder since the scope of possible state changes is the entire project, it prevents you from ever changing the internals of your class because they're actually both state AND user API.

Instead you should probably devise methods that perform meaningful work on your class while maintaining the class invariants. In some cases providing read-only access to specific state variables is perfectly fine while in others you only want to provide access to end state in another way.

Answer 4

Doing so completely defeats the purpose of making it private in the first place.

The purpose of an accessor is to expose the internal state in such a way that the class can maintain invariants when external code tries to modify the state (either by only allowing read access, or by validating write access before modifying the state); they will usually look something like

int  attr() const {return _attr;}
void attr(int a)  {_attr = a;}

If, in the future, you need to constrain the values that the attribute can take, or change the way it is stored internally, then you can modify the implementation of these without changing the class interface.

If this is not necessary (and you have decided that it will never be necessary), just make it public; exposing it as a reference gains nothing but a few lines of unnecessary code and a quirky syntax when using it.

Answer 5

It depends. If the role of the class is to contain such objects (e.g. a container class), then its very idiomatic, and the normal way of doing things. In most other cases, however, it is considered preferrable to use getter and setter methods. Not necessarily named getXxx and setXxx---the most frequent naming convention I've seen uses m_attr for the name of the attribute, and simply attr for the name of both the getter and the setter. (Operator overloading will choose between them according to the number of arguments.)

-- James Kanze

Answer 6

Returning references to POD members is generally not too common. I think must readers would expect a setter for this purpose. However, if you do it, don't forget to overload it for the const case as well.

Answer 7

It's relatively frequent, but it's bad style.

If you have a look at the STL, for example, you'll remark that the class that yields const& or & to internal state are usually plain containers and only provide this kind of access for what you actually store in them. You have, obviously, no way to modify directly attributes such as size or the internal nodes of a binary tree.

Providing direct access to elements breaks encapsulation. Not only do you lose all class invariants wrt to those elements, but you also make them part of your API, and therefore cannot change the implementation of the class without also updating all clients.

As far as I am concerned, they are two types of classes in C++:

struct BigBag {
  Foo _foo;
  Bar _bar;
  FooBar _foobar;
};

class MyClass {
public:
  explicit MyClass(int a, int b);

  int getSomeInfo();

  void updateDetails(int a, int b);
private:
  // no peeking
};

That is: either it's just a collection of related elements, in which case everything is public and you're done, or there are class invariants and/or you're concerned with its API because there's a lot of client code, in which case you do not expose at all the implementation details.