Question

How to refactor this type-dependant code to avoid downcasting?

Background

I am coding in Unity, with C#8.0.

I am making a game where the player battles enemies by playing cards that have different effects (think Slay the Spire). The player selects the card, then selects the card's target, and then (if the target is valid) the card's effect is invoked.

Lots of different game objects can be the target of a card: enemies, the player character, other cards, the turn counter, the sky, etc. For each object that can be a target I have its class implement an interface:

public interface ICardTarget
{
    public bool IsValidTarget(Card targetingCard);
}

Similarly, lots of different game objects can play cards: the player character, enemies, obstacle objects, etc. For each object that can use a card I have its class implement an interface:

public interface ICardUser
{
    //I don't have anything in here, this is really more of just a tag to group together any card users. Am I using interfaces right?
}

So some of the objects in my scene might be:

public class Enemy : ICardTarget, ICardUser
{
    public bool IsValidTarget(Card targetingCard) {/*...*/}

    public string Species {get;set;}
}

public class PlayerCharacter : ICardTarget, ICardUser
{
    public bool IsValidTarget(Card targetingCard) {/*...*/}

    public float AttackSpeed {get;set;}
    public int Health {get;set;}
}

public class Weather : ICardTarget
{
    public bool IsValidTarget(Card targetingCard) {/*...*/}

    public bool IsCloudy {get;set;}
}

public class Boulder: ICardUser
{
    public int Mass {get;set;}
}

etc.

Each card defines which types of targets are valid. When the player (or any card user for that matter) selects an object as the card's target, the card passes itself to the IsValidTarget function and receives a bool response as to whether this is a valid target for this card.

public class Card
{
    private ICardUser _user;     //reference to this card's user, set somewhere else
    private ICardTarget _target; //reference to this card's target, set somewhere else

    public void CheckTarget()
    {
        if (_target.IsValidTarget(this))
        {
            PerformCardEffect();
        }
    }

    private void PerformCardEffect() {/*whatever the card does*/}
}

So far so good! Now the card needs to perform its effect on the targeted object, and also may have an effect on the card's user.

The Problem

Currently, the only thing the card knows about this targeted object is that it is a valid target (that's all that the ICardTarget interface guarantees, after all). So how do I perform the card's effect that depends on the type of the target without checking the type and downcasting? The exact same problem applies to the card user: all I know is that the card came from a card user, how can I interact with it without downcasting? Or is downcasting not the boogieman I think it is?

Attempted Solution 1

I don't worry about it and downcast the target/user to one of the classes that implements it when PerformCardEffect() is run:

public class Card
{
    private ICardUser _user;
    private ITarget _target;

    public PerformCardEffect()
    {
        if (_target is Weather w)
        {
            w.IsCloudy = true;
        }
        if (_target is Enemy e)
        {
            print(e.Species);
        }
        if (_target is PlayerCharacter p)
        {
            p.AttackSpeed *= 2;
        }

       if (_user is PlayerCharacter p)
        {
            p.Health -= 3;
        }
       if (_user is Enemy e)
        {
            e.Health -= 5;
        }

        //etc.
    }
}

This is easiest for me to iterate on right now, but I know this is frowned upon.

Attempted Solution 2

I modify ICardTarget/ICardUser so that the responsibility of calling the correct function is moved off of Card and onto them:

public interface ICardTarget
{
    //...
    public void SelectCardEffectTarget(Card targetingCard);
    //...
}

public interface ICardUser
{
    public void SelectCardEffectUser(Card targetingCard);
}

public class Enemy : ICardTarget, ICardUser
{
    //...
    public void SelectCardEffectTarget(Card targetingCard)
    {
        targetingCard.PerformCardEffectTarget(this);
    }

    public void SelectCardEffectUser(Card targetingCard)
    {
        targetingCard.PerformCardEffectUser(this);
    }
    //...
}

public class PlayerCharacter : ICardTarget, ICardUser
{
    //...
    public void SelectCardEffectTarget(Card targetingCard)
    {
        targetingCard.PerformCardEffectTarget(this);
    }

    public void SelectCardEffectUser(Card targetingCard)
    {
        targetingCard.PerformCardEffectUser(this);
    }
    //...
}

public class Weather : ICardTarget
{
    //...
    public void SelectCardEffectTarget(Card targetingCard)
    {
        targetingCard.PerformCardEffectTarget(this);
    }
    //...
}

public class Card
{
    private ICardUser _user;
    private ITarget _target;

    public void CheckTarget()
    {
        if (_target.IsValidTarget(this))
        {
            _target.SelectCardEffectTarget(this);
            _user.SelectCardEffectUser(this);
        }
    }


    public void PerformCardEffectTarget(Weather w)
    {
        w.IsCloudy = true;
    }
    public void PerformCardEffectTarget(Enemy e)
    {
        print(e.Species);
    }
    public void PerformCardEffectTarget(PlayerCharacter p)
    {
        p.AttackSpeed *= 2;
    }

    public void PerformCardEffectUser(Enemy e)
    {
        e.Health -= 5;
    }
    public void PerformCardEffectUser(PlayerCharacter p)
    {
        p.Health -= 3;
    }

    //I have to include these calls so that any objects not caught by the above overloaded parameters default to doing nothing.
    public void PerformCardEffectTarget(ICardTarget t) {}
    public void PerformCardEffectUser(ICardUser u) {}
}

This seems better but there's a lot of passing references back and forth, which isn't a bad thing but to me it feels a little excessive. I also have to expose a lot of methods on both Card and the interfaces.

P.S. Is there a shorter way of writing SelectCardEffectTarget()/SelectCardEffectUser() so that I don't have to repeat the same code block in each subclass? I know I could make ICardTarget/ICardUser into a class and set the function there so it's inherited, but that doesn't seem like the appropriate hierarchy for this situation.

Attempted Solution 3 (Failed)

I tried to utilize a generic class to specify the target/user types, but I couldn't figure out a way to involve it and get useful functionality from it.

public class CardEffectDetails<T,U> where T : ICardTarget where U : ICardUser
{
    T Target {get;set;}
    U User {get;set;}
}

public class Card
{
    private ICardTarget _target;
    private ICardUser _user;

    private void CreateEffectDetails()
    {
        CardEffectDetails<_target.GetType(), _user.GetType()> effectDetails = new();
        //what can I even do with this thing?
    }
}
 4  102  4
1 Jan 1970

Solution

 1

If you read this Eric Lippert article on this subject he ends up recommending a rules-based approach.

This pushes the logic for playing a card onto a target into a class that is responsible for tying the logic together. Let's have a go at that. (This will be a somewhat simplified version for brevity.)

First let's define some basic interfaces that we will need:

public interface ITarget
{
    string Name { get; }
}

public interface ICard : ITarget
{
}

(Obviously a real application would expand these somewhat.)

Next, we can write a "rules collection" which will encapsulate and hide any reflection and will manage a collection of rules for playing a card on a target:

public sealed class RulesCollection
{
    public void AddRule<TCard, TTarget>(Action<TCard, TTarget> play) where TCard : ICard where TTarget : ITarget
    {
        string key = keyFromTypes(typeof(TCard), typeof(TTarget));
        _rules.Add(key, (card, target) => play((TCard)card, (TTarget)target));
    }

    public bool IsValidTarget(ICard card, ITarget target)
    {
        string key = keyFromObjects(card, target);
        return _rules.ContainsKey(key);
    }

    public void PlayCard(ICard card, ITarget target)
    {
        string key = keyFromObjects(card, target);

        if (_rules.TryGetValue(key, out var play))
            play(card, target);
        else
            throw new InvalidOperationException($"{card.Name} cannot be played on {target.Name}");
    }

    static string keyFromObjects(ICard card, ITarget target)
    {
        return keyFromTypes(card.GetType(), target.GetType());
    }

    static string keyFromTypes(Type cardType, Type targetType)
    {
        return cardType.FullName + "|" + targetType.FullName;
    }

    delegate void playDelegate(ICard card, ITarget target);

    readonly Dictionary<string, playDelegate> _rules = new Dictionary<string, playDelegate>();
}

(This class only handles two types - the card type and the target type. However, you can easily extend it to handle more types for more complex interactions.)

Now let's define some concrete game object types:

public sealed class Fireball : ICard
{
    public int Damage { get; set; } = 10;

    public string Name => "Fireball";
}

public sealed class Hurricane : ICard
{
    public string Name => "Hurricane";

    public int WindSpeed => 90;
    public int Rain      => 100;
}

public sealed class Player : ITarget
{
    public int    Health { get; set; } = 100;
    public string Name   => "Player";
}

public sealed class Enemy : ITarget
{
    public int    Health { get; set; } = 50;
    public string Name   => "Enemy";
}

public sealed class Weather : ITarget
{
    public string Name => "Weather";

    public int WindSpeed { get; set; } = 5;
    public int Rain      { get; set; } = 0;
}

Now we can create a class which actually implements all the necessary card rules and uses a RulesCollection to store them:

public sealed class RulesManager
{
    public RulesManager()
    {
        _rules.AddRule<Fireball, Player>(fireballPlayer);
        _rules.AddRule<Fireball, Enemy>(fireballEnemy);
        _rules.AddRule<Hurricane, Weather>(hurricaneWeather);
    }

    public bool IsValidTarget(ICard card, ITarget target) => _rules.IsValidTarget(card, target);
    public void PlayCard     (ICard card, ITarget target) => _rules.PlayCard     (card, target);

    static void hurricaneWeather(Hurricane hurricane, Weather weather)
    {
        weather.Rain      = hurricane.Rain;
        weather.WindSpeed = hurricane.WindSpeed;

        Console.WriteLine($"Hurricane set weather wind speed to {hurricane.WindSpeed} and rain to {hurricane.Rain}");
    }

    static void fireballPlayer(Fireball fireball, Player player)
    {
        player.Health -= fireball.Damage;
        Console.WriteLine($"Fireball damages player for {fireball.Damage}. Player health is now {player.Health}");
    }

    static void fireballEnemy(Fireball fireball, Enemy enemy)
    {
        enemy.Health -= fireball.Damage;
        Console.WriteLine($"Fireball damages enemy for {fireball.Damage}. Enemy health is now {enemy.Health}");
    }

    readonly RulesCollection _rules = new RulesCollection();
}

You should be able to see how to extend this this cover all your card interactions.

Finally we can exercise the rules:

public static void Main()
{
    var rules = new RulesManager();

    var player    = new Player();
    var fireball  = new Fireball();
    var hurricane = new Hurricane();
    var weather   = new Weather();
    var enemy     = new Enemy();

    rules.PlayCard(fireball, player);
    rules.PlayCard(fireball, enemy);
    rules.PlayCard(hurricane, weather);

    Console.WriteLine(rules.IsValidTarget(hurricane, weather)); // Prints "true".
    Console.WriteLine(rules.IsValidTarget(hurricane, player));  // Prints "false".

    rules.PlayCard(fireball, weather); // Throws exception with message "Fireball cannot be played on Weather". 
}

See this DotNetFiddle for a runnable example:

The output of this is:

Fireball damages player for 10. Player health is now 90
Fireball damages enemy for 10. Enemy health is now 40
Hurricane set weather wind speed to 90 and rain to 100
True
False
Unhandled exception. System.InvalidOperationException: Fireball cannot be played on Weather
   at RulesCollection.PlayCard(ICard card, ITarget target)
   at RulesManager.PlayCard(ICard card, ITarget target)
   at Net48Console.Program.Main()
2024-07-24
Matthew Watson

Solution

 0

Thinking out loud, but this message is too big for a comment anyway. Plus, the question is also kinda abstract =)

Option 1 (not your case, I think, but still)

You can make a unified interface with which your effects can interact. It worked well for me in the case of characters (atk, def, hp, mana, evasion, crit chance, etc.), but it doesn't look like a valid option if your objects are too different. However, it can be an option if you only have 5-10 parameters to set up.

Option 2 (boxing, ofc, but it can be ok many cases)

It is almost the same as Option 1, but with Dictionary<string, object> as a container for your parameters. You can place whatever you want there, and your effects can try to get any needed parameters. In that case, your effect would rely not on the type of the card but mostly on the presence of the required parameters for this effect to be applied.


Also, I highly recommend thinking about moving your effects outside of your card. Then, your card's common logic will be mostly like a container with a set of parameters to be changed and set effects to be applied on use.

2024-07-23
Morion