5
\$\begingroup\$

I am writing code for games in Unity. I should write the part very often and would like to know the flaws of the piece of code. It seems like everything is fine, but nevertheless I feel that some parts may be wrong. I am interested whether my style of coding is good or bad and would like to hear from someone who see my code for the first time whether he or she understands it or not.

Here is the code:

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

public class MovementController : MonoBehaviour
{
    private enum Turned
    {left, right};
    private Turned turned;

    public float walkingSpeed, runningSpeed, currentSpeed;
    public Transform playerTransform;
    public PlayerAnimationController playerAnimationController;
    public SpriteRenderer playerSpriteRenderer;

    void Start ()
    {
        initializeNeededForMovementVariables();
    }

    private void initializeNeededForMovementVariables()
    {
        turned = Turned.left;
    }

    void Update ()
    {
        moveHorizontallyAndAnimateMovementIfNecessary();

    }

    private void moveHorizontallyAndAnimateMovementIfNecessary()
    {
        float moveHorizontallyValue = Input.GetAxis("Horizontal");
        if (moveHorizontallyValue == 0)
        {
            playAnimationOfBeingIdle();
            return;
        }

        if (Input.GetKey(KeyCode.LeftShift) || Input.GetKey(KeyCode.RightShift))
        { currentSpeed = runningSpeed; }
        else
        { currentSpeed = walkingSpeed; }

        conductHorizontalTranslationOfThePlayer(moveHorizontallyValue);
        flipFacingIfNecessary(moveHorizontallyValue);
        playAnimationOfHorizontalMovement();
    }

    private void playAnimationOfBeingIdle()
    {
        playerAnimationController.beingOnlyIdling();
    }

    private void playAnimationOfHorizontalMovement()
    {
        if (currentSpeed == walkingSpeed)
        { playerAnimationController.beingOnlyWalking(); }
        else if(currentSpeed == runningSpeed)
        { playerAnimationController.beingOnlyRunning(); }
    }

    private void flipFacingIfNecessary(float moveHorizontallyValue)
    {
        if (moveHorizontallyValue > 0 && turned == Turned.left)
        {
            toggleFlipXOnPlayerRenderer();
            turned = Turned.right;
        }
        else if(moveHorizontallyValue < 0 && turned == Turned.right)
        {
            toggleFlipXOnPlayerRenderer();
            turned = Turned.left;
        }
    }

    private void toggleFlipXOnPlayerRenderer()
    {
        playerSpriteRenderer.flipX = !playerSpriteRenderer.flipX;
    }

    private void conductHorizontalTranslationOfThePlayer(float moveHorizontallyValue)
    {
        transform.Translate(moveHorizontallyValue * currentSpeed, 0, 0);
    }
}

Will appreciate any attention and comments. If this question is not appropriate for the site and should be in game overflow, please, move it there.

\$\endgroup\$
  • \$\begingroup\$ 1) There is no Game Overflow. 2) Did you read the help center? 3) Are you aware one of the first Unity tutorials deals specifically with how to write a program for a movement controller? \$\endgroup\$ – Mast Jun 17 '17 at 16:46
  • \$\begingroup\$ @Mast, could you provide me with the link for that tutorial, please? \$\endgroup\$ – Yaroslav Jun 18 '17 at 8:21
  • \$\begingroup\$ Roll-a-ball - Moving the Player. That entire tutorial is a good one to finish before doing anything else in Unity, it contains some really basic stuff. \$\endgroup\$ – Mast Jun 18 '17 at 9:44
1
\$\begingroup\$

First of all, I didn't like the long method names you used in the script. like:

void Start ()
{
    initializeNeededForMovementVariables();
}

I mean, why did you need to call it like that? In my opinion this makes the code very messy once you have a lot of code in a script. I would had call it something like: "InitVariables" or simply "Init"

Again,

float moveHorizontallyValue = Input.GetAxis("Horizontal");

The same point applies here too. You could have just written:

float h = Input.GetAxis("Horizontal");

Another thing: Why are you doing the following block?

private void playAnimationOfHorizontalMovement()
{
    if (currentSpeed == walkingSpeed)
    { playerAnimationController.beingOnlyWalking(); }
    else if(currentSpeed == runningSpeed)
    { playerAnimationController.beingOnlyRunning(); }
}

You have all this code to play animations. I suppose you are using other scripts too, and again, this is my opinion, it makes everything not totally understandable.

Then, Why aren't using an animator?

The code would be A LOT less and also A LOT more understandable, plus WAY EASIER to code!

Now, don't worry if your code isn't perfect now, you'll get better at it. It requires time. I really hope this helps you, Let me know!

Cheers

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.