One-to-many multiplicity pattern

Question

The following working C++11 code demonstrates working code for a pattern for representing one-to-many relationships between objects.

GitHub

(A variant of that code is also copied inline at the bottom of this post).

I think these are the desirable features:

• Children only created through owning parent so that they are managed by parent.
• std::shared_ptr and related utilities used for memory management.
• const-correctness

Notes:

• The link to GitHub uses abstract names (A, B) instead of the concrete example used here (Model, Component).
• The abstract names in the GitHub version hint at a possible template implementation

Model.h

#ifndef Model_h
#define Model_h

#include "Component.h"
#include <vector>
#include <string>


//! For collection of const children (i.e. each element immutable).
//! Overall collection is also not mutable
typedef std::vector<std::shared_ptr<const Component>> const componentCollectionConstT;

//! Overall collection is not mutable, but each element is mutable
typedef std::vector<std::shared_ptr<Component>> const componentCollectionT;

//! The class for a parent object
class Model :
  public std::enable_shared_from_this<Model> // Needed so that a reference to the Model parent object creating the Component child object can be stored by the child component object.
{
    //! Children
    std::vector<std::shared_ptr<Component>> components;

    //! Model name
    std::wstring mName;

public:
    //! Construct passing in value for name.
    //! \param name will be stored as the model name.
    Model(const std::wstring name);

    //! Getter for name
    //! \return the name
    const std::wstring getName() const;

    /** Get read-only collection of children components
     *
     * Each child element is immutable.
     * \return The collection of children, read-only.
     */
    componentCollectionConstT getComponentsReadOnly() const;

    /** Get collection of mutable children
     *
     * Each child element is mutable.
     * Overall membership of collection is still fixed.
     * \return The collection of mutable children.
     */
    componentCollectionT getComponents();

    /**
     * Create a child component object, of which this object is the parent
     * \return a pointer the newly created child component object.
     */
    const std::shared_ptr<const Component> createComponent(int);
};

#endif

Model.cpp

#include "Model.h"
#include <memory>
#include <algorithm>

using namespace std;

Model::Model(wstring name) :
  mName(name)
{}


const wstring Model::getName() const {
    return mName;
}


componentCollectionConstT Model::getComponentsReadOnly() const {
    componentCollectionConstT componentsC(components.begin(), components.end()); // Have to copy to get const version.
    return componentsC;
}


componentsCollectionT Model::getComponentss() {
    return components;
}


const shared_ptr<const Component> Model::createComponent(int count) {
    shared_ptr<Component> componentSP(new Component(count, shared_from_this())); // Have to use "shared_from_this"
    components.push_back(componentSP);
    return componentSP;
}

Component.h

#ifndef Component_h
#define Component_h

#include <memory>

class Model;

//! Components group aspects of the model.
class Component {
    friend class Model;

    //! Constructor kept private so that only Model (which is a friend) can construct. \see Model::createComponent
    Component(const int count, std::weak_ptr<Model> model);

    //! Some arbitrary data member as a place-holder for more interesting content in the real world.
    int mCount;

    //! Reference from child back to parent.
    //! This needs to be a weak_ptr to avoid circular ref counting issue.
    std::weak_ptr<Model> mModel;

public:
    //! Getter for arbitrary data member.
    const int getCount() const;

    //! Setter for arbitrary data member.
    void setCount(const int);

    //! Get a weak reference to parent model object.
    const std::weak_ptr<const Model> getModel() const;
};


#endif

Component.cpp

#include "Component.h"
#include "Model.h"

using namespace std;

Component::Component(const int count, weak_ptr<Model> model):
  mCount(count),
  mModel(model)
{}


const int Component::getCount() const {
    return mCount;
}

void Component::setCount(const int count) {
    mCount = count;
}

const weak_ptr<const Model> Component::getModel() const {
    return mModel;
}

main.cpp

/**
 * Demonstrates one-to-many relationship with parent ownership semantics,
 * and bi-directional navigability.
 * Memory management using std::shared_ptr and related utilities.
 *
 * Components are created through Model, so that they are never orphans.
 * Also, the collection of children of a Model object cannot have members added or removed.
 *
 * So far this has only been tested on XCode 5.1.1 and 6.0.1
 */

#include <iostream>
#include <memory>

#include "Model.h"
#include "Component.h"

using namespace std;


