0

I know that similar questions might have been asked before, but I am looking for an alternatvie solution to using IS, as I have read that IS is considered evil in the top-comment here.

So basically my Problem is that I have Abilities and Objects (Object A and Object B) that can use said abilities. However Object A lacks some data/information meaning it can only use some types of abilities (Ability A), while Object B has should be able to use all Abilities of type A and in addition some of type B.

For now I solved this the following way:

public abstract class BaseEffect
{
}

public abstract class GeneralEffect : BaseEffect
{
    public abstract void Trigger(int damage, float factor);
}

public class GeneralEffectTest : GeneralEffect
{
    public override void Trigger(int damage, float factor)
    {
        Console.WriteLine("Deal " + damage * factor + " damage");
    }
}

public abstract class PlayerEffect : BaseEffect
{
    public abstract void Trigger(int damage, string text, float factor);
}

public class PlayerEffectTest : PlayerEffect
{
    public override void Trigger(int damage, string playerOnlyInfo, float factor)
    {
        Console.WriteLine("Deal " + damage * factor + " " + text + "-damage");
    }
}

And basically a class that would be able to use both types of effects is the following:

public class TestClass
{
    public BaseEffect effect;

    public TestClass()
    {
        effect = new PlayerEffectTest();
    }

    public void Trigger()
    {
        if (effect is GeneralEffect)
        {
            ((GeneralEffect)effect).Trigger(2, 0.5f);
        }
        else if (effect is PlayerEffect)
        {
            ((PlayerEffect)effect).Trigger(2, "Fire", 0.2f);
        }
    }
}

A class that would be able to use just GeneralEffects would on the other hand just have a field defining a GeneralEffect.

I should maybe also note that for my actual use-case I would iterate through a List/Array of Effects.

Is there any way to avoid the "evil" IS here? How would you solve that issue?

Courier
  • 71
  • 6
  • 1
    How about making the parameters of all the effects instance properties? That way all the effects can have a parameterless `Trigger` method. – Sweeper Aug 31 '22 at 09:19
  • You might want to take a look at the visitor pattern: https://refactoring.guru/design-patterns/visitor – Natrium Aug 31 '22 at 09:20
  • 2
    Why do you assume `is` is evil? If you used `if (effect is GeneralEffect ge)` you could write `ge.Trigger()` in the branch. This uses `is` as a pattern matching operation, not a type checker. – Panagiotis Kanavos Aug 31 '22 at 09:20
  • 1
    Do look at the visitor pattern and how much code it requires. Then look at pattern matching and how cleaner it is – Panagiotis Kanavos Aug 31 '22 at 09:21
  • Is it important that your usage example uses different damage values (`0.5f` and `0.2f`) for the calls? (If that was a mistake and you meant to write the same value, I can propose a solution.) – Heinzi Aug 31 '22 at 09:25
  • @Heinzi It is more important that the string playerOnlyInfo does not exist in one of those functions. For the actual use-case: Think Card Game Abilities (Slay the Spire). Some abilities are relevant to players and enemies alike (i.e. Dealing damage). Other Abilities are only relevant to the player. (Only a player has a hand of cards -> Only player should be able to use Abilities that discard a hand card). – Courier Aug 31 '22 at 09:34
  • @PanagiotisKanavos: Mostly because I have seen dislike for it in the linked thread and another thread. – Courier Aug 31 '22 at 09:35
  • That discussion is marked as a duplicate of [this one](https://stackoverflow.com/questions/496096/casting-vs-using-the-as-keyword-in-the-clr) where the recent top-voted answer explains that `is` in pattern matching isn't bad, it's `is` for lazy type checking that's bad – Panagiotis Kanavos Aug 31 '22 at 09:41
  • @PanagiotisKanavos Oh thank you very much that seems interesting. So from your comments and the answer I guess I wasn't on that bad of a track, but I could take a look into pattern matching to improve this further? – Courier Aug 31 '22 at 09:44
  • 1
    If you can create a base method and override it, or overload a method based on type, you shouldn't need `is` or casting. There are complex cases like yours that can't be solved with overriding or overloading it. Before pattern matching the Visitor pattern was one way to customize behavior per type without forcing callers to know about every possible type. That knowledge was moved to the visitor. Unfortunately, all visitors had to handle all types, whether they cared or not, resulting in a different type of complexity – Panagiotis Kanavos Aug 31 '22 at 09:46
  • @PanagiotisKanavos Yeah exactly that was my problem here. I wasn't able to solve this using basic inheritance/override/overload, since the 2 method signatures(?is that the right word?) differed. – Courier Aug 31 '22 at 09:54
  • @Heinzi Why did you delete your answer, I think it's a good one? – Charlieface Aug 31 '22 at 19:10
  • @Charlieface: I overlooked that OP uses different damage values for GeneralEffect and PlayerEffect. My solution does not take that into account. – Heinzi Aug 31 '22 at 20:06

1 Answers1

5

is isn't evil. Everything depends on its usage. In fact, is can be used since C# 7 in pattern matching which would allow you to write :

if (effect is GeneralEffect ge)
{
    ge.Trigger(2, 0.5f);
}
else if (effect is PlayerEffect pe)
{
    pe.Trigger(2, "Fire", 0.2f);
}

An even better option would be to use pattern matching with switch :

switch(effect)
{
    case PlayerEffect pe:
        pe.Trigger(2, "Fire", 0.2f);
        break;
    case GeneralEffect ge:
        ge.Trigger(2, 0.5f);
        break;
}

Pattern matching is far more powerful than this, as you can match on properties, not just types. It's also possible to add guard clauses:

switch(effect)
{
    case PlayerEffect pe when player.Ammo>0:
        pe.Trigger(2, "Fire", 0.2f);
        break;
    case PlayerEffect pe:
        pe.Trigger(0, "Click", 0.0f);
        break;
    case GeneralEffect ge:
        ge.Trigger(2, 0.5f);
        break;
}
Panagiotis Kanavos
  • 120,703
  • 13
  • 188
  • 236