"Avoid returning handles to object internals", so what's the alternative?

Question

Effective C++ by Scott Meyers tells in Chapter 5, Item 28 to avoid returning "handles" (pointers, references or iterators) to object internals and it definitely makes a good point.

I.e. don't do this:

class Family
{
public:
    Mother& GetMother() const;
}

because it destroys encapsulation and allows to alter private object members.

Don't even do this:

class Family
{
public:
    const Mother& GetMother() const;
}

because it can lead to "dangling handles", meaning that you keep a reference to a member of an object that is already destroyed.

Now, my question is, are there any good alternatives? Imagine Mother is heavy! If I now return a copy of Mother instead of a reference, GetMother is becoming a rather costly operation.

How do you handle such cases?

Ceporah

That depends on the reason I want to access this thing... – Emad

I'm trying so hard not to attempt a wisecrack at "Mother is heavy" :P – Duaneduarchy

Perhaps an std::shared_ptr<Mother>? – Windbound

Actually, you missed the point. It's not so much an issue of lifetime (though there is this as well), it's much more importantly a breach of encapsulation: and what if you'd like to replace that Mother class with a MotherImpl (more lightweight) class at some point in Family implementation ? How do you fulfill the contract of GetMother now ? – Alderson

@DeadMG: Well, I would say that YAGNI applies to returning by shared_ptr, it's certainly much more complicated than simply returning by copy ;) – Alderson

@LewsTherin: "his mother is so heavy, but SO heavy, that she wanted a banana and the function returned a gorilla HOLDING a banana..." – Mondrian

Answer 1

First, let me re-iterate: the biggest issue is not one of lifetime, but one of encapsulation.

Encapsulation does not only mean that nobody can modify an internal without you being aware of it, encapsulation means that nobody knows how things are implemented within your class, so that you can change the class internals at will as long as you keep the interface identical.

Now, whether the reference you return is const or not does not matter: you accidentally expose the fact that you have a Mother object inside of your Family class, and now you just cannot get rid of it (even if you have a better representation) because all your clients might depend on it and would have to change their code...

The simplest solution is to return by value:

class Family {
public:

    Mother mother() { return _mother; }
    void mother(Mother m) { _mother = m; }

private:
    Mother _mother;
};

Because in the next iteration I can remove _mother without breaking the interface:

class Family {
public:

     Mother mother() { return Mother(_motherName, _motherBirthDate); }

     void mother(Mother m) {
         _motherName = m.name();
         _motherBirthDate = m.birthDate();
     }

private:
     Name _motherName;
     BirthDate _motherBirthDate;
};

See how I managed to completely remodel the internals without changing the interface one iota ? Easy Peasy.

Note: obviously this transformation is for effect only...

Obviously, this encapsulation comes at the cost of some performance, there is a tension here, it's your judgement call whether encapsulation or performance should be preferred each time you write a getter.

Alderson

Answer 2

Possible solutions depend on actual design of your classes and what do you consider "object internals".

1. Mother is just implementation detail of Family and could be completely hidden from Family user
2. Family is considered as composition of other public objects

In first case you shall completely encapsulate subobject and provide access to it only via Family function members (possibly duplicating Mother public interface):

class Family
{
  std::string GetMotherName() const { return mommy.GetName(); }
  unsigned GetMotherAge() const { return mommy.GetAge(); }
  ...
private:
   Mother mommy;
   ...
};

Well, it can be boring if Mother interface is quite large, but possibly this is design problem (good interfaces shall have 3-5-7 members) and this will make you revisit and redesign it in some better way.

In second case you still need to return entire object. There are two problems:

1. Encapsulation breakdown (end-user code will depend on Mother definition)
2. Ownership problem (dangling pointers/references)

To adress problem 1 use interface instead of specific class, to adress problem 2 use shared or weak ownership:

class IMother
{
   virtual std::string GetName() const = 0;
   ...
};

class Mother: public IMother
{
   // Implementation of IMother and other stuff
   ...
};

class Family
{
   std::shared_ptr<IMother> GetMother() const { return mommy; }
   std::weak_ptr<IMother> GetMotherWeakPtr() const { return mommy; }

   ...
private:
   std::shared_ptr<Mother> mommy;
   ...
};

Pugnacious

GetWeakMother ma momma ain't weak! – Alderson

@MatthieuM. Changed to GetMotherByWeakPtr, just for you pleasure :-D – Pugnacious

Answer 3

If a read-only view is what you're after, and for some reason you need to avoid dangling handles, then you can consider returning a shared_ptr<const Mother>.

That way, the Mother object can out-live the Family object. Which must also store it by shared_ptr, of course.

Part of the consideration is whether you're going to create reference loops by using too many shared_ptrs. If you are, then you can consider weak_ptr and you can also consider just accepting the possibility of dangling handles but writing the client code to avoid it. For example, nobody worries too much about the fact that std::vector::at returns a reference that becomes stale when the vector is destroyed. But then, containers are the extreme example of a class that intentionally exposes the objects it "owns".

Ammons

Do you mean shared_ptr<const Mother>? – Encumber

I would not really recommend this. While it solves the immediate problem of a dangling reference it adds cost and complexity in the design that I don't think are needed. The lifetime of objects in C++ is not something that can be blindly ignored, just learn to live with it. Document what the guarantees for the validity of the returned reference is and move on :) – Napoleon

@DavidRodríguez-dribeas: you wouldn't recommend considering it, or you believe that the consideration normally will decide against the shared_ptr? I agree with the latter, not the former. For that matter I'd also question my very first assumption, "if a read-only view is what you're after" before complicating things. Returning by value is good, and if performance is an issue then there are options like immutable types or COW to consider before resorting to a view in place of a value. – Ammons

In short, any time where you think you want to break Meyers's advice and need an alternative might be an XY problem, and plenty of answers here try to address X in various ways. – Ammons

I see this as another realization of the Yogui Berra quote: In theory, theory and practice are the same. In practice they aren't (add to the same set, for example NVI). Yes, in theory that improves the design, in practice people just don't do it, and in some cases it might actually complicate things. (And yes, I would not recommend doing it, considering is always a good thing to do :) – Napoleon

Answer 4

This goes back to a fundamental OO principle:

Tell objects what to do rather than doing it for them.

You need Mother to do something useful? Ask the Family object to do it for you. Hand it any external dependencies wrapped up in a nice interface (Class in c++) through the parameters of the method on the Family object.

Counterchange

Hence you get functions printMotherName, printFatherName, printMaternalGrandfatherName etc. Sometimes it's possible to cut that thicket back with a useful abstraction, of course, such as printNameOfRelative(RelationShip). – Ammons

And it gets messier with structures like this because you can have lots of children in the graph. printSonName() doesn't work if there are 74 sons, so you need an overload that takes an index, and also a count getter, but dad might have remarried, so you might need the same for printMotherName()... it gets messy quickly. – Mesosphere

To over-simplify, I'd say that if your client code looks like graph traversal, then you might be able to make the Visitor pattern work but otherwise the fundamental OO principle probably isn't applicable. There's a conceptual difference between the family "having a mother" and controlling all access to that mother from clients of the Family interface, vs the family and the mother being reasonably independent objects whose relationship to each other might be observed by code that is a client of both the Family interface and the Mother interface. – Ammons

Yes, and applied enthusiastically the Law of Demeter forbids file.getLine(0).rstrip(), on the basis that users of the File interface shouldn't thereby be coupled to the String interface. So you write file.getrstrippedLine(0) instead. You have to make a judgement call, when it's practical for users of the Family interface to avoid also using the Mother interface. And if it's practical, do it. – Ammons

I certainly don't disagree with anything here yet. No need to over complicate. I was just pointing out a way to eliminate the problem and make sure things stay nice and decoupled. My code is full of double dotted lines, and I'd like to believe it's still pretty darn maintainable. – Counterchange

Answer 5

because it can lead to "dangling handles", meaning that you keep a reference to a member of an object that is already destroyed.

Your user could also de-reference null or something equally stupid, but they're not going to, and nor are they going to do this as long as the lifetime is clear and well-defined. There's nothing wrong with this.

Boldt

This becomes not so obvious if actual Mother consumer is not the code that acquires it. When e.g. it gets passed through some non-trivial stack of calls. – Pugnacious

One way of making lifetime clear and well-defined is to not hand out references to the object in question willy-nilly, of course ;-) – Hartsell

Answer 6

It's just a matter of semantics. In your case, Mother is not Family internals, not its implementation details. Mother class instance can be referenced in a Family, as well as in many other entities. Moreover, Mother instance lifetime may even not correlate with Family lifetime.

So better design would be to store in Family a shared_ptr<Mother>, and expose it in Family interface without worries.

Weiweibel