How can I improve this code?

Question

I just wrote a simple direction based combat script. However, it feels unresponsive and I have the feeling that I used the code inefficiently.

How can I improve it?

 using System.Collections;
 using System.Collections.Generic;
 using UnityEngine;
 
 public class attack : MonoBehaviour {
     static Animator anim;
     public bool attack1;
     public bool attack2;
     public bool attack3;
     public bool position1;
     public bool position2;
     public bool position3;
 
     IEnumerator attackone()
     {
         yield return new WaitForSeconds(1);
         attack1 = false;
     }
     IEnumerator attacktwo()
     {
         yield return new WaitForSeconds(1);
         attack2 = false;
     }
     IEnumerator attackthree()
     {
         yield return new WaitForSeconds(1);
         attack3 = false;
     }
     // Use this for initialization
     void Start () {
         anim = GetComponent<Animator>();
     }
     
     // Update is called once per frame
     void Update () {
 
         if (Input.GetAxis("Mouse X") > 0)
         {
             position1 = true;
             position2 = false;
             position3 = false;
 
         }
         if (Input.GetAxis("Mouse X") < 0)
         {
             position2 = true;
             position1 = false;
             position3 = false;
         }
         if (Input.GetAxis("Mouse Y") < 0)
         {
             position3 = true;
             position2 = false;
             position1 = false;
         }
         if (position1 == true)
         {
             if (Input.GetMouseButtonDown(0))
             {
                 attack1 = true;
             }
         }
         if (position2 == true)
         {
             if (Input.GetMouseButtonDown(0))
                 attack2 = true;
         }
         if (position3 == true)
         {
             if (Input.GetMouseButtonDown(0))
             {
                 attack3 = true;
             }
         }
         if (attack1 == true)
         {
             anim.SetBool("attackleft", true);
             anim.SetBool("idle", false);
             StartCoroutine("attackone");
         }
         if (attack2 == true)
         {
             anim.SetBool("attackright", true);
             anim.SetBool("idle", false);
             StartCoroutine("attacktwo");
         }
         if (attack3 == true)
         {
             anim.SetBool("attackup", true);
             anim.SetBool("idle", false);
             StartCoroutine("attackthree");
         }
         if (attack1 == false && attack2 == false && attack3 == false)
         {
             anim.SetBool("idle", true);
         }
         if (attack1 == false)
         {
             anim.SetBool("attackleft", false);
         }
         if (attack2 == false)
         {
             anim.SetBool("attackright", false);
         }
         if (attack3 == false)
         {
             anim.SetBool("attackup", false);
         }
     }
 }

Answer 1

CharacterDirection enum

From reading your script, it looks like you're representing the different directions your character is facing with the variables position1, position2, etc. The way I would write this instead would be to use an enum. An enum gives you the advantage of being more concise (one variable instead of three) and more understandable (I can give the enum value meaning, like "Left", "Up", and "Right"). I called this enum CharacterDirection because that seemed like the concept it represented.

isAttacking

It also looks like your attacks are tied to position where attack1 represents an attack at position1 and so on. Since the attack is tied with the position, once you're using an enum you can condense the multiple attack variables into a simpler "isAttacking" boolean. I like using true/false words at the start of my boolean variables like "is", "can", or "should" because I find it helps make the variable name clearer.

Coroutine

With those changes -- using an enum and an "isAttacking" variable -- you also only need a single coroutine to handle the duration of the attack, so that can be condensed as well.

Start/Stop Animation Functions

Lastly, I notice you're setting a lot of different booleans on your animation in several different places. I think it might help to give those their own meaningful functions like StartAttackAnimation() and StopAttackAnimation(). This would let you better control exactly when the animation begins and ends, and might help you to figure out that unresponsiveness and get the exact behavior you want.

Script

The code I came up with looks like this:

 using System.Collections;
 using System.Collections.Generic;
 using UnityEngine;
  
 public enum CharacterDirection
 {
     Left,
     Right,
     Up
 }
 
 public class Attack : MonoBehaviour 
 {
     static Animator anim;
     public bool isAttacking;
     public CharacterDirection characterDirection;
 
     IEnumerator attackWait()
     {
         yield return new WaitForSeconds(1);
         if(!isAttacking)
         {
             StopAttackAnimation();
         }
     }
 
     // Use this for initialization
     void Start() 
     {
         anim = GetComponent<Animator>();
     }
      
     // Update is called once per frame
     void Update() 
     {
         if (Input.GetAxis("Mouse X") > 0)
         {
             characterDirection = CharacterDirection.Left;
         }
         if (Input.GetAxis("Mouse X") < 0)
         {
             characterDirection = CharacterDirection.Right;
         }
         if (Input.GetAxis("Mouse Y") < 0)
         {
             characterDirection = CharacterDirection.Up;
         }
         if (Input.GetMouseButtonDown(0))
         {
             isAttacking = true;
             StartAttackAnimation();
             StartCoroutine("attackWait");
         }
         else 
         {
             isAttacking = false;
         }
     }
     
     private void StartAttackAnimation()
     {
         anim.SetBool("idle", false);
         anim.SetBool("attackleft", characterDirection == CharacterDirection.Left);
         anim.SetBool("attackright", characterDirection == CharacterDirection.Right);
         anim.SetBool("attackup", characterDirection == CharacterDirection.Up);
     }
 
     private void StopAttackAnimation()
     {
         anim.SetBool("idle", true);
         anim.SetBool("attackleft", false);
         anim.SetBool("attackright", false);
         anim.SetBool("attackup", false);
     }
 }

Note that the intent of this code is so that your player should be able to hold down the mouse button and continuously attack. If that's not what you want, you can play around with conditions for when to set isAttacking to false and when to call StopAttackAnimation();

How can I improve this code?

1 Reply

CharacterDirection enum

isAttacking

Coroutine

Start/Stop Animation Functions

Script

Your answer

Follow this Question

Related Questions