Need to make timer for damage for the player

I have made player health and timer but for some reason its not all working need help

using System.Collections;
using System.Collections.Generic;
using UnityEngine;

public class PlayerHealth : MonoBehaviour
{
public int maxHealth;
public int curHealth;
public GameObject Enemy;
public GameObject Player;
public float enemydistance;
public HealthBar healthBar;
public float timer = 3;
public int TakeDamage = 5;
public bool Damage = false;
// Start is called before the first frame update
void Start()
{
curHealth = maxHealth;
healthBar.SetMaxHealth(maxHealth);
}

// Update is called once per frame
void Update()
{
    enemydistance = Vector3.Distance(Enemy.transform.position, Player.transform.position);


    if (enemydistance < 1.5f)
    {
        StartCoroutine("TakeDamage");
        Damage = true;
    }
    IEnumerator TakeDamage()
    {
        if (Damage == true) 
        {
            curHealth -= TakeDamage;
            Damage = false;
            yield return new WaitForSeconds(timer);
            Damage = true;
        }
    }
}
}

Firstly, it seems we’re getting a compiler error because we have named both the coroutine and the amount of damage you want to take “TakeDamage”. I’d say you should change the name of the variable “TakeDamage” to something else - make sure you change it in both where it’s defined public int TakeDamage = 5; and where it’s used curHealth -= TakeDamage;

Secondly, we’re running this coroutine at weird times. I am assuming that the intention is that if the enemy is close enough (< 1.5f) then you get damaged once every 3 seconds, as specified by the timer variable.

Currently if the enemy is close enough then we start this coroutine every single frame. Now you might think this is okay because we only enter the damaging part of the coroutine if Damage is true? However we’re also setting Damage to true whenever we start the coroutine as well. I’d expect us to enter the damaging part of the coroutine either every frame or every other frame when the enemy is close to us, depeneding on when coroutines are scheduled. Either way, this is not the intention.

There are a few points at which we can restructure this to make the logic much simpler. Firstly, I think we should check Damage outside the coroutine, next to where we check the enemy distance. This means that we only ever start the coroutine if we’re able to take damage and we’re close enough to take damage. Secondly, we need to make sure Damage is set at the right times. I think we should start with Damage set to true public bool Damage = true; because we only set it to false just after we’ve taken damage, when we’re invulnerable. We start with damage true as we’re not invulnerable at the start, we are able to take damage even though we won’t until the enemy is close enough.

So if our code now looks like this: (note I’ve changed the name of the damage amount variable to TakeDamageAmount)

if (enemydistance < 1.5f && Damage == true)
{
    StartCoroutine("TakeDamage");
}
IEnumerator TakeDamage()
{
    curHealth -= TakeDamageAmount;
    Damage = false;
    yield return new WaitForSeconds(timer);
    Damage = true;
}

Now the only thing that controls our Damage variable is the coroutine. During the time that we’re waiting for timer to complete, Damage is false and therefore we won’t enter the coroutine.


This all said, it might be a lot simpler to do this without a coroutine entirely. If you store the time at which you take damage then you can check how much time has elapsed since your last damage instead of using your Damage variable.
if (enemydistance < 1.5f && (Time.time - lastDamageTime) >= timer)
{
    lastDamageTime = Time.time;
    curHealth -= TakeDamageAmount;
}