int main(int argc, const char * argv[])
{

    // Demonstrate creating a parent model, and creating child Components through the parent.
    // The parent is mutable.
    // Recommendation: all handle's are shared_ptr. (Or perhaps mandatory, since createComponent calls "shared_from_this"?)
    shared_ptr<Model> a(new Model(L"The first model object"));
    a->createComponent(5);
    a->createComponent(55);
    a->createComponent(5555);
    a->createComponent(-345);
    a->createComponent(2316724);
    a->createComponent(0);
    componentCollectionT bs = a->getComponents();
    //components.push_back(make_shared<Component>(42)); // Compile error <= const (otherwise new children could be "smuggled in"); and Component constructor private.
    for(auto b : bs) {
        b->setCount(b->getCount()*10000); // Demonstrate that each child is mutable.
        auto aName = b->getModel().lock()->getName();
        wcout << b->getCount() << " - " << aName  << endl;
    }

    cout << "**********************" << endl;

    // Repeat the above output (this demonstrates that we really modified the underlying children, not just copies of them.
    for(auto b : a->getComponents()) {
        wcout << b->getCount() << " - " << b->getModel().lock()->getName() << endl;
    }

    cout << "**********************" << endl;

    // If we start out with a const parent (i.e. instance of Model), we can't even add children!
    const shared_ptr<const Model> ac = const_pointer_cast<const Model>(make_shared<Model>(L"The second model object"));
//    ac->createComponent(5); // Compile error <= const.
//    componentCollectionT bsc = ac->getComponents(); // Compile error <= const.
    componentCollectionConstT bsc = ac->getComponentsReadOnly();
//    bsc[1]->setCount(444); // Compile error <= const.
//    bsc.push_back(1); // Compile error <= const.
    for(auto b : bsc) {
        wcout << b->getCount() << " - " << b->getModel().lock()->getName() << endl;
    }

    cout << "**********************" << endl;

    // If we have a const child, then the handle we get to its parent is also const.
    const shared_ptr<Component const> b1(a->createComponent(21112));
    auto ac2 = b1->getModel().lock();
    auto bsc2 = ac2->getComponentsReadOnly();

    for(auto b : bsc2) {
        wcout << b->getCount() << " - " << b->getModel().lock()->getName() << endl;
    }

//    ac2->createComponent(5); // Compile error <= const.
//    componentCollectionT bsc22 = ac2->getComponents(); // Compile error <= const.
//    bsc2[1]->setCount(444); // Compile error <= const.
//    bsc2.push_back(1); // Compile error <= const.


    return 0;
}

Answer 1

There are way too many const in your code. const-correctness is great, but you are overdoing it. A few tips to improve your code:

• When I run g++ with the option -Wextra, I get a warning with this piece of code:

  const int Component::getCount() const {
      return mCount;
  }
  

  The warning says:

  warning: type qualifiers ignored on function return type [-Wignored-qualifiers]

  It means that the const qualifier will simply be ignored by the compiler. You gain nothing by putting it there. When you return built-in types, const-qualifying them doesn't do anything at all. You can - and should - remove this kind of const-qualification. See this answer for more details.

• Actually, you could remove the const qualification when you return a type by value. Take this piece of your code:

  const wstring Model::getName() const {
      return mName;
  }
  

  const-qualifying wstring in the return type inhibits move semantics, which means that unless the return value optimization kicks in, mName will always be copied. If you remove the const qualification from the return type, some functions may perform a move operation instead. Bottom line: don't const-qualify when you return by value. const-qualifying pointers and references in return types is fine.

• Let's talk about this constructor:

  Model::Model(wstring name) :
    mName(name)
  {}
  

  First, you declared name to be const in the header but dropped the qualification in the implementation, you should fix this. That said, what is the best fix? There have been many debates, even among experts, about whether arguments should be passed by value or const reference. The answer is actually quite long and too complicated to be explained here. However, Herb Sutter talked about this at CppCon2014 (see the slides, page 33) and for your particular case where a constructor simply copies its parameters, the best solution is to pass by non-const value then to move:

  Model::Model(wstring name) :
    mName(std::move(name))
  {}
  

  The reason is that such a constructor may take advantage of move semantics if an rvalue is passed to it and it won't cost you more that a constructor taking a const reference if an lvalue is passed to the constructor. Remember that this is a very specific case though.

• The methods *ReadOnly don't really feel alright. Actually, the const qualification of the function should be the element that tells whether you are using the read-only overload or the other one (you can overload on const). I would simply remove the ReadOnly suffix from the method's names. However, I am not 100% sure for this one, you should try to find articles about when it is a good idea to overload a method on const and when it's not.

• Since you are using C++11, you could use the new type alias instead of typedef. It may improve readability (read dest = source instead of set source dest):

  using componentCollectionT = const std::vector<std::shared_ptr<Component>>;
  

• You should order your includes in alphabetical order. That will allow you to perform a faster visual search (yes, I am talking about humans here, not computers) to check whether you already included some header or not. When your include list grows, it really helps.

• Try to avoid naked new when you use std::shared_ptr, use the factory function std::make_shared instead:

  shared_ptr<Component> componentSP(make_shared<Component>(count, shared_from_this()));
  

  It is safer since you may avoid memory leaks if an exception is thrown during construction. And it may even be faster. Have a look at the "Notes" section in the documentation I linked if you want more details. However, since Component's constructor is private, you will need to work around that and that may be tricky. The obvious fix would be to befriend std::make_shared, but that may not work since some implementations use a helper function, and you would then have to befriend the helper function, which is not portable. The two portable ways to achieve this would either be to make Component's constructor public or to create a helper function. See this answer for a good workaround.

Answer 2

I don't think this pattern will serve you well. The main reason is that I don't believe a Model should hold the instances of Component.

I have to create a concrete example in order to explain this. We are on StackExchange, so let us say that a User has many Questions. Following your pattern, how do we get a list ( more than one ) of questions that have not been answered? We have to iterate through every User and look at that User's Questions,

Rather, we should start with separate "tables" in our database:

std::vector< User > all_users;
std::vector< Questions > all_questions;

Questions must "belong to" one ( and only one ) User and thus this relationship can be achieved with a simple reference. This User must keep track of his/her Questions, and for this we can use std::reference_wrapper ( found in <functional> ).

class Question
{
  public:
  Question( User& user ) : user( user ){  user.ask( *this ); }

  protected:
  User& user;
};

class User
{
  public:
  void ask( Question& question )
  {
    questions.push_back( question );
  }

  protected:
  std::vector< std::reference_wrapper< Question > > questions;
};

This set up requires that one be careful when "deleting" elements, especially when a User deletes his/her account:

~Question( void )
{
  user.unAsk( *this );
}

~User( void )
{
  for ( auto& question: questions )
    question.invalidate();
}

As for const correctness, indeed, it requires no change of function name ( or copying ):

const std::vector< std::reference_wrapper<Question> >& User::getQuestions( void ) const
{
  return questions;
}

std::vector< std::reference_wrapper<Question> >& User::getQuestions( void )
{
  return questions;
}

Somewhere in our code:

void function( const User& user )
{
  user.getQuestions()[0].get().text = "edit question"; // compile error
}

void anotherFunction( User& user )
{
  user.getQuestions()[0].get().text = "edit question"; // just fine
}

Answer 3

I am going to bring up couple of issues that have not been addressed by the current answers.

1. Your use of the typedef

   typedef std::vector<std::shared_ptr<const Component>> const componentCollectionConstT;
   

   doesn't buy you anything. You use this in the function.

   componentCollectionConstT Model::getComponentsReadOnly() const {
       componentCollectionConstT componentsC(components.begin(), components.end());
       return componentsC;
   }
   

   You are returning a temporary object that you created on the stack. Whether that object is const or not does not matter. You are adding unnecessary complexity to the code.

2. You are returning an std:vector by value in Model::getComponents().

   componentCollectionT Model::getComponents() {
       return components;
   }
   

   The user of this function is forced to make a copy of the std::vector. You can change the return type of the function to be a const reference. This will allow the user of the function the choice of not paying the price of making a copy of the std::vector if they don't need to. While at it, make the function a const member function too.

   componentCollectionT const& Model::getComponents() const {
       return components;
   }
   

   After you make this change, you won't need the first function at all.

Couple of stylistic suggestions.

1. When defining types, whether they are typedefed or actual type, the naming convention should be same. In that vein, ComponentCollection seems a better typedef than componentCollectionT.

2. Adding const to a typedef makes sense only if you somehow include const in the typedef.

   I think you should either

    typedef std::vector<std::shared_ptr<Component>> const ConstComponentCollection;
   

   or

   typedef std::vector<std::shared_ptr<Component>> ComponentCollection;
   

Answer 4

I will comment here two specific qualities that the OP listed as desirable for his code:

• Children only created through owning parent so that they are managed by parent.
• std::shared_ptr and related utilities used for memory management.

If children are 'managed' by their parent, then destroying the parent should destroy the children, IMHO. But returning (a vector of) shared_ptrs allows the callers of getComponents() to store these shared_ptrs. Children will then only be destroyed when all references are gone. After all, shared_ptr is really about, well, shared ownership...

My other point will be that "using shared_ptr and related utilities" should not be a goal in itself. It's not that this is not a good idea. It's that these are tools, and the best possible tools should be used. In this case, I question their usefulness... Assuming again that the Parent fully owns its Children, it can use vectors of raw pointers, rather than vectors of shared_ptrs. shared_ptrs are much more expensive in terms of CPU and memory than raw pointers. The only drawback of raw pointers is that the destructor of parent must explicitly destroy all its children: the destructor of a raw pointer doesn't do much... ;-)

And this probably means that the best tool for the job may be std::unique_ptr... But I'll study this question later...