5
\$\begingroup\$

I have created a pretty simple IOC container for C++ that should allow me to write less coupled code in the future and make my code easier to unit test.

Overall I am fairly happy however I feel like I could probably include some additional functionality to improve the code further.

Any suggestions on how the code could be improved or any noticeable issues are much appreciated.

In the real production code the implementation will be namespaced etc. but I left it out for sake of example. I understand that singleton pattern is generally frowned upon however I do think it serves a purpose here.

Worth noting I cannot use std::unique_ptr due to real implementation using boost instead of c++11.

Implementation(IOC.hpp)

class IOCContainer
{
    public:
        static IOCContainer& Instance()
        {
            static IOCContainer instance;
            return instance;
        }

        ///
        /// Pass in a shared pointer of class T and we will automatically deduce the type using the typeid of T
        ///
        template<class T>
        void Register(std::shared_ptr<T> t)
        {
            const std::type_info* typeId = &typeid(T);

            Register(typeId->raw_name(), t);
        }

        ///
        /// Pass in a shared pointer as well as a type id. This is useful for registering a base class/interface
        /// alongside a derived class shared pointer i.e. pass in &typeid(Base) and boost::shared_ptr<Derived>
        ///
        template<class T>
        void Register(const std::type_info* typeId, std::shared_ptr<T> t)
        {
            if (!typeId)
            {
                throw std::runtime_error("Invalid type id to register");
            }

            Register(typeId->raw_name(), t);
        }

        ///
        /// Pass in a shared pointer as well as a custom id. This can be useful if we wish to use an interface more than once
        /// with different base classes
        ///
        template<class T>
        void Register(const std::string& id, std::shared_ptr<T> t)
        {
            std::lock_guard<std::mutex> lock(m_mapMutex);

            std::map<std::string, std::shared_ptr<void> >::iterator iter = m_mapping.find(id);

            if (iter == m_mapping.end())
            {
                m_mapping[id] = t;
            }
        }

        ///
        /// Resolves the pointer type based on the type id of the supplied T
        /// 
        template<class T>
        std::shared_ptr<T> Resolve()
        {
            const std::type_info* typeId = &typeid(T);

            return Resolve<T>(typeId->raw_name());
        }

        ///
        /// Resolves the pointer type based on the supplied type id
        /// 
        template<class T>
        std::shared_ptr<T> Resolve(const std::type_info* typeId)
        {
            if (!typeId)
            {
                throw std::runtime_error("Invalid type id to resolve");
            }

            return Resolve<T>(typeId->raw_name());
        }

        ///
        /// Resolves the type based on the supplied ID
        ///
        template<class T>
        std::shared_ptr<T> Resolve(const std::string& id)
        {
            std::lock_guard<std::mutex> lock(m_mapMutex);

            std::map<std::string, std::shared_ptr<void> >::iterator iter = m_mapping.find(id);

            if (iter != m_mapping.end())
            {
                return std::static_pointer_cast<T>(iter->second);
            }

            throw std::runtime_error("Could not locate type in IOC");
        }
    private:
        std::map<std::string, std::shared_ptr<void> > m_mapping;
        std::mutex m_mapMutex;

        // Disable construction
        IOCContainer(){}
        // Disallow copying of object to ensure we don't get accidental copies of our IOCContainer
        IOCContainer(const IOCContainer&);
        void operator=(const IOCContainer&);
    };

Test Code

#include "IOC.hpp"

#include <iostream>
#include <string>
#include <memory>

class A
{
public:
    A(std::string stuff) : m_stuff(stuff){}
    virtual ~A(){}
    virtual std::string GetStuff() const { return m_stuff; }
private:
    std::string m_stuff;
};

class C : public A
{
public:
    C(std::string stuff) : A(stuff)
    {

    }
};

class B
{
public:
    B() : 
        m_default(IOCContainer::Instance().Resolve<C>()),
        m_baseTest(IOCContainer::Instance().Resolve<A>()),
        m_baseTest2(IOCContainer::Instance().Resolve<A>(&typeid(A))),
        m_idTest1(IOCContainer::Instance().Resolve<A>("IdTest")),
        m_idTest2(IOCContainer::Instance().Resolve<A>("IdTest2"))
    {
        std::cout << m_default->GetStuff() << std::endl;
        std::cout << m_baseTest->GetStuff() << std::endl;
        std::cout << m_baseTest2->GetStuff() << std::endl;
        std::cout << m_idTest1->GetStuff() << std::endl;
        std::cout << m_idTest2->GetStuff() << std::endl;
    }
private:
    std::shared_ptr<C> m_default;
    std::shared_ptr<A> m_baseTest;
    std::shared_ptr<A> m_baseTest2;
    std::shared_ptr<A> m_idTest1;
    std::shared_ptr<A> m_idTest2;
};

int main()
{
    IOCContainer::Instance().Register(std::make_shared<C>("DefaultTest"));
    IOCContainer::Instance().Register(&typeid(A), std::make_shared<C>("BaseTest"));
    IOCContainer::Instance().Register("IdTest", std::make_shared<C>("IdTest"));
    IOCContainer::Instance().Register("IdTest2", std::make_shared<C>("IdTest2"));

    B b;
}
\$\endgroup\$
  • \$\begingroup\$ This is pretty cool \$\endgroup\$ – Valdrinium Oct 30 '18 at 6:54
3
\$\begingroup\$

Just some ideas about implementation.

disabling methods

You might consider deleted functions (=delete) feature of C++11.

http://en.cppreference.com/w/cpp/language/function#Deleted_functions

    // Disable construction
    IOCContainer() = delete;
    // Disallow copying of object to ensure we don't get accidental copies of our IOCContainer
    IOCContainer(const IOCContainer&) = delete;
    void operator=(const IOCContainer&) = delete;

std::type_info::name

I would be a little careful with this.

http://www.cplusplus.com/reference/typeinfo/type_info/name/

The particular representation pointed by the returned value is implementation-defined, and may or may not be different for different types.

Standard does not ask compilers to produce different names for different types so it is implementation defined. Compilers are probably going to implement it in a sensible manner.

pointer error handling

You might consider changing pointer to reference as you don't need to think about null_ptr here

    ///
    /// Pass in a shared pointer as well as a type id. This is useful for registering a base class/interface
    /// alongside a derived class shared pointer i.e. pass in typeid(Base) and boost::shared_ptr<Derived>
    ///
    template<class T>
    void Register(const std::type_info& typeId, std::shared_ptr<T> t)
    {
        Register(typeId.raw_name(), t);
    }

and here

    ///
    /// Resolves the pointer type based on the supplied type id
    /// 
    template<class T>
    std::shared_ptr<T> Resolve(const std::type_info& typeId)
    {
        return Resolve<T>(typeId->raw_name());
    }

duplicate registration

Is it intentional that in case specific type has already been registered you silently do nothing during second registration? I would consider throwing some exception.

    template<class T>
    void Register(const std::string& id, std::shared_ptr<T> t)
    {
        std::lock_guard<std::mutex> lock(m_mapMutex);

        std::map<std::string, std::shared_ptr<void> >::iterator iter = m_mapping.find(id);

        if (iter == m_mapping.end())
        {
            m_mapping[id] = t;
        }
    }

synchronization

You are probably locking too much. Simultaneous lookup is definitely not a problem. You might like some ideas about readers/writers locks or shared mutexes.

See e. g. these questions:

\$\endgroup\$
  • \$\begingroup\$ Hi, thanks for the feedback. Unfortunately I cannot use delete since I do not have c++11 support on all compilers(I have used boost::shared_ptr etc.) instead. Changing to ref instead of pointer for type_info is a good idea, the design has changed since original implementation where I used to cache the typeinfo instead of the string for the key, that is why it is still a pointer. Duplicate registration should not occur if I do the find in the map first, no? And yes you are correct I can remove the mutex from the read for sure. \$\endgroup\$ – James Moore Jun 23 '16 at 11:03
  • \$\begingroup\$ wrt duplicate registration, I should have an exception throw probably and instead do an insert. Thanks \$\endgroup\$ – James Moore Jun 23 '16 at 11:11

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.