patterncppMinor
Implementation of a C++ IOC container
Viewed 0 times
implementationioccontainer
Problem
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.
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
void Register(std::shared_ptr 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
///
template
void Register(const std::type_info* typeId, std::shared_ptr 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
void Register(const std::string& id, std::sha
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
void Register(std::shared_ptr 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
///
template
void Register(const std::type_info* typeId, std::shared_ptr 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
void Register(const std::string& id, std::sha
Solution
Just some ideas about implementation.
disabling methods
You might consider deleted functions (
http://en.cppreference.com/w/cpp/language/function#Deleted_functions
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
and here
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.
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:
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
///
template
void Register(const std::type_info& typeId, std::shared_ptr t)
{
Register(typeId.raw_name(), t);
}and here
///
/// Resolves the pointer type based on the supplied type id
///
template
std::shared_ptr Resolve(const std::type_info& typeId)
{
return Resolve(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
void Register(const std::string& id, std::shared_ptr t)
{
std::lock_guard lock(m_mapMutex);
std::map >::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:
- https://stackoverflow.com/questions/16774469/a-rw-lock-for-c11-threads
- https://stackoverflow.com/questions/14306797/c11-equivalent-to-boost-shared-mutex
Code Snippets
// 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;///
/// 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);
}///
/// 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());
}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;
}
}Context
StackExchange Code Review Q#132821, answer score: 3
Revisions (0)
No revisions yet.