Class to move a sprite in Unity

Question

I first got introduced to coding with Ruby and now I am learning how to create games in C# with Unity.

I am following a tutorial and I refactored the code to make it easier for me to grasp. I am really curious to know the opinion of more advanced programmers on the following code.

Here is the initial code:

using UnityEngine;
using System.Collections;

public class CharacterMovement : MonoBehaviour{
  private Rigidbody2D playerRigidBody2D;

  private float movePlayerHorizontal;
  private float movePlayerVertical;
  private Vector2 movement;

  public float speed = 4.0f;

  private Animator playerAnim;

  private SpriteRenderer playerSpriteImage;

  void Awake(){
    playerRigidBody2D = (Rigidbody2D)GetComponent(typeof(Rigidbody2D));
    playerAnim=(Animator)GetComponent(typeof(Animator));
    playerSpriteImage=(SpriteRenderer)GetComponent(typeof(SpriteRenderer));
  }

  void Update () {
    movePlayerHorizontal = Input.GetAxis("Horizontal");
    movePlayerVertical = Input.GetAxis("Vertical");
    movement=new Vector2(movePlayerHorizontal,movePlayerVertical);

    playerRigidBody2D.velocity=movement*speed;

    if(movePlayerVertical==0 && movePlayerHorizontal==0){
      playerAnim.SetBool("moving",false);
    }else{
      playerAnim.SetBool("moving",true);
    }

    if(movePlayerVertical!=0){
      playerAnim.SetBool("xMove",false);
      playerSpriteImage.flipX=false;

      if(movePlayerVertical>0){
        playerAnim.SetInteger("yMove",1);

      }else if(movePlayerVertical<0){
        playerAnim.SetInteger("yMove",-1);

      }
    }else {
      playerAnim.SetInteger("yMove",0);

      if(movePlayerHorizontal>0){
        playerAnim.SetBool("xMove",true);
        playerSpriteImage.flipX=false;

      }else if(movePlayerHorizontal<0){
        playerAnim.SetBool("xMove",true);
        playerSpriteImage.flipX=true;

      }else{
        playerAnim.SetBool("xMove",false);
      }
    }
  }
}

All the logic is placed inside of the #Update method, and there are no private methods to help self-document/declutter it.

This is how I refactored the code:

using System.Collections;
using UnityEngine;

public class CharacterMovement : MonoBehaviour {
  private Rigidbody2D _playerRigidbody2D;
  private Animator _playerAnimator;
  private SpriteRenderer _playerSpriteRenderer;

  private float movePlayerHorizontal;
  private float movePlayerVertical;
  private Vector2 movement;

  public float speed = 4.0f;

  void Awake() {
    _playerRigidbody2D    =    (Rigidbody2D)GetComponent(typeof(Rigidbody2D));
    _playerAnimator       =       (Animator)GetComponent(typeof(Animator));
    _playerSpriteRenderer = (SpriteRenderer)GetComponent(typeof(SpriteRenderer));
  }

  void Update () {
    movePlayer();
    detectMovementDirection();
  }

  private void detectMovementDirection() {
    if(isMovingVertical()) {
      moveVertical();
    } else {
      moveHorizontal();
    }
  }

  private void moveHorizontal(){
    _playerAnimator.SetInteger("yMove", 0);
    if (isMovingRight()) {
      _playerAnimator.SetBool("xMove", true);
      _playerSpriteRenderer.flipX = false;
    } else if (isMovingLeft()) {
      _playerAnimator.SetBool("xMove", true);
      _playerSpriteRenderer.flipX = true;
    } else {
      _playerAnimator.SetBool("xMove", false);
    }
  }

  private void moveVertical(){
    _playerAnimator.SetBool("xMove", false);
    _playerSpriteRenderer.flipX = false;
    if(isMovingUp()) {
      _playerAnimator.SetInteger("yMove", 1);
    } else if (isMovingDown()) {
      _playerAnimator.SetInteger("yMove", -1);
    }
  }

  private bool isMovingVertical() {
    return movePlayerVertical != 0;
  }

  private bool isMovingUp() {
    return movePlayerVertical > 0;
  }

  private bool isMovingDown() {
    return movePlayerVertical < 0;
  }

  private bool isMovingRight() {
    return movePlayerHorizontal > 0;
  }

  private bool isMovingLeft() {
    return movePlayerHorizontal < 0;
  }

  private void movePlayer(){
    movePlayerHorizontal = Input.GetAxis("Horizontal");
    movePlayerVertical = Input.GetAxis("Vertical");
    movement = new Vector2(movePlayerHorizontal, movePlayerVertical);
    _playerRigidbody2D.velocity = movement * speed;
  }
}

The code is exactly the same, but I broke it up in smaller methods(a la Ruby) and the #Update method, now consists of only 2 calls which in my opinion, makes the code easier to reason about.

Is there an experienced C# programmer that can enlighten me if 1 of the coding style is better and if so why?

Answer 1

First thing that catches my eye is that you are determining if you are moving in direction by testing your variable value against 0. You shouldn't use 0 for that as short movements combined with some velocity calculations can make it seems like you are moving when you are not. Instead test it against 0.1.

There are few problems with your 2 main methods - movePlayer && detectMovementDirection.

• You need to call 2 different methods in order for your character movement and animation to play. This is weird because you can call movePlayer without invoking the later which will result in movement without animation. You're on the right track, they do 2 different things and that's why they should be in 2 different methods, but they are chained together. You cant just play movement animations without moving the actual character and vice-versa. I would suggest to call detectMovementDirection at the end of your movePlayer method like this:

  private void movePlayer()
  {
      movePlayerHorizontal = Input.GetAxis("Horizontal");
      movePlayerVertical = Input.GetAxis("Vertical");
      movement = new Vector2(movePlayerHorizontal, movePlayerVertical);
      _playerRigidbody2D.velocity = movement * speed;
      detectMovementDirection();
  }
  

• detectMovementDirection should be called something like PlayMovementAnimation.

Speaking of detectMovementDirection, you shouldn't have all those 1 liners and in fact you don't need to know exactly where your character is moving.

Let's start by simplifying your moveVertical method.

Take a look at this if statement:

if (isMovingUp())
{
    _playerAnimator.SetInteger("yMove", 1);
}
else if (isMovingDown())
{
    _playerAnimator.SetInteger("yMove", -1);
}

We can see that there is just a small difference between the 2 bodies and that's just the number 1 and -1. Let's extract some information:

We know that that the range of movePlayerVertical is [-1, 1].

if (isMovingUp()) will be triggered only when movePlayerVertical > 0.

else if (isMovingDown()) will be triggered only when movePlayerVertical < 0.

With that information we can see that we move up when movePlayerVertical is positive and move down when it's negative. Well that's pretty good news because they are matching with what value we give to _playerAnimator.SetInteger("yMove", x);.

Well what if we round up the value of movePlayerVertical always to the nearest integer like this?

_playerAnimator.SetInteger("yMove", Mathf.RoundToInt(movePlayerVertical));

With this line we can remove 2 of your methods reduce the size of your moveVertical method.

private void moveVertical()
{
    _playerAnimator.SetBool("xMove", false);
    _playerSpriteRenderer.flipX = false;
    _playerAnimator.SetInteger("yMove", Mathf.RoundToInt(movePlayerVertical));
}

Let's inspect the moveHorizontal() method.

private void moveHorizontal()
{
    _playerAnimator.SetInteger("yMove", 0);
    if (isMovingRight())
    {
        _playerAnimator.SetBool("xMove", true);
        _playerSpriteRenderer.flipX = false;
    }
    else if (isMovingLeft())
    {
        _playerAnimator.SetBool("xMove", true);
        _playerSpriteRenderer.flipX = true;
    }
    else
    {
        _playerAnimator.SetBool("xMove", false);
    }
}

Take a look at the else statement here. When will this be triggered? Only when the previous 2 statements are wrong, let's put in a different way:

It will always happen unless there is something to negate it.

This means we can do this:

_playerAnimator.SetBool("xMove", false);
if (isMovingRight())
{
    _playerAnimator.SetBool("xMove", true);
    _playerSpriteRenderer.flipX = false;
}
else if (isMovingLeft())
{
    _playerAnimator.SetBool("xMove", true);
    _playerSpriteRenderer.flipX = true;
}

But we can shorten that even further, xMove has value true only if one of the 2 statements is correct, in all the rest cases it's false. We can do this:

_playerAnimator.SetBool("xMove", isMovingRight() || isMovingLeft());

Which makes our if statements seems pretty empty as they have only 1 line - _playerSpriteRenderer.flipX = ???;. Can we shorten this? Of course!

_playerSpriteRenderer.flipX = isMovingLeft();

This reduces your method to just 3 lines:

private void moveHorizontal()
{
    _playerAnimator.SetInteger("yMove", 0);
    _playerAnimator.SetBool("xMove", isMovingRight() || isMovingLeft());
    _playerSpriteRenderer.flipX = isMovingLeft();
}

Next we can simplify your Awake method.

void Awake()
{
    _playerRigidbody2D = (Rigidbody2D) GetComponent(typeof(Rigidbody2D));
    _playerAnimator = (Animator) GetComponent(typeof(Animator));
    _playerSpriteRenderer = (SpriteRenderer) GetComponent(typeof(SpriteRenderer));
}

There are 2 problems here:

1. It's ugly.
2. It involves unboxing.

We can fix those 2 problems easily using generics:

void Awake()
{
    _playerRigidbody2D = GetComponent<Rigidbody2D>();
    _playerAnimator = GetComponent<Animator>();
    _playerSpriteRenderer = GetComponent<SpriteRenderer>();
}

You're not really following the C# naming conventions but I will leave that for someone else to comment on.