CODEWITHSUNDEEP
c++
turoni
turoni

Reputation: 1435

Avoid almost identical code duplication in double for loop with different iterators

I must construct a graph from 2 different custom types that I cannot change.
Because their signatures and purpose are almost the same I have some code duplication I would like to remove.
Problem is they return a different iterator.

for (auto itFrom = m_pModelSpace->newIterator(); !itFrom->done() && nodesPassed < maxNodes; itFrom->step(), ++nodesPassed)
{
    /*
    ...
    */
    for (auto itTo = m_pModelSpace->newIterator(); itTo->objectId() != itFrom->objectId(); itTo->step())
    {
        /*
        ...
        */
    }
}

for (auto itFrom = m_pSelectionSet->newIterator(); !itFrom->done() && nodesPassed < maxNodes; itFrom->next(), ++nodesPassed)
{
    /*
    ...
    */
    for (auto itTo = m_pSelectionSet->newIterator(); itTo->objectId() != itFrom->objectId(); itTo->next())
    {
        /*
        ...
        */
    }
}

The comments are exactly the same code.
Is there any way to work around this?
I was thinking about a wrapper with 2 constructors for each type but then I also need a wrapper for the object iterator.
Another idea was multiple inheritance of the 2 types but that also doesn't really feel right.

Any advice?

Summurarized solution
Thanks to Mark B I have come to the following solution: 

namespace {
OdDbObjectIteratorPtr get_iterator(OdSmartPtr<OdDbBlockTableRecord>& pEntityCollection)
{
    return pEntityCollection->newIterator();
}

OdDbSelectionSetIteratorPtr get_iterator(OdSmartPtr<OdSelectionSet>& pEntityCollection)
{
    return pEntityCollection->newIterator();
}

void increment_iterator(OdDbObjectIteratorPtr& iter)
{
    iter->step();
}
void increment_iterator(OdDbSelectionSetIteratorPtr& iter)
{
    iter->next();
}

}

namespace spax{
template <typename Collection>
void ConnectionGraph::ConstructGraph(Collection& pEntityCollection, int maxNodes)
{
    // ...
    for (auto itFrom = get_iterator(pEntityCollection); !itFrom->done() && nodesPassed < maxNodes; increment_iterator(itFrom), ++nodesPassed)
    {
        //...
        for (auto itTo = get_iterator(pEntityCollection); itTo->objectId() != itFrom->objectId(); increment_iterator(itTo))
        {
            //...
        }
    }
}
}

Because the Selection set returned a parent OdSelectionSetIteratorPtr of OdDbSelectionSetIteratorPtr I added another function inspired by this solution to get the iterators. Thanks for the help, I'm really pleased with the outcome.

Upvotes: 2

Views: 158

Answers (4)

Slava
Slava

Reputation: 44268

You can write a function with m_pModelSpace as parameter. In case you have to pass to match that is used in that code from local context use lambda:

auto advance = []( auto &it ) { it->step() };
auto loop = [&]( auto p ) {
     for (auto itFrom = p->newIterator(); !itFrom->done() && nodesPassed < maxNodes; advance( itFrom ), ++nodesPassed)
     {
        /*
         ...
        */
           for (auto itTo = p->newIterator(); itTo->objectId() != itFrom->objectId(); advance( itTo ) )
           {
               /*
                ...
               */
           }
     }
};
loop( m_pModelSpace );
advance = []( auto &it ) { it->next() };
loop( m_pSelectionSet );

Note: I changed lamdas parameter type to auto as you said that container types are unrelated. This would work for c++14 or later, otherwise you can use template.

Upvotes: 2

Mark B
Mark B

Reputation: 96271

What about using a template function with a level of indirection to handle the iterator increment?

void increment_iterator(<model space iterator type>& iter) { iter->step(); }
void increment_iterator(<selection set iterator type>& iter) { iter->next(); }

template <typename Container>
void execute_nested_loop(Container* c)
{
    for (auto itFrom = c->newIterator(); !itFrom->done() && nodesPassed < maxNodes; increment_iterator(itFrom), ++nodesPassed)
    {
        /*
        ...
        */
        for (auto itTo = c->newIterator(); itTo->objectId() != itFrom->objectId(); increment_iterator(itTo))
        {
            /*
            ...
            */
        }
    }
}

Then call it:

execute_nested_loop(m_pModelSpace);
execute_nested_loop(m_pSelectionSet);

Upvotes: 4

Peter
Peter

Reputation: 5728

In C++, normalize the interfaces (get rid of step() vs next()), then use a template function.

Upvotes: 1

Loay Ashmawy
Loay Ashmawy

Reputation: 687

Create a function that takes as entry the parameters that you use and where you put your identical code and call it in each loop and

Upvotes: 1

Related Questions