I have a map container to store certain objects, together with their name and type:
typedef std::map<std::string, std::pair<ObjType, ObjBase*> > ObjContainer;
However, in many parts of the code, there are constructions like this:
ObjContainer::iterator it = mObjContainer.find(name);
if (it != mObjContainer.end()) {
if (it->second.second) {
it->second.second->setObj2Default();
delete it->second.second;
it->second.second = 0;
}
}
Obviously, the many "it->second.second" are not very clear, and unmaintainable. If it is changed in the future, to support one more field, for example, it will be all broken. So, I am trying to change them by functions to access the fields, like this:
ObjBase*& getObjPtr(ObjContainer::iterator it) {
return it->second.second;
}
Similarly, also function getObjName and getObjType.
It was also suggested to me that it would be more clear to have the iterator returning those fields:
it.objPtr();
it.objName();
it.objType();
But I think that the STL iterators should not be inherited to have those functions, right? I see no other way to do it except to create a wrapper for the map and have its own iterator with those functions.
So, what would be the most appropriate option? Is there any other way to solve this problem that I am not seeing?
If the biggest problem is maintainability, I would replace the std::pair with a custom class/struct that wraps the ObjType and ObjBase* as one.
I'd just make a local copy of the pointer (or reference) -- it'll probably be optimized out anyway:
ObjContainer::iterator const it = mObjContainer.find(name);
if (it != mObjContainer.end())
{
ObjBase * & p = it->second.second;
if (p) { p->foo(); delete p; p = NULL; }
}
Use a reference to simplify the syntax.
ObjContainer::iterator it = mObjContainer.find(name);
if (it != mObjContainer.end()) {
std::pair<ObjType, ObjBase*> & ref = it->second;
if (ref.second) { // ...
I'd first question myself on whether ObjType is mandatory there.
If the aim is just to tell what kind of class is actually pointed by the second ObjBase* parameter of the pair, use dynamic_cast
and get rid of the pair.
typedef std::map<std::string, ObjBase*> ObjContainer;
No more second.second
in the code then:
ObjContainer::iterator it = mObjContainer.find(name);
if (it != mObjContainer.end()) {
if (it->second) {
it->second->setObj2Default();
delete it->second;
it->second = NULL;
}
}
And when you need to test the actual type of the object:
ObjContainer::iterator it = mObjContainer.find(name);
if (it != mObjContainer.end()) {
if (ChildObj* p_Child = dynamic_cast<ChildObj*>(it->second)) {
// Work on p_Child...
}
}