Enemy is losing 2 health instead of 1

Question

I am trying to componentize all of my scripts and I would like the enemy to lose 1 health (enemy max health is 3) every time the player collides with the enemy. Right now the enemy is losing 2 health instead of 1. This is my hierarchy for the enemy:

There is Health and Take Damage On Collision scripts under the Enemy gameObject and a Health Bar script under the Health Bar gameObject.

Enemy gameObject:

Health Bar gameObject:

Perhaps there's code running one in two separate places? I know OnCollisionEnter2D runs every frame but that's why I created a damageInterval. Here are my scripts:

public class Health : MonoBehaviour {

    public int maxHealth;
    public int currentHealth;

    void Start() {
        currentHealth = maxHealth;
    }
}

public class HealthBar : MonoBehaviour {

    [SerializeField] private GameObject character;
    public Slider slider;
    public Gradient gradient;
    public Image fill;

    private void Start() {
        Health health = character.GetComponent<Health>();
        HealthBar healthBar = gameObject.GetComponent<HealthBar>();
        healthBar.SetMaxHealth(health.maxHealth);
    }

    public void SetMaxHealth(int health) {
        slider.maxValue = health;
        slider.value = health;

        /*on the gradient that has (left to right) red, yellow and green fixed portions,
        all the way to the left 0f (red), and all the way to the right 1f (green)
        we assign the entire fill of the HealthBar to this color
        */
        fill.color = gradient.Evaluate(1f);
    }

    public void SetHealth(int health) {
        slider.value = health;

        /*we assign the entire fill of the HealthBar to the normalized value (0 to 1) so
        //the 0 to 10 points of health syncs with 0 to 1 on the gradient. this determines
        what color the fill will be depending on how much health is left
        */
        fill.color = gradient.Evaluate(slider.normalizedValue);
    }
}

public class TakeDamage : MonoBehaviour {

    private float damageInterval = 2f;
    private float currentDamageInterval;

    private void Update() {
        if(currentDamageInterval >= 0)
            currentDamageInterval -= Time.deltaTime;
    }

    public bool CanTakeDamage() {
        return (currentDamageInterval < 0);
    }

    public void EnemyTakeDamage(int damageAmount) {
        Health health = gameObject.GetComponent<Health>();

        if(gameObject.CompareTag("Hazard") && CanTakeDamage()) {
            health.currentHealth -= damageAmount;
            currentDamageInterval = damageInterval;
            if (health.currentHealth <= 0)
                Destroy(gameObject);
        }
    }
}

public class TakeDamageOnCollision : MonoBehaviour {

    [SerializeField] private int damageAmount;
    [SerializeField] private GameObject healthBarGO;
    HealthBar healthBar;
    TakeDamage takeDamage;

    private void Start() {
        takeDamage = gameObject.AddComponent<TakeDamage>();
        healthBar = healthBarGO.GetComponent<HealthBar>();
    }
    //if the player collides with enemy weak spot and can damage enemy
    private void OnCollisionEnter2D(Collision2D collision) {
        if (gameObject.CompareTag("Hazard") && collision.gameObject.CompareTag("Player")) {
            takeDamage.EnemyTakeDamage(damageAmount);
            healthBar.SetHealth(damageAmount);
        }
    }
}

If you have any tips on componentizing my scripts, I'm all ears.

Answer 1

I think you have twice as many components as you need here. While I'm generally in favour of small single-purpose classes, "have a health value" is so tightly linked to "modify health value" that they're really together a single purpose.

By splitting the jobs of having, displaying, and modifying health between separate files, you make the reader do a lot more jumping around in their editor, and juggle more moving parts in mind. That added complexity makes it easy to miss obvious mistakes like this:

healthBar.SetHealth(damageAmount)

You're setting the displayed health value to the amount of damage you took, not the amount of health remaining after subtracting that damage.

I'd split out the classes like this:

• ResourceBar: responsible for displaying a bar measuring some quantity. Could be health, but you can also re-use the same component to handle stamina/mana/time/food/etc.

• Health: responsible for having and modifying a health number, handling taking damage, immunity interval, issuing OnDie events when health reaches zero, etc. Has a reference to a ResourceBar that can be updated when the health changes.

• CollisionDamage: handles responding to collision event by issuing damage to Health.TakeDamage(). Doesn't need to know anything about ResourceBar since updating the display is not its responsibility.

  Also, does not need to check its own tag in the collision handler (gameObject.CompareTag("Hazard")). If you didn't want this object to run this collision handler, you would simply not attach this component to it.

---

Here's an example:

public class Health : MonoBehaviour {
    [field:SerializeField]
    public int MaxHealth {get; private set;}

    [field:SerializeField]
    public int CurrentHealth {get; private set;}

    [SerializeField] 
    float _invulnerabilityDuration;
    float _invulnerabilityRemaining;

    [SerializeField]
    ResourceBar _bar;
    
    public UnityEvent OnDie;

    public bool IsAlive => CurrentHealth > 0;
    public bool CanTakeDamage => IsAlive && (_invulnerabilityRemaining <= 0f);

    public void TakeDamage(int damage, bool bypassInvulnerability = false) {
        Assert.IsTrue(damage >=0, 
            "Cannot apply negative damage - call Heal instead");

        if (!(bypassInvunerability || CanTakeDamage)) return;

        CurrentHealth -= damage;

        if (CurrentHealth <= 0) {
            CurrentHealth = 0;
            OnDie?.Invoke();
        }

        if (!bypassInvulnerability) {
            _invulnerabilityRemaining = invulnerabilityDuration;
        }

        if (_bar != null) {
            _bar.SetValue(CurrentHealth, MaxHealth);
        }
    } 

    public void Heal(float gain) {
        Assert.IsTrue(gain >=0, 
             "Cannot apply negative healing - call TakeDamage instead");

        CurrentHealth = Mathf.Min(CurrentHealth + gain, MaxHealth);

        if (_bar != null) {
            _bar.SetValue(CurrentHealth, MaxHealth);
        }
    }

    void Start() {
        if (_bar != null) {
            _bar.SetValue(CurrentHealth, MaxHealth);
        }
    }

    void FixedUpdate() {
        _invulnerabilityRemaining -= Time.deltaTime;
    }
}

Since the Health component's TakeDamage()/Heal() functions are the only ones that can modify the internal health value, and calling them always updates the health bar if one is provided, you know that anything that affects health will correctly update both the internal health value and the bar in sync, as well as handle doing things like invoking death. All that complexity is encapsulated, so outside code doesn't need to know the details of everything that might need to respond to changes in health state - it just needs to know the TakeDamage()/Heal() interface. And this restricts the possible search area for bugs in health changes to just a few lines in one file.