Type Storage Class

Question

So I have designed a "type store" in C++ that can hold a single instance of any subclass of a defined type.

Here is an example of how it would be used:

class BaseClass { };
class FirstSubClass : public BaseClass { };
class SecondSubClass : public BaseClass { };

TypeStore<BaseClass> store;

// Each of these returns a pointer to the newly created instance
store.Create<BaseClass>();
store.Create<FirstSubClass>();
store.Create<SecondSubClass>();

// Each of these fails (returning nullptr) as an instance already exists for the type
store.Create<BaseClass>();
store.Create<FirstSubClass>();
store.Create<SecondSubClass>();

// Returns pointers to the instances for each type or nullptr if it is not present
store.Get<BaseClass>();
store.Get<FirstSubClass>();
store>Get<SecondSubClass>();

I have designed the class in a way that it handles all memory allocation and deallocation, making it easier to use.

If you are wondering what I would use this for, I am currently developing an Entity Component System that would use this for storing components; there can only be a single instance of each component type per entity.

There is one thing I don't really like about my implementation, and that is how I am forced to write the body of my methods in the header. I know this is required because I have a templated class, and I could extract them out into another file to be included by type_store.h, however I still feel this is a rather messy solution.

And here is the source:

identifier.h

#ifndef IDENTIFIER_H
#define IDENTIFIER_H

#include <stddef.h>

typedef size_t Identifier;

#endif

type_identifier.h

#ifndef TYPE_IDENTIFIER_H
#define TYPE_IDENTIFIER_H

class TypeIdentifier {
public:
    template<typename T>
    static Identifier GetIdentifier() {
        static Identifier identifier = nextIdentifier++;

        return identifier;
    }

private:
    static Identifier nextIdentifier;
};

#endif

type_identifier.cpp

#include "type_identifier.h"

Identifier TypeIdentifier::nextIdentifier = 0;

type_store.h

#ifndef TYPE_STORE_H
#define TYPE_STORE_H

#include <map>

template<typename T>
class TypeStore {
public:
    typedef std::map<Identifier, T *> Map;

    typedef typename Map::iterator Iterator;
    typedef typename Map::const_iterator ConstIterator;

    ~TypeStore() {
        for (auto iterator = Begin(); iterator != End(); /* unused */) {
            delete iterator->second;

            store.erase(iterator++);
        }
    }

    template<typename V>
    bool Contains() {
        static_assert(std::is_base_of<T, V>::value, "incorrect type for container");

        return store.find(TypeIdentifier::GetIdentifier<T>()) != store.end();
    }

    template<typename V>
    V *Get() {
        static_assert(std::is_base_of<T, V>::value, "incorrect type for container");

        if (!Contains<V>()) {
            return nullptr;
        }

        Identifier identifier = TypeIdentifier::GetIdentifier<T>();

        return static_cast<V *>(store.at(identifier));
    }

    template<typename V, typename ...A>
    V *Create(A &&... args) {
        static_assert(std::is_base_of<T, V>::value, "incorrect type for container");

        if (Contains<V>()) {
            return nullptr;
        }

        Identifier identifier = TypeIdentifier::GetIdentifier<T>();
        V *instance = new V(std::forward(args)...);

        if (!store.insert(std::make_pair(identifier, instance)).second){
            delete instance;

            return nullptr;
        }

        return instance;
    }

    template<typename V>
    bool Remove() {
        static_assert(std::is_base_of<T, V>::value, "incorrect type for container");

        Identifier identifier = TypeIdentifier::GetIdentifier<T>();

        delete Get<V>();

        return store.erase(identifier) == 1;
    }

    typename Map::size_type Size() {
        return store.size();
    }

    Iterator Begin() {
        return store.begin();
    }

    Iterator End() {
        return store.end();
    }

    ConstIterator Begin() const {
        return store.begin();
    }

    ConstIterator End() const {
        return store.end();
    }

    ConstIterator CBegin() const {
        return store.cbegin();
    }

    ConstIterator CEnd() const {
        return store.cend();
    }
private:
    Map store;
};

#endif

Answer 1

unique_ptr:

You might consider using at least some automated memory management internally. If the TypeStore will retain ownership of all objects it creates, instead of this:

typedef std::map<Identifier, T *> Map;

~TypeStore() {
    for (auto iterator = Begin(); iterator != End(); /* unused */) {
        delete iterator->second;
        store.erase(iterator++);
    }
}

You could simply store a unique_ptr in the map and completely remove the TypeStore destructor:

typedef std::map<Identifier, std::unique_ptr<T>> Map;

The deletes you have scattered around would also go away. A much cleaner solution, IMO.

Use of null as an error indicator:

You chose to return nullptr to indicate errors. Unfortunately that precludes returning a reference to the object. A reference would be better because it more clearly conveys the notion that the object is owned by TypeStore, so there'd be no risk of accidentally attempting to delete a pointer returned by the store.

You might consider other ways of indicating the errors instead, so you could return references. One might be exceptions, but that's not a great strategy if the TypeStore is expected to get invalid requests often. The other strategy which would also be a great fit in this case is an optional type. Sadly there's currently no such type in the Standard, but the next best thing is Boost.Optional.

In any case, this is more of a suggestion, if you're comfortable with your current design, by all means, keep it. Just be sure to document clearly that the pointers returned are not to be deleted outside TypeStore.