Method that calculates the estimated number of attacks

Question

This method assumes that both unit and target start at the same location, and that both will continuously run in a straight line in the same direction. Every time enemy gets within the attack range of unit, unit will attack but it needs to stand still and complete it's attack animation before being able to run or attack again. Attack will hit once it completes it's fore swing even if the back swing and idle time were not able to be completed due to duration ending. Even if enemy is able to get out of unit attack range while unit is doing his attack animation, attack will still hit and animation will still complete.

Is there anything I can do to make it more optimized? I need it to have the best performance it can get.

    public static float EstimatedNumberOfAttacks(this Unit unit, Unit target, float duration, float overrideMoveSpeed = 0)
    {
        var immobileDuration = target.GetImmobileDuration();
        var attackAnimation = unit.ForeSwing + unit.BackSwing + unit.AttackIdleTime;

        //target is not able to move for whole duration
        if (!target.IsMoving() || immobileDuration >= duration)
        {
            return (float) Math.Floor(duration / attackAnimation);
        }

        var durationLeft = duration;
        float endDuration;

        var enemySpeed = target.MovementSpeed;
        var unitSpeed = overrideMoveSpeed <= 0 ? unit.MovementSpeed : overrideMoveSpeed;
        var speedDifference = unitSpeed - enemySpeed;

        var numberOfAttacksWhileImmobile = (float) Math.Ceiling(immobileDuration / attackAnimation);
        var immobileTimeElapsed = numberOfAttacksWhileImmobile * attackAnimation;
        var distanceTraveledAfterImmobile = (immobileTimeElapsed - immobileDuration) * enemySpeed;

        //duration ends in the middle of the attack animation of your last attack before immobility ends
        if ((endDuration = durationLeft - immobileTimeElapsed + attackAnimation) < 0)
        {
            while (endDuration < unit.ForeSwing)
            {
                numberOfAttacksWhileImmobile--;
                endDuration += attackAnimation;
            }

            return numberOfAttacksWhileImmobile;
        }

        durationLeft -= immobileTimeElapsed;

        var startNumberOfAttacks = 0f;
        var startTimeElapsed = 0f;
        float startCatchUpTime;

        var distanceTraveledDuringAttack = attackAnimation * enemySpeed;
        var unitAttackRange = unit.GetAttackRange();

        //enemy was not able to get out of attack range within the last attack animation before immobility ended
        if (unitAttackRange >= distanceTraveledAfterImmobile)
        {
            float startDistance;
            //enemy was not able to get out of attack range after a single attack
            if (distanceTraveledDuringAttack <= unitAttackRange - distanceTraveledAfterImmobile)
            {
                startNumberOfAttacks = (float) Math.Ceiling((unitAttackRange - distanceTraveledAfterImmobile) / distanceTraveledDuringAttack);
                startDistance = startNumberOfAttacks * distanceTraveledDuringAttack;
                startTimeElapsed = startDistance / enemySpeed;
            }
            else
            {
                startNumberOfAttacks = 1;
                startDistance = distanceTraveledDuringAttack + distanceTraveledAfterImmobile;
                startTimeElapsed = attackAnimation;
            }

            //duration ends in the middle of the attack animation of your last attack before enemy is able to get out of your attack range
            if ((endDuration = durationLeft - startTimeElapsed + attackAnimation) < 0)
            {
                while (endDuration < unit.ForeSwing)
                {
                    startNumberOfAttacks--;
                    endDuration += attackAnimation;
                }

                return numberOfAttacksWhileImmobile + startNumberOfAttacks;
            }

            startCatchUpTime = startDistance / speedDifference;
        }
        else
        {
            startCatchUpTime = distanceTraveledAfterImmobile / speedDifference;
        }

        //target is uncatchable
        if (speedDifference <= 0)
        {
            return numberOfAttacksWhileImmobile + startNumberOfAttacks;
        }

        durationLeft -= startTimeElapsed + startCatchUpTime;

        durationLeft -= attackAnimation;
        startNumberOfAttacks++;

        //see if last attack was able to hit
        if (durationLeft < 0)
        {
            if (durationLeft + attackAnimation > unit.ForeSwing)
            {
                startNumberOfAttacks--;
            }

            return numberOfAttacksWhileImmobile + startNumberOfAttacks;
        }

        //when both are now running its just a repeating pattern of catch up-attack-catch up-attack until duration ends
        var catchUpTime = distanceTraveledDuringAttack / speedDifference;
        var additionalAttacks = (float) Math.Floor(durationLeft / (catchUpTime + attackAnimation));
        var additionalAttackTimeTaken = (catchUpTime + attackAnimation) * additionalAttacks;

        durationLeft -= additionalAttackTimeTaken;

        durationLeft -= catchUpTime;

        //see if last attack was able to hit
        if (durationLeft > unit.ForeSwing) additionalAttacks++;

        return numberOfAttacksWhileImmobile + startNumberOfAttacks + additionalAttacks;
    }

Answer 1

Vague naming

var attackAnimation = unit.ForeSwing + unit.BackSwing + unit.AttackIdleTime

If "IdleTime" hadn't been in the third property's name; it would be impossible to know what the variable expresses:

var attackAnimation = unit.ForeSwing + unit.BackSwing + unit.Attack;

Is this a number? A string? Are these actual Animation objects which you are able to concatenate?

Just like AttackIdleTime, you should be more clear about the other names: BackSwingDuration, ForeSwingDuration, attackAnimationDuration, AttackIdleDuration.

Notice that I use Duration instead of Time. Semantically, "time" is an absolute value (13:00:00) whereas "duration" (a.k.a. a timespan) is a length of time (13 hours).

---

Vague comments

//duration ends in the middle of the attack animation of your last attack before enemy is able to get out of your attack range 

//enemy was not able to get out of attack range within the last attack animation before immobility ended

//duration ends in the middle of the attack animation of your last attack before immobility ends

I'm sure that you knew what you were talking about at the time of writing the comment, but these comments are incredibly hard to accurately decipher.
I doubt that you'll understand the comments in a year's time without first having to read the code again; thus defeating the purpose of the comment in the first place.

For me, as a "new developer" looking at this code, this comment does nothing more than hint that you have separate logical algorithms. The exact distinction between these algorithms isn't all that clear.

---

The method body

Your code is checking every possible combination of factors separately, and is not unifying the shared pieces of logic.

To showcase the issue:

public void MyMethod(User user, bool userWinks, bool userTalks, bool userWalks)
{
    if (userWinks)
    {
        if (userTalks)
        {
            if (userWalks)
            {
                user.Wink();
                user.Talk();
                user.Walk();
            }
            else
            {
                user.Wink();
                user.Talk();
            }
        }
        else
        {
            if (userWalks)
            {
                user.Wink();
                user.Walk();
            }
            else
            {
                user.Wink();
            }
        }
    }
    else
    {
        //omitted for brevity's sake
    }
}

This is essentially what you're doing. You're trying to provide every single mutually exclusive use case in a single method body. This is not good. There is no benefit to having mutually exclusive logic grouped together, as these will inherently never be useful at the same time.

You should be separating these checks, not nesting them. To correct the example:

public void MyMethod(User user, bool userWinks, bool userTalks, bool userWalks)
{
    if (userWinks)
    {
        user.Wink();
    }

    if (userTalks)
    {
        user.Talk();
    }

    if (userWalks)
    {
        user.Walk();
    }
}

Note: because you use return statements, you're technically not nesting (i.e. indenting) them, but rather relying on the implicit "this code will only be reached if we don't alredy return a value" approach. While this technically isn't nesting, it's still bad code for the same reason.

---

The calculation

I think there's room for improvement here. You've separate the situations to a point where they don't overlap, even though this method is used for calculating a deterministic behavior. That's weird.

As far as I understand it, the attacker's actions are a logical iteration:

1. Run until the target is in attack range.
2. Attack the target (foreswing)
3. Experience "attack cooldown" (backswing + idle time)
4. Repeat

Note that this logical progression is true for every unique case you've listed:

• //target is not able to move for whole duration If the target is not moving, step 1 will have a duration of 0.
• //target is uncatchable If the target moves faster than the attacker, step 1 will never complete
• //duration ends in the middle of the attack animation of your last attack before immobility ends It has taken me some time to understand what you mean (not the easiest comment, to be honest). But this isn't an exceptional case, limiting yourself to the configured duration is inherent to the algorithm. Since your return value is the number of attacks, it suffices to perform this check just before you increase the number.
• //enemy was not able to get out of attack range after a single attack Similar to an idling target, this simply means that step 1 will have a duration of 0.

Look at the simplicity of the 3 steps. They are true for every case, regardless of:

• Which character runs faster
• The starting positions of the characters
• How long it takes to perform an attack
• How long the configured duration is

While it is true that in some cases, some steps are irrelevant (e.g. target is immobile => attacker doesn't need to catch up), it's not wrong to still execute that step (with an expected duration of 0).
There is no point preventing an unnecessary step, if taking the step does not negatively impact the outcome.

---

Algorithm simplification

This is just a peusodocode implementation of the 3 steps I listed. Note that I keep track of a currentTimer, which runs from 0 to duration (at which point the simulation ends)

1. Run until the target is in attack range.

Notice that the only importance of this method is to figure out how long it takes to catch up.

var distanceToCatchUp = distanceBetweenCharacters - attackRange;

if(distanceToCatchUp > 0) //target is out of range
{
    var attackerCatchUpSpeed = attackerMoveSpeed - targetMoveSpeed;

    if(attackerCatchUpSpeed <= 0)
    {
         //attacker can never catch up, time needed is infinite (we cap it at the max duration)
         currentTimer += duration;
    }
    else
    {
         //calculate the needed duration
         var neededTime = Math.Ceiling((float)distanceToCatchUp/attackerCatchUpSpeed); 

         currentTimer += neededTime;
    }
}

2. Attack the target (foreswing)

Note the responsibilities of this snippet:

• Perform the foreswing duration
• Check if the currentTimer has exceeded the duration
  • If not, increase numberOfAttacks by 1
  • If so, return the current numberOfAttacks

currentTimer += unit.ForeSwingDuration;

if(currentTimer >= duration)
    return numberOfAttacks;
else
    numberOfAttacks++;

This is the only line that will return the value! There will be no other return statement.

3. Experience "attack cooldown" (backswing + idle time)

currentTimer += unit.BackSwingDuration + unit.AttackIdleDuration;

4. Repeat

Wrap all of the above in a while(true). The idea is that the simulation runs until it reaches the configured duration time, at which point the return statement will be called.

Footnotes

I've omitted a few things for brevity's sake, as they are not the main focus on the issue here:

• Setting the initial character positions
• Calculating the distance traveled by the target during the "attack cooldown", which is simply totalAttackDuration * target.MoveSpeed.
• (unspecified) Is there a fringe case where an attack misses if the target leaves the attack range before the foreswing completes?
  • If so, should the attacker then make sure to approach closer than the max attack range, so he can ensure hitting the target?