7

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?

  • 1
    Why do you end up having to make it a shared pointer? Why are you using reference pointers to point to value types? – Robert Harvey Oct 22 '15 at 15:25
  • 1
    Also, this post seems like a good summary of when to use each type of pointer. – Robert Harvey Oct 22 '15 at 15:28
  • 7
    Your approach to use a shared ptr is because in C# every reference is a like a shared ptr, so you're applying C# design to your C++ classes. Don't be afraid to use value types and copy by value. The system has so many optimisations in this area you almost never need to treat everything as a shared heap object (eg. research RVO and Move operators) – gbjbaanb Oct 22 '15 at 15:30
  • 3
    I highly recommend that you read Effective Modern C++: 42 Specific Ways to Improve Your Use of C++11 and C++14 by Scott Meyers. This book goes into excruciating detail about how to use the new smart pointers, figuring out who should own which object, move semantics, and other topics relevant to this question. – user22815 Oct 22 '15 at 16:08
  • To give useful advice, we have to understand why those objects are being modified, how they are modified, why they can't be copied (i.e. why the modifications need to be shared / made visible to multiple owners), what are the performance implications of your current choice / better choice, and whether your current choice is thread-safe, among many other factors to be considered. – rwong Oct 23 '15 at 1:55
6

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;
};
  • What type would you make the 'thing' objects if you want Container to own the objects but also wanted to use dependency injection in the constructor? – innova Oct 23 '15 at 8:15
  • @innova Then a reference to Thing is fine. It just depends if you need const or not const reference in the return type. Assuming you want to inject thing and things into another class through it's constructor. – BЈовић Oct 23 '15 at 8:26
  • If thing is given to container as a reference then the object that passed thing to container owns the object though? When thing goes out of scope it will be deleted and container will no longer be able to access it... – innova Oct 23 '15 at 8:37
  • @innova thing in the Container is a value type, created in the constructor, and it owns that object. I would advise you to find a good c++ book, and read about references. – BЈовић Oct 23 '15 at 9:09
  • 1
    "If you want to copy the non-copyable class" is strangely worded. By design you can't copy a non-copyable object. You can pass around references to one, however. shared_ptr is only necessary when you don't know which of two scopes will live longest. Almost always one of those places has an obviously shorter lifespan than the other – Caleth Feb 28 '18 at 15:36
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.

  • most of the lines of this are syntax errors. If you mean to return by reference, the & goes after the type name – M.M Oct 23 '15 at 7:16
  • Research around to check that how I'm using the objects, this approach generally matches in most cases. What about instances where you want the Container to own the object, but you also want to be able to perform dependency injection? – innova Oct 23 '15 at 8:09
  • There's little point in exposing a private member variable through a getter that returns a non-const reference. You might as well make the member variable public to reduce the boilerplate code. – D Drmmr Mar 2 '18 at 11:53
1

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.

  • The problem with giving methods to the container is that it can become a bit of a god class if it has lots of objects in it, I guess that's down to architecture design though. – sydan Oct 22 '15 at 16:15
  • @sydan If it has a lots of objects in it, then it is already a god class anyway, or it has violated SRP somehow. A judicious use of predicates and functors may allow the caller to specify "actions" that are applied on a subset of those objects, without the god class having to implement every specific way of processing (modifying or filtering) them, Ultimately a refactoring is needed. – rwong Oct 23 '15 at 1:57
  • In fact, one approach is to have one designated object that is "the ultimate owner of everything", and every other object merely keeps a "weak_ptr" to this ultimate owner. For any one object to access another object, it will have to ask this ultimate owner for access. – rwong Oct 23 '15 at 2:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.