Iterating through objects with slightly different interfaces

Question

I am constructing modular logic for my object: separate pieces of functionality are going to separate modules. I will have the ability to connect and disconnect modules on demand.

At some point my object will iterate through collection of connected modules:

class MyObject {
    private List<IModule> modules = new List<IModule>();
    void Update()
    {
        modules.ForEach(a=>a.Update());
    }
    void Destroy()
    {
        modules.ForEach(a=>a.Destroy());
    }
}

And here it is becoming interesting. All my modules are Destroyable, but not all of them has to be Updateable. Some of them may have another method, Throw() for example.

1. I may create the following interface:

interface IModule {
    void Update();
    void Destroy();
    void Throw();
}

And inherit all my modules from it. But most of the modules will end up having empty Throw(): only a couple of them require some Throw logic. Then my system works just fine, the only drawback - empty method calls: modules.ForEach(a=>a.Throw());

2. I may create several interfaces:

interface IModule {
    void Destroy();
}
interface IUpdateable : IModule {
    void Update();
}
interface IThrowable : IModule {
    void Throw();
}

In that case my modules end up looking like this:

class SimpleModule: IModule {...}
class ModuleWithUpdate: IUpdateable {...}
class ComplexModule: IUpdateable, IThrowable {...}

class MyObject {
    private List<IModule> modules = new List<IModule>();
    void Update()
    {
        foreach (var module in modules)
            if (module is IUpdateable updateable)
                updateable.Update();    // It will call ModuleWithUpdate and ComplexModule.
    }
}

---

The second option seems preferable, but I can't help but wonder if it is a common approach to the problem. Is there a better way to create a collection of objects, some of which has additional methods, and some of them don't?

I may end up with dozen of modules, and Update will fire 50 to 100 times per second, but I think I covered performance-wise with both approaches. As far as i can tell there are no garbage or expensive operations.

Answer 1

General good practice

Playing the devil's advocate, I can use a good practice argument in either case to explain why it's a bad idea. Essentially:

1. You're violating the interface segregation principle, the I in SOLID.
2. Needing to upcast your objects is polymorphism abuse.

That being said, the second argument is the weaker one here (which I will elaborate on further in this answer). There are valid reasons to consider this abuse though, e.g.:

public void LetThisDuckSwim(Animal a)
{
     if(a is Duck d)
         d.Swim();
}

This is an exaggerated example, but it gets the point across that the method parameter should be of type Duck, not Animal.

While not exactly the same issue, you start seeing an overlap with the classic bad example for the Liskov Substitution Principle:

void MakeDuckSwim(IDuck duck)
{
    if (duck is ElectricDuck educk)
        educk.TurnOn();

    duck.Swim();
}

Other than a minor difference in using ElectricDuck vs IElectricDuck, you basically have the same scenario as what you want to achieve, which is used as a prime example of bad practice.

---

Your scenario

Since you're talking about interfaces where you intentionally refuse to implement part of the interface in certain implementing classes, in order to both avoid LSP and/or ISP violations, the interfaces should be separated.

But, as you are already aware, this does in fact lead you to needing the following upcasting logic (or similar):

void Update()
{
    foreach (var module in modules)
        if (module is IUpdateable updateable)
            updateable.Update();
}

Is there a better way to create a collection of objects, some of which has additional methods, and some of them don't?

You're somewhat asking the wrong question. The core question is whether you should be using that centralized "collection of objects" at all. Consider not putting everything together in one big list:

// 1
private List<IModule> modules = new List<IModule>();

// 2
private List<IUpdateableModule> updateableModules = new List<IUpdateableModule>();
private List<IDestroyableModule> destroyableModules = new List<IDestroyableModule>();
private List<IThrowableModule> throwableModules = new List<IThrowableModule>();

There's a reasonable argument for both cases here. Your question doesn't really shine enough of a light on whether the second approach would be equally viable for you.

There are justifications for the first approach. Unity (the game engine) actually uses components this way. It feels slightly dirty to have to upcast your components before you can use them, but admittedly it makes component handling significantly easier.

However, Unity does provide some query logic to fetch components of the right type, which is something you might want to do. I think this is a reasonable compromise between the two possibilities. LINQ actually already provides this possibility via OfType

private List<IModule> modules = new List<IModule>();

private List<IUpdateableModule> updateableModules => modules.OfType<IUpdateableModule>();

Whether you define these separate properties or use OfType directly is arguable, though I would say that defining the properties would minimize the impact of changing the implementation of how modules are stored in the future.

I may end up with dozen of modules, and Update will fire 50 to 100 times per second, but I think I covered performance-wise with both approaches. As far as i can tell there are no garbage or expensive operations.

Performance is a relevant thing to consider here - though you may want to wait until an actual problem appears.

Assuming the lower end of the spectrum, iterating over 12 modules 50 times per second leads to 600 iterations in total. This is an O(m*n) complexity, so this is sensitive to increases of either m (module count) or n (framerate).

Optimizing this by separating the lists is not as straightforward as you'd initially expect:

• If all modules implement all interfaces, you will lose performance (since you iterate over the module list once per available interface.
• If modules on average implement less than half of the available interfaces, you will gain performance by not needlessly iterating over them (when iterating over the interfaces that a given module doesn't implement).
• If modules on average implement the majority of interfaces, having them on separated lists will actually cause them to be iterated over several times (once per implemented interface), which would be detrimental to performance.

I would suggest leaving the optimizations for when there is an actual problem. It might be interesting to already log this information so you can keep an eye out for any changes to performance (e.g. as the framerate increases or more modules are developed).

Note: I suspect Unity already did the math here and decided to stick with the centralized list de/inspite of performance considerations; but I have not yet found explicit confirmation of my suspicion.

Answer 2

You also can consider third way of doing this. It's extended version of second example without redundant cast but with additional collection.

class MyObject {
    private List<IModule> modules = new List<IModule>();
    private List<IUpdateable> updateableModules = new List<IUpdateable>();
    public void Add(IModule module)
    {
        modules.Add(module);
        if (module is IUpdateable updateable)
            updateableModules.Add(updateable);

    }
    public void Update()
    {
        foreach (var updateable in updateableModules)
                updateable.Update();
    }
}

Answer 3

Whether you choose a single interface or multiple interfaces depends on situation and preference. Both have pres /cons.

It would be possible to just use IModule interface with the Update, Destroy and Throw methods. And then implement an base class with 'empty' versions for those operations.

interface IModule
{
    void Update();
    void Destroy();
    void Throw();
}

class ModuleBase  : IModule
{
    public virtual void Update() { }

    public virtual void Destroy() { }

    public virtual void Throw() { }
}

This way a module can implement any of those on a as-needed basis.

class Module1 : ModuleBase
{
    public override void Update()
    {
        // do module updating required for module1
    }
}

class Module2 : ModuleBase
{
    public override void Destroy()
    {
        // implementation
    }

    public override void Throw()
    {
        // implementation
    }
}

btw. Like ppl pointed out. The second method using seperate interfaces is more pure as the intent becomes more clear.

Answer 4

If the objects support different actions, then yes, you should use separate interfaces (option 2).

To avoid the upcasting problem, just filter the list (e.g. using OfType<T>() for each operation:

class MyObject {
    private List<IModule> modules = new List<IModule>();
    void Update()
    {
        modules.OfType<IUpdateable>().ForEach(a=>a.Update());
    }
    void Destroy()
    {
        modules.OfType<IDestroyable>().ForEach(a=>a.Destroy());
    }
}