CODEWITHSUNDEEP
c++shared-ptrc++03
Enaid
Enaid

Reputation: 305

Is it good practice to store copies of the same shared pointers in different vectors?

I have a base class, BaseObject, and two derived class DerivedObject1 and DerivedObject2. They share a common behavior and methods, but DerivedObject1 has an additional method. My main class MyClass stores (in std::vector) boost::shared_ptr of instances of those classes. MyClass needs to call commonMethod() for all the BaseObject, and sometimes call additionalMethod() for all DerivedObject1. 

class BaseObject
{
  virtual void commonMethod();
}

Class DerivedObject1 : public BaseObject
{
  void commonMethod();
  void additionalMethod();
}

Class DerivedObject2 : public BaseObject
{
  void commonMethod();
}

Are there any disadvantages of having two vectors in MyClass, one that stores ALL the pointers of DerivedObject1 and DerivedObject2, and another vector that stores only the pointers of DerivedObject1 ? Meaning I would have all the DerivedObject1 pointers twice. But I think the call to the different methods would be clear, at least.

class MyClass
{
  typedef std::vector<std::shared_ptr<BaseObject>> BaseObjectVector;
  typedef std::vector<std::shared_ptr<DerivedObject1>> DerivedObject1Vector;
  BaseObjectVector everything;
  DerivedObject1Vector only_derived1;

  void doSomething()
  {
    for (BaseObjectVector::iterator iter = everything.begin(); iter != everything.end(); ++iter)
    {
      (*iter)->commonMethod();
    }
  }

  void doSomethingForDerivedObject1()
  {
    for (DerivedObject1Vector::iterator iter = only_derived1.begin(); iter != only_derived1.end(); ++iter)
    {
      (*iter)->additionalMethod();
    }
  }
}

I can think of other ways to do this, mainly having one vector for DerivedObject1 and one vector for DerivedObject2, but to call commonMethod(), I would have to iterate over both vectors. My original solution seems the best to me, except that some pointers are stored twice. What are the disadvantages of this ?

Upvotes: 7

Views: 427

Answers (3)

Hiroki
Hiroki

Reputation: 2880

Interesting question. We sometimes meet such a situation in maintenance of legacy codes which we don’t know who wrote.

Are there any disadvantages of having two vectors in MyClass … ?

I think there are no mechanical (or performance) disadvantages. If we are struggled to meet the release deadline, we have no choice but to pick such an simple way. However, storing same vectors twice actually lowers maintainability and we should consider improving it in the future.

  • std::dynamic_pointer_cast (or boost::dynamic_pointer_cast) [Demo]

If you need dynamic_pointer_cast to implement functions like @Caleth ’s push_back / remove, how about removing only_derived1 and reluctantly applying just one dynamic_pointer_cast in doSomethingForDerivedObject1() from the get-go as follows ? It will make MyClass simpler. Required modification will not be complicated if DerivedObject3 is defined in the future.

void MyClass::doSomethingForDerivedObject1()
{
    for (const auto& obj_i : everything)
    {
      if (auto derived1 = std::dynamic_pointer_cast<DerivedObject1>(obj_i))
        {
           derived1->additionalMethod();
        }
    }
}

void MyClass::doSomethingForDerivedObject3()
{
    for (const auto& obj_i : everything)
    {
      if (auto derived3 = std::dynamic_pointer_cast<DerivedObject3>(obj_i))
        {
           derived3->additionalMethod();
        }
    }
}

Declaring virtual function BaseObject::additionalMethod() and implementing

void DerivedObject2::additionalMethod()
{ 
  /* nothing to do */
}

then you can again remove only_derived1. In this method you must implement DerivedObject3::additionalMethod() only if DerivedObject3 is defined.

But, although it depends on your constructor or setter code, if the following case will also occur

everything;    ->derived2
only_derived1; ->derived1

this method is still insufficient.

Ideally, we should not use public inheritance to implement objects within "IS-ALMOST-A“ relations, as Herb Sutter says. The relation between BaseObject, DerivedObject1 and DerivedObject2 looks like this one. Since I don’t know whole code of your application I may be wrong, but it will be worthwhile to consider extracting DerivedObject1::additionalMethod() as another class or a function pointer and putting its vector in MyClass as a private member.

Upvotes: 6

Caleth
Caleth

Reputation: 62719

Yes, your first method is good, the main problem is that you end up duplicating the public members of vector to ensure that the consistency of the Derived vector.

class MyClass
{
    typedef std::vector<std::shared_ptr<BaseObject>> BaseObjectVector;
    typedef std::vector<std::shared_ptr<DerivedObject1>> DerivedObject1Vector;
    BaseObjectVector everything;
    DerivedObject1Vector only_derived1;
public:
    void push_back(shared_ptr<Base> ptr)
    {
        everything.push_back(ptr);
        if (shared_ptr<Derived1> derived = dynamic_ptr_cast<Derived1>(ptr))
        {
            only_derived1.push_back(derived);
        }
    }
    void remove(shared_ptr<Base> ptr)
    {
        base.remove(ptr);
        only_derived1.remove(dynamic_ptr_cast<Derived1>(ptr));
    }
    // dozens more... 
};

What you can instead do is provide a view of your bases using something like boost::range's adaptors

shared_ptr<Derived1> convert(shared_ptr<Base> ptr)
{
    return dynamic_ptr_cast<Derived1>(ptr);
}

bool not_null(shared_ptr<Derived1> ptr)
{
    return ptr.get();
}

boost::for_each(bases 
              | boost::adaptors::transformed(convert) // Base to Derived
              | boost::adaptors::filtered(not_null)   // Eliminate null
              | boost::adaptors::indirected,          // dereference
                boost::bind(&Derived1::additionalMethod, boost::placeholders::_1));

Upvotes: 0

sagi
sagi

Reputation: 40481

I can suggest this : Store everything in one array , and make a dummy additionalMethod() in DerivedObject2 . Then - just invoke additionalMethod for every object .

Alternativly :

They share a common behavior and methods, but DerivedObject1 has an additional method

Make DerivedObject1 inherit from DerivedObject2

Upvotes: 1

Related Questions