Ability System in my game

Question

I'm writing a game in C# Monogame, and I've decided to make my project as modular as possible.

GitHub

At first, I've had a single project with a folder that's separated inside to other folders which represent different systems in the game (Items, Abilities, Stats).

I've started by making a .dll project for everything that I've considered as a 'system', i.e - Items, Abilities, Stats, and others that I will soon add, like the player itself (that's just a class right now).

I'm designing the Abilities system based on the answer given on this post.

I'm asking for a code review about the Ability system module, and if you care, about the rest of the game (the MainModule is just remainder of what I've had before I refactored into modularizing everything, so for now there's not much need for whatever's there, unless you see something the could help).

I'd appreciate referencing stuff (or solutions to problems I have) to design patterns, as I'd rather use design patterns to solve problems.

The ability system code:

public interface IBehavior
{
    void ApplyBehavior(IUnit unit);
}

// ---------------------------------------------------- //   

public abstract class ActivatableBehavior : IBehavior
{
    public abstract bool BehaviorImplentation(IUnit destinationPlayer);

    public void ApplyBehavior(IUnit unit)
    {
        BehaviorImplentation(unit);
    }
}

// ---------------------------------------------------- //   

public abstract class LimitedTimeBehavior : IBehavior
{
    public Stopwatch Timer { get; }
    public abstract void ApplyBehavior(IUnit unit);
    public long Duration { get; }

    protected LimitedTimeBehavior(long durationOfBehavior)
    {
        Timer = new Stopwatch();
        Duration = durationOfBehavior;
    }
}

// ---------------------------------------------------- //   

public abstract class TickBasedBehavior : LimitedTimeBehavior
{
    public long TickTime { get; }
    protected abstract void ApplyTick(IUnit unit);


    protected TickBasedBehavior(long durationOfBehavior, long timeBetweenTicks)
        : base(durationOfBehavior)
    {
        TickTime = timeBetweenTicks;
    }

    public override void ApplyBehavior(IUnit unit)
    {
        Thread trd = new Thread(() => ThreadMethod(unit));
        trd.Start();
    }

    private void ThreadMethod(IUnit unit)
    {
        Timer.Start();

        while (Timer.ElapsedMilliseconds < Duration)
        {
            if (Timer.Elapsed.TotalMilliseconds % TickTime == 0)
                ApplyTick(unit);
        }

        Timer.Stop();
    }
}

// ---------------------------------------------------- //   

public abstract class DurationBehavior : LimitedTimeBehavior
{
    protected abstract void Activate(IUnit unit);
    protected abstract void Deactivate(IUnit unit);

    private bool IsActivated { get; set; }

    protected DurationBehavior(long durationOfBehavior)
        : base(durationOfBehavior)
    {
        IsActivated = false;
    }

    public override void ApplyBehavior(IUnit unit)
    {
        if (IsActivated) return;
        Thread trd = new Thread(() => ThreadMethod(unit));
        trd.Start();
        IsActivated = true;
    }

    private void ThreadMethod(IUnit unit)
    {
        Timer.Start();
        Activate(unit);
        while (Timer.Elapsed.TotalMilliseconds < Duration) { }
        Timer.Reset();
        Deactivate(unit);
        IsActivated = false;
    }
}

Answer 1

That is really not how you should be using a Stopwatch...

E.g. this

while (Timer.Elapsed.TotalMilliseconds < Duration) { }

You're needlessly using CPU cycles while you're waiting. That's not good! You know exactly when you want to do something so you should scheduled something rather than do pretend work. A simple example could be:

private void ThreadMethod(IUnit unit)
{
    Activate(unit);
    Task.Delay(Duration).ContinueWith(_ => Deactivate(unit));
}

This isn't safe either:

while (Timer.ElapsedMilliseconds < Duration)
{
    if (Timer.Elapsed.TotalMilliseconds % TickTime == 0)
        ApplyTick(unit);
}

You're very unlikely to actually get true on that if statement meaning you're unlikely to call ApplyTick and almost certainly won't call it the number of times you want to! That's because a Tick is a really, really small measure of time... I want to say 1/10000 of a millisecond.

Edit - I misread your if statement - I thought you were using Ticks (due to the TickTime field. It's still not a great idea but less serious than the above implies.

You shouldn't need a stopwatch at all for this sort of thing.

---

You should use the highest level of abstraction you can. A double can be anything (e.g. hours, minutes, seconds...) - a timespan is a period of time, much more suitable for your LimitedTimeBehavior:

public abstract class LimitedTimeBehavior : IBehavior
{
    public abstract void ApplyBehavior(IUnit unit);

    public TimeSpan Duration { get; }

    protected LimitedTimeBehavior(TimeSpan duration)
    {
        Duration = duration;
    }
}

---

You said you're interested in design patterns so I though I'd draw your attention to one you're nearly using:

public abstract class ActivatableBehavior : IBehavior
{
    public abstract bool BehaviorImplentation(IUnit destinationPlayer);

    public void ApplyBehavior(IUnit unit)
    {
        BehaviorImplentation(unit);
    }
}

That's the Template Method pattern but it's more normal to call multiple methods within the template method.

public abstract class ActivatableBehavior : IBehavior
{
    public abstract void BehaviorImplementation(IUnit destinationPlayer);

    public abstract bool CanApplyBehvaiorTo(IUnit destinationPlayer);

    public void ApplyBehavior(IUnit unit)
    {
        if (CanApplyBehvaiorTo(unit))
        {
            BehaviorImplementation(unit);
        }
    }
}

Note that I've fixed your typo (Implentation -> Implementation) and have removed the bool return type that was never used.