Problem
I am currently developing a Unity-based multiplatform game (PC, iOS, Android) in a small team. This is the code we have created for the character actions inside the game. I am looking for feedback on the code itself as well as tips on how some Unity features could be used better. This is my first time developing a game with Unity.
The game is also aimed for mobile platforms as stated above, so tips on optimization are welcome.
using AI.Enemy;
using UnityEngine;
namespace Player.Script.Miru
{
public class MiruScript : MonoBehaviour
{
public float Health;
public float MoveSpeed;
public float JumpHeight;
public float EvadeSpeed;
public float DefendCooldownTimer;
public float NormalDamage;
public float Cooldown;
public float Stamina;
public float HealthRegenerationRate;
public float StaminaRegenerationRate;
private bool _IsFacingForward;
private bool _IsGrounded;
private Rigidbody2D _RigidBody;
private RaycastHit2D _GroundDetector;
private RaycastHit2D _RightRayCast;
private RaycastHit2D _LeftRayCast;
private MinionScript _MinionScript;
private void Start() //Temporary values
{
MoveSpeed = 4f * Time.deltaTime;
JumpHeight = 10f;
EvadeSpeed = 5f;
DefendCooldownTimer = 0f;
NormalDamage = 50f;
Cooldown = 1f;
Stamina = 100f;
Health = 100f;
HealthRegenerationRate = 0.5f;
StaminaRegenerationRate = 10f;
_RigidBody = GetComponent<Rigidbody2D>();
_MinionScript = GetComponent<MinionScript>();
}
private void Update()
{
MoveRight(MoveSpeed);
MoveLeft(MoveSpeed);
Jump(JumpHeight);
Evade(EvadeSpeed);
Attack(NormalDamage);
DistanceFromObject();
AttackCooldown();
Defend();
}
private bool AttackCooldown()
{
if (Cooldown < 1f)
{
Cooldown += Time.deltaTime;
return true;
}
return false;
}
public int DamageDealt(float _damageDealt)
{
Health -= _damageDealt;
if (Health <= 0)
Destroy(gameObject);
return 0;
}
private int DistanceFromObject()
{
_RightRayCast = Physics2D.Raycast(transform.position, Vector2.right);
_LeftRayCast = Physics2D.Raycast(transform.position, Vector2.left);
if (_RightRayCast.distance < 1.5f && _LeftRayCast.distance < 1.5f)
return 3;
if (_RightRayCast.distance < 1.5f)
return 1;
if (_LeftRayCast.distance < 1.5f)
return 2;
return 0;
}
private int Attack(float _damageDealt)
{
if (_IsFacingForward && Input.GetKeyDown(KeyCode.X) && _RightRayCast.distance <= 1.5f && !AttackCooldown())
{
_MinionScript = _RightRayCast.collider.gameObject.GetComponent<MinionScript>();
_MinionScript.DamageDealt(_damageDealt);
Cooldown = 0;
return 1;
}
if (_IsFacingForward == false && Input.GetKeyDown(KeyCode.X) && _LeftRayCast.distance <= 1.5f && !AttackCooldown())
{
_MinionScript = _LeftRayCast.collider.gameObject.GetComponent<MinionScript>();
_MinionScript.DamageDealt(_damageDealt);
Cooldown = 0;
return 2;
}
return 0;
}
private int MoveRight(float _moveSpeed)
{
if (Input.GetKey(KeyCode.RightArrow) && Defend() == 0)
{
transform.Translate(_moveSpeed, 0, 0);
_IsFacingForward = true;
return 1;
}
return 0;
}
private int MoveLeft(float _moveSpeed)
{
if (Input.GetKey(KeyCode.LeftArrow) && Defend() == 0)
{
transform.Translate(-_moveSpeed, 0, 0);
_IsFacingForward = false;
return 1;
}
return 0;
}
private int Jump(float _height)
{
_GroundDetector = Physics2D.Raycast(transform.position, Vector2.down);
if (Input.GetKeyDown(KeyCode.Z) && _IsGrounded)
{
_RigidBody.AddForce(Vector2.up * _height, ForceMode2D.Impulse);
return 1;
}
if (_GroundDetector.distance > 0.6f)
{
_IsGrounded = false;
return 2;
}
_IsGrounded = true;
return 0;
}
private int Evade(float _evadeSpeed)
{
if (Input.GetKeyDown(KeyCode.Space) && _IsGrounded)
switch (DistanceFromObject())
{
case 1:
_RigidBody.AddForce(Vector2.up * _evadeSpeed, ForceMode2D.Impulse);
_RigidBody.AddForce(Vector2.left * _evadeSpeed, ForceMode2D.Impulse);
return 1;
case 2:
_RigidBody.AddForce(Vector2.up * _evadeSpeed, ForceMode2D.Impulse);
_RigidBody.AddForce(Vector2.right * _evadeSpeed, ForceMode2D.Impulse);
return 2;
case 3:
_RigidBody.AddForce(Vector2.up * _evadeSpeed * 3, ForceMode2D.Impulse);
return 3;
}
return 0;
}
private int Defend()
{
if (Input.GetKey(KeyCode.Space) && _IsGrounded)
{
DefendCooldownTimer += Time.deltaTime;
if (DefendCooldownTimer >= 0.5f)
{
return 1;
}
}
else
DefendCooldownTimer = 0f;
return 0;
}
}
}
Solution
Magic numbers like 0, 1, 2 and 3 returned by DistanceFromObject are difficult to read and are easily mixed up. Use an enum instead.
[Flags]
private enum Proximity
{
None = 0,
Right = 1,
Left = 2,
LeftAndRight = Left | Right
}
// A constant allows you to change the value easily.
private const float ProximityLimit = 1.5f;
private Proximity DistanceFromObject()
{
_RightRayCast = Physics2D.Raycast(transform.position, Vector2.right);
_LeftRayCast = Physics2D.Raycast(transform.position, Vector2.left);
if (_RightRayCast.distance < ProximityLimit && _LeftRayCast.distance < ProximityLimit)
return Proximity.LeftAndRight;
if (_RightRayCast.distance < ProximityLimit)
return Proximity.Right;
if (_LeftRayCast.distance < ProximityLimit)
return Proximity.Left;
return Proximity.None;
}
The Evade method becomes easier to read.
private Proximity Evade(float _evadeSpeed)
{
if (Input.GetKeyDown(KeyCode.Space) && _IsGrounded) {
Proximity proximity = DistanceFromObject();
switch (proximity) {
case Proximity.Right:
_RigidBody.AddForce(Vector2.up * _evadeSpeed, ForceMode2D.Impulse);
_RigidBody.AddForce(Vector2.left * _evadeSpeed, ForceMode2D.Impulse);
break;
case Proximity.Left:
_RigidBody.AddForce(Vector2.up * _evadeSpeed, ForceMode2D.Impulse);
_RigidBody.AddForce(Vector2.right * _evadeSpeed, ForceMode2D.Impulse);
break;
case Proximity.LeftAndRight:
_RigidBody.AddForce(Vector2.up * _evadeSpeed * 3, ForceMode2D.Impulse);
break;
}
return proximity;
}
return Proximity.None;
}
Corresponding refactorings can be applied to Attack and Jump
In places where Unity requires you to use an int (I’m not a Unity developer), at least use constants.
const int NoProximity = 0, RightProximity = 1, ...;
Disclaimer: I’m not a Unity coder so this is C#-focused feedback, Unity might change some things or have different conventions. Apologies if I cause confusion as a result.
Prefer properties over fields and prefer encapsulation (i.e avoid public setters), for example public float Health { get; private set; }. Currently, any other code in your game can set the Health to whatever it wants, when it should be calling DamageDealt() and letting the object work out its own health!
The private method Start() appears not to be called at all, though perhaps Unity works some reflection magic to call it? In any case, prefer inline initialization, for example private Rigidbody2D _RigidBody = GetComponent<Rigidbody2D>();. This may not always be appropriate (e.g. if members need different starting values depending how they are constructed) but used properly means the object can immediately be in a valid state as soon as it is created.
Avoid “magic numbers” – another answer has already mentioned enums over ints, but the same applies for other constants. For example private const float MaxHealth = 100f; then later Health = MaxHealth; gives more meaning to what’s going on. It also means that if the same value is used in multiple places (such as the repeated appearance of 1.5f in DistanceFromObject and Attack) and you later decide it needs to change, you only need to change it in one place if you have private const float CheckDistance = 1.5f; – otherwise, can you guarantee you wouldn’t miss changing it somewhere? Finally, it gives you the flexibility in future to change constants to variables – perhaps you decide your game has Easy/Normal/Hard difficulty levels, you could make MaxHealth a static variable and change how much health the player starts with, for example.
Personally I’d put MoveRight() and MoveLeft() into a single Move() method. It’d be a bit clearer that way, and would mean Defend need only be called once, and the result stored. (It’s currently possible for it to be called by both methods, so the DefendCooldownTimer will be incremented twice if left arrow, right arrow, and space are all pressed. Is that intentional?) In fact I wonder if it might be appropriate for all the code that checks input to be in the same method, because it checks for different key presses in different places at the moment and that’s not very clear. However, there’s a danger that approach could end up with a giant method that tries to do everything, which would be just as bad (and which you currently avoid).