C++ returning persistent objects

Question

I'm currently trying to learn best practices in C++ after coming from a C# background. I understand that there are three ways of handling objects:

• By value (objects are copied or moved when passed into and out of functions)
• By reference
• By shared or unique pointer (raw pointers are frowned upon unless you really need them)

In general I see it as good practice to avoid using shared pointers but as I'm developing a lot of code recently I'm finding that I initial define something as a value type and then end up having to make it a shared pointer. This situation occurs so frequently that almost every object in my system is in a shared pointer! This seems wrong.

Most of my classes look somewhat like this:

class Container
{
public:
    // ...other functions

    std::shared_ptr<Thing> GetThing() const;
    std::vector<std::shared_ptr<Thing>> GetThings() const;

private:
    std::shared_ptr<Thing> thing;
    std::vector<std::shared_ptr<Thing>> things;
}

Initially this class would have contained value objects of type Thing, but then other classes need access to these objects and so to avoid copying them when they get returned from the 'getter' functions I've put them into shared pointers. This means that if any changes occur to these objects their state will be consistent to the container and those objects currently accessing the 'things'.

Why does this feel wrong, and how can I improve this approach? What is the correct 'C++' way of doing this?

Answer 1

It is not clear what you do with those objects.

If you want to copy the non-copyable class, then using shared_ptr is fine as you did.

If you want to copy objects, then return a value.

If you just want to provide access to those objects, then use references :

class Container
{
public:
    // ...other functions

    const Thing& GetThing() const;
    const std::vector<Thing>& GetThings() const;

private:
    Thing thing;
    std::vector<Thing> things;
};

Answer 2

Return/pass by const ref before turning to shared_ptr. They let you pass by reference without allowing them to change the object. This requires that you take care to maintain const-correctness throughout.

class Container
{
public:
    // ...other functions

    const Thing& GetThing() const;
    Thing& GetThing();

    const std::vector<Thing>& GetThings() const;
    std::vector<Thing>& GetThings();

private:
    Thing thing;
    std::vector<Thing> things;
}

What is important is reasoning about ownership. With store-by-value and pass-by-reference you know exactly what owns the object and know who is responsible for destroying it.

Answer 3

It sounds like you are doing this because you want to modify the internal values inside Container. So you are doing something like:

void foo(Container &container) {
   container.GetThing().SetFoo(12);
}

The problem is that you are not supposed to be modifying Container's internal state this way. Only methods on Container should modify it. So this function should probably be a method insider Container.

void Container::foo() {
    thing.SetFoo(12);
}

If you really must modify thing outside of Container, you should prefer something more explicit:

void foo(Container &container) {
    Thing thing = container.GetThing();
    thing.SetFoo(12);
    container.SetThing(thing);
}

But having Get/Set methods for internal pieces of state in Container is a code smell. It suggests you've got other code that really should be part of Container.