Tukukkk

Problem

I ran into a simple issue, though I can't come up with a proper OOD for it.

What I have:

• Base class


• Subclass adding a new method foo()
  


• List of pointers to the base class instances

What I need:

I need to loop through this list and call the foo() for objects supporting this method, i.e. objects of (or derived from) the aforementioned subclass. Or speaking generally, I need a "non-smelly" polymorphic access to a subclass via list of pointers to a base class.

Example Code

class Entity {

    // ...

    // This class contains methods also needed by subclasses.

};



class SaveableEntity : public Entity {

public:

    virtual void save() = 0;

};



// SaveableEntity has multiple subclasses with specific save() implementations.



std::vector<Entity *> list;

for (Entity *entity : list) {

    // Here I need to save() the descendants of a SaveableEntity type.

}

I came up with some ideas, however none of them seem right to me. Here are some of them:

Method 1: dynamic_cast

As some elements are saveable and some are not, the most obvious way I see is dynamic casting:

std::vector<Entity *> list;

for (Entity *entity : list) {

    auto saveable = dynamic_cast<SaveableEntity *>(entity);

    if (saveable) {

        saveable->save();

    }

}

However, using dynamic_cast looks like a bad OOD in this situation (correct me if I'm wrong). Also, this approach can easily lead to the violation of the LSP.

Method 2: Move save() to a base class

I could remove SaveableEntity and move the save() method to the base Entity. However, this makes us implement dummy method:

class Entity {

    virtual void save() {

        // Do nothing, override in subclasses

    }

};

This eliminates the dynamic_cast usage, but the dummy method still doesn't seem right: now the base class holds the information (save() method) totally unrelated to it.

Method 3: Apply design patterns

• 
  Strategy pattern: SaveStrategy class and its subclasses like NoSaveStrategy, SomeSaveStrategy, SomeOtherSaveStrategy, etc. Again, the presence of NoSaveStrategy brings us back to the flaw of the previous method: base class has to know the particular details about its subclass, which seems like a bad design.


• 
  Proxy or Decorator patterns can easily encapsulate the dynamic_cast, however this will only hide the unwanted code, not get rid of the bad design itself.


• Add some composition-over-inheritance layers, and so on and so on...

Question

Maybe I'm missing some obvious solution, or maybe the described methods (1 or 2) are not as bad and smelly in this particular context as I'm seeing them.

So what design approach is suitable in such situation?

There is solution #4 encouraged by data-oriented programming (there's been an excellent talk on it in the cppcon 2018, available on youtube): having two lists. One list is for all the SavableEntitys and the other for Entitys that are not savable.

Now, you iterate over the first list and ->save() those items.

The major advantage is that you only iterate over relevant entities. With some (probably major) refactoring, you could have a collection of objects rather than pointers to some. This would increase data locality and drastically decrease the number of cache misses.

My first idea would have been to augment the base class with a virtual bool trySave() method. The default can be return false;, while SaveableEntity provides this override:

class SaveableEntity : public Entity {

public:

    virtual bool trySave() final override

    {

        save();

        return true;

    }



    // You could also make this protected.

    virtual void save() = 0;

};

Whether this is more appropriate than @YSC's suggestion for your particular case is something that you have to decide for yourself. It's similar to moving save() into the base class but is significantly less confusing for the user.