CODEWITHSUNDEEP
c++dictionaryiteratorstd-pair
Gustavo N
Gustavo N

Reputation: 23

Getting a field from an STL map iterator

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?

Upvotes: 2

Views: 1441

Answers (4)

aroyer
aroyer

Reputation: 111

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...
    }
}

Upvotes: 0

supertopi
supertopi

Reputation: 3488

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.

  • it's easy to add a new field in the mix
  • it's easy to access struct fields ObjType and ObjPair
  • it's easy to write getters/setters/other functions for a class that handle ObjType and ObjPair

Upvotes: 4

Mark Ransom
Mark Ransom

Reputation: 308530

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) { // ...

Upvotes: 0

Kerrek SB
Kerrek SB

Reputation: 477640

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; }
}

Upvotes: 2

Related Questions