59

What is the best practice for a C++ getter method which is supposed to return a non trivial type, but a member which is of type class, or struct.

  1. Return by value, such as: MyType MyClass::getMyType() { return mMyType; }
  2. Return by const reference: const MyType& MyClass::getMyType() { return mMyType; }
  3. Return by address: MyType* MyClass::getMyType() { return &mMyType; }

where

class MyType { /* ... */ };

class MyClass
{
  private:
     MyType mMyType;
}

I specifically worry about the following usages of this method. Can you please elaborate in details how this might affect copying the object, and the danger of dangling references and wild gone pointers if function() wants to save it for further usage.

MyType* savedPointer;

SomeType function(MyType* pointer) { savedPointer = pointer; };

a. valid for 1. and 2.

{
  MyType t = myClass.getMyType();
  function(&t);
}

// is savedPointer still valid here?

b. valid for 1. and 2.

{
  const MyType& t = myClass.getMyType();
  function(&t);
}

// is savedPointer still valid here?

c. valid for 1. and 2.

{
  MyType& t = myClass.getMyType();
  function(&t);
}

// is savedPointer still valid here?

d. valid for 3.

{
  MyType* t = myClass.getMyType();
  function(t);
}

// is savedPointer still valid here?

where myClass is an object of type MyClass.

braaterAfrikaaner
  • 1,072
  • 10
  • 20
Ferenc Deak
  • 34,348
  • 17
  • 99
  • 167
  • It depends so much on the context. Do you want to see modifications inside the class (e.g. modify a vector element) or do you want to exlicitely only have the value, but never(!) have any modifications to the original? In my opinion, pointer shouldn't be used in general. Container types should return by ref or const ref and the rest by value (or even no getter at all!). The point of classes is data encapsulation (among other features). Always returning by non-const reference would be silly. – stefan Nov 20 '13 at 08:48
  • You can also return smart pointers which save you a lot of worrying... – jbat100 Nov 20 '13 at 08:49
  • @jbat100 You _cannot_ return a smart pointer here. There is no dynamically allocated object (nor should there be). – James Kanze Nov 20 '13 at 09:43
  • @JamesKanze agreed, to return a smart pointer the instance variable must be a smart pointer, that's what I meant – jbat100 Nov 20 '13 at 09:45
  • @jbat100 But why would you make the instance variable a smart pointer? – James Kanze Nov 20 '13 at 09:56
  • @JamesKanze well there are many situations where it's justified, perhaps not here but given the OP was worried of the "danger of dangling references and wild gone pointers", I thought mentioning smart pointers would be adequate. – jbat100 Nov 20 '13 at 10:08
  • @jbat100 There are very, very few situations where a smart pointer member is justified (except maybe `unique_ptr`, but in that case, you don't want to return a "copy" of the `unique_ptr`). – James Kanze Nov 20 '13 at 10:20
  • let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/41522/discussion-between-jbat100-and-james-kanze) – jbat100 Nov 20 '13 at 10:23
  • There's no "should" answer to your question. It depends on your intent, what access level youa re willing to provide and how much implementation details you are willing to expose. See here for a more detailed answer: https://stackoverflow.com/a/2977172/187690 – AnT stands with Russia Feb 01 '18 at 19:35

5 Answers5

31

You can provide both const and non-const versions:

MyType       & MyClass::getMyType()       { return mMyType; }
MyType const & MyClass::getMyType() const { return mMyType; }

I wouldn't provide a pointer version, since that implies that the return value might be the null pointer, which it can never be in this instance.

The real point, however, is that you are basically giving the caller direct access to the internal object. If this is your intent, then you may as well make the data member public. If it isn't, then you will need to work harder to hide the object.

One option is to retain the MyType const & accessor, but provide more indirect means to modify the internal object (setMyType(…) or something more tailored to the semantics that you are trying to express at the level of the containing class).

Marcelo Cantos
  • 181,030
  • 38
  • 327
  • 365
  • 3
    @Simple: Absolutely. I was getting there… – Marcelo Cantos Nov 20 '13 at 08:49
  • 9
    The caveat here is that encapsulation is lost, so one could argue that you might as well make the member public. Of course, the getters allow you to change the implementation later. – juanchopanza Nov 20 '13 at 08:50
  • @juanchopanza once you've decided that your interface returns a `MyType&` then changing the implementation later is pretty much impossible without breaking the ABI. – Simple Nov 20 '13 at 08:52
  • 2
    @Simple: not impossible - you could potentially start returning a reference to one of several objects in an array within `MyClass`, or a dynamically allocated instance (which might be created on demand), or a thread-specific instance etc. – Tony Delroy Nov 20 '13 at 08:56
  • Keeping the accessor rather than making the member public allows you to add a breakpoint every time the element is accessed. It is a justification for the accessor by itself ! – Johan Nov 20 '13 at 09:00
  • 1
    @Johan: Sort of. Callers can cache the value after accessing it once. – Marcelo Cantos Nov 20 '13 at 09:02
  • That implies bad coding style :) If you make the member public, even good coding style won't help you. – Johan Nov 20 '13 at 09:02
  • 1
    @Johan: Caching is bad coding style? – Marcelo Cantos Nov 20 '13 at 09:03
  • @MarceloCantos No, hence the ":)". But when you're caching data you have to know you're adding to the ownership/accessing and debugging problem. And there often arise the bad coding style. – Johan Nov 20 '13 at 09:05
  • 1
    The only time in my mind when you need getters/setters is when you have invariants to maintain. `pair` just has public members because there are no invariants. `vector` on the other hand has a number of getters and setters because it needs to manage its size, capacity, pointer to the beginning, etc. @John why do you need a break point on access? It sounds like what you really want is a data break point, which you don't need an accessor for. – Simple Nov 20 '13 at 09:07
  • if you really want to do public members. at least hide them inside a struct (implicitly convert them to the type you want). this allows you to overload asignment operator later if required. – Alexander Oh Nov 20 '13 at 09:20
  • @Simple Data break point won't work when you have a bunch of instance. I needed the accessor to simply find when someone is accessing to our data too often, asserting that the data was correct when the client accessed it, ... is often vital. And simple code review is not always an option when you're dealing with large software or several entities (client/software provider). – Johan Nov 20 '13 at 09:23
  • 1
    @Simple Once you return a reference, even if it is const, you've locked yourself into a specific implementation. (Of course, one might argue that you could change the const reference to a value later, but even that may affect calling code.) – James Kanze Nov 20 '13 at 09:54
25

In general, you should prefer return by value, unless you explicitly want to guarantee that the reference will designate a member (which exposes part of your implementation, but is desirable in cases like std::vector<>::operator[]). Returning a reference prevents later changes in class, since it means that you cannot return a calculated value. (This is especially important if the class is designed to be a base class, since returning a reference creates this restriction for all derived classes.)

The only time you should return by pointer is if a lookup or something is involved, which may return in having to return a null pointer.

Returning a reference to const may be a valid optimization, if the profiler indicates performance problems here, and the call site can also deal with a const reference (no modification of the returned value, no problems with lifetime of object). It must be weighed against the additional constraints on the implementation, of course, but in some cases, it is justified.

James Kanze
  • 150,581
  • 18
  • 184
  • 329
3

I would always return a const reference. If you need to modify the value it is returning just use a setter function.

Javier Castellanos
  • 9,346
  • 2
  • 15
  • 19
  • 4
    Returning a const reference imposes significant constraints on the implementation of the class, and at least partially defeats the purpose of having a getter. – James Kanze Nov 20 '13 at 09:51
  • 5
    The purpose of having a getter is to access the stored information, not to modify it. By having the getter function return a const reference you ensure that the object's state can be only modified through setters. This gives you some really nice compile time optimizations and makes easier to catch some bugs. If you think you need to get a fresh copy of the item you can always use the copy constructor explicitly (which would be called anyways if you are returning and object not by reference) or if you need to modify a lot of the internal data of the object you can provide an helper function. – Javier Castellanos Nov 21 '13 at 06:25
  • 1
    The purpose of using a function to access attributes, rather than just making the data member visible, is to not lock the code into a particular implementation. – James Kanze Nov 21 '13 at 10:57
2

Return by value, such as: MyType MyClass::getMyType() { return mMyType; } should be avoided as you will copy the content of your object. I do not see the gain you could have but I see the drawbacks on performance.

Return by const reference: const MyType& MyClass::getMyType() { return mMyType; } is more generaly used this way:

const MyType& MyClass::getMyType() const { return mMyType; }
MyType& MyClass::getMyType() { return mMyType; }

Always provide the const version. The non-const version is optional as it means that someone can modify your data. It is what I would encourage you to use.

Return by address: MyType* MyClass::getMyType() { return &mMyType; } is mostly used when the data is optionally there. It often has to be checked before being used.

Now, your use case, I would strongly advice not to keep a pointer save for more than a scope. I can often lead to ownership problems. I you have to do so, take a look to shared_ptr.

For your examples, there is two cases:

a. savedPointer won't be valid any more after the closing brace.

b,c, and d. savedPointer is valid after the closing brace, but beware it should not outlive myClass.

Johan
  • 3,728
  • 16
  • 25
  • Return by const reference imposes significant constraints on the implementation of the class, and should only be used when the profiler says it is necessary; return by value is the default. – James Kanze Nov 20 '13 at 09:52
  • 6
    @JamesKanze I disagree, what constraints ? Returning by value will always create a copy... that's a big constraint by itself. – Johan Nov 20 '13 at 09:58
0

a) MyType t = will create a copy of the object for both 1 and 2. Saved pointer will not be valid once t is out of scope.

b) The saved pointer will be valid for the case returning a reference, but invalid for the case returning an object. For the reference, the lifetime of the pointer would be the same as myClass. Of course &t on a const ref would be a const t* not a t* and so would fail to cast in your call to function(MyType*).

c) Same as b, though the code is invalid for 2 because you can't cast a const MyType& to a MyType&. Generally this would be bad practice and the const form would be more acceptable.

d) savedPointer will have the same lifetime as myClass.

I'd generally lean toward either returning a reference or a const reference depending on what you expect to be able to do with the return value. If you return a reference (non-const), you can do things like: myClass.getMyType() = ..., while if you return a const reference, the object is read-only.

Charlie
  • 1,487
  • 10
  • 9