2

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.

  • 1
    The first method keeps things simple, but requires lots of empty method calls. The latter is more complex, requires a cast, but avoids empty method calls. Both approaches are equally valid and both are common approaches. Go with the one that fits your scenario best. There's no one right answer for this one. – David Arno Dec 11 '19 at 10:30
  • 1
    I somewhat disagree with David. #1 is lying the developer telling that all the modules are updatable, but under the hood, some are not. Treating all the modules as they were all the same when they are not, just makes hard to reach determinism and consistency, which IMO are two important property any code should aspire to have. Unlike #1, option #2 is pure and genuine Interface Segregation which comes to solve your problem. It also gets rid of the extra cognitive burden of does this module, in this loop, at this iteration, is updatable? and throwable? Or just one? None? – Laiv Dec 11 '19 at 11:30
  • A third option would be to maintain 3 lists, each at the appropriate interface level. If you're calling these methods significantly more than you're adding or removing items from the list the overhead of maintaining multiple lists may be less than the overhead of casting or calling empty methods. – combinatorics Dec 11 '19 at 11:51
  • Thanks for opinions, it helps a lot! I felt like I'm inventing hideous constructs instead of something simple, and now it seems like my solutions are valid after all. – Xamtouo Dec 11 '19 at 12:26
  • 3
    There is some LINQ support for option 2 that makes the code a little cleaner: modules.OfType<IUpdateable>().ForEach(m => m.Update()); will work I think. I suppose this is also provides evidence that filtering a collection on type is fairly common, since they've built it into LINQ. – combinatorics Dec 11 '19 at 13:25
2

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.

  • I suspect Unity hiring Mike Acton to work on their ECS work means that they're embracing a data-oriented approach for performance reasons. I suspect will involve using SoA to ensure the processor's cache is tightly packed? (Very possibly wrong, game programming isn't really my area of expertise.) – combinatorics Dec 13 '19 at 17:42
  • Thanks for detailed breakdown! In fact, I was inspired by Unity's component system. I didn't mention one thing in my original post: I may add several new modules/interfaces throughout development. Maintaining separate lists of interfaces feels like violation of Open/Closed principle - I'll have to add not only one additional upcast, but a whole new list inside my object and populate it. So I decided to stick with upcasting for now and test separate lists later, when my system will be up and running (it may end up the best solution considering potential performance gains). – Xamtouo Dec 16 '19 at 7:58
  • @Xatmo: Addition != modification. When the codebase expands, it's only normal that those new types get referenced in places they didn't before. You can write this without violating OCP by having an extended class for each interface type, thus essentially separating the module lists into objects of their own (which do the same thing as before). – Flater Dec 16 '19 at 8:53
1

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();
    }
}
1

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.

1

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());
    }
}
  • Thanks for advice! OfType is nice thing to know, but LINQ call will create lots of allocations if called frequently, so it isn't an option in my particular case. – Xamtouo Dec 16 '19 at 9:12
  • OfType<> doesn't allocate anything (source). What are you referring to? – John Wu Dec 16 '19 at 9:19
  • OfType<> is using foreach loop, which (in some cases) leads to allocations. Though I see that most of the issue was optimized already, which means it's another thing that I should test myself. – Xamtouo Dec 16 '19 at 9:40
  • Really? You've avoiding foreach as an optimization? – John Wu Dec 17 '19 at 20:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.