2
\$\begingroup\$

This is a Unity3D script I wrote for my game to control a character. I don't consider myself a highly-skilled coder, so I come here to ask for suggestions. Is my code clean enough? How can I improve my coding style and pattern?

using UnityEngine;

public enum OnWallStatus
{
    None = 0,
    OnLeft = 1,
    OnRight = 2
}

public enum OnGroundStatus
{
    OnGround = 0,
    InAir = 1
}

public class MoveControl : MonoBehaviour {


    private const float minSpeed = 1f;
    private const float maxSpeed = 5f;
    private const float minJump = 1f;
    private const float maxJump = 10f;


    [SerializeField]
    private float speedModifier;
    [SerializeField]
    private float jumpModifier;

    private new Rigidbody2D rigidbody2D;
    private new Transform transform;


    private OnGroundStatus _onGroundStatus;
    private OnWallStatus _onWallStatus;


    public OnGroundStatus onGroundStatus
    {
        get { return _onGroundStatus; }
    }

    public OnWallStatus onWallStatus
    {
        get { return _onWallStatus; }
    }

    private bool isHorizontalStill
    {
        get { return (rigidbody2D.velocity.x == 0); }
    }

    private bool isVerticalStill
    {
        get { return (rigidbody2D.velocity.y == 0); }
    }


    void Awake ()
    {
        speedModifier = Mathf.Clamp(speedModifier, minSpeed, maxSpeed);
        jumpModifier = Mathf.Clamp(jumpModifier, minJump, maxJump);

        rigidbody2D = GetComponent<Rigidbody2D>();
        transform = GetComponent<Transform>();

        _onGroundStatus = OnGroundStatus.OnGround;
        _onWallStatus = OnWallStatus.None;
    }


    public void MoveHorizontal(float speed)
    {
        speed = Mathf.Clamp(speed, -1f, 1f);
        rigidbody2D.AddForce(new Vector2(speed * speedModifier * 10f, 0f));

        Vector2 velocity = rigidbody2D.velocity;
        velocity.x = Mathf.Clamp(velocity.x, -1f * speedModifier, 1f * speedModifier);
        rigidbody2D.velocity = velocity;
    }


    public void MoveJump(float speed)
    {
        if (_onWallStatus == OnWallStatus.None && _onGroundStatus == OnGroundStatus.InAir)
            return;

        speed = Mathf.Clamp(speed, 0f, 1f);

        if (_onGroundStatus == OnGroundStatus.OnGround)
        {
            rigidbody2D.velocity = new Vector2(rigidbody2D.velocity.x, speed * jumpModifier);
        }
        else if(_onWallStatus == OnWallStatus.OnLeft)
        {
            rigidbody2D.velocity = new Vector2(1f * speedModifier, speed * jumpModifier);
        }
        else if (_onWallStatus == OnWallStatus.OnRight)
        {
            rigidbody2D.velocity = new Vector2(-1f * speedModifier, speed * jumpModifier);
        }
    }


    public void MoveLand(float speed)
    {
        if (_onWallStatus != OnWallStatus.None || _onGroundStatus != OnGroundStatus.InAir)
            return;

        speed = Mathf.Clamp(speed, -1f, 0f);
        rigidbody2D.velocity = new Vector2(rigidbody2D.velocity.x, speed * jumpModifier);
    }

    void OnCollisionEnter2D(Collision2D collision)
    {
        if (collision.gameObject.tag != "Platform")
            return;

        if (collision.contacts[0].normal.y > 0)
        {
            _onGroundStatus = OnGroundStatus.OnGround;
        }
        else
        {
            if(collision.contacts[0].normal.x < 0)
            {
                _onWallStatus = OnWallStatus.OnRight;
            }
            else
            {
                _onWallStatus = OnWallStatus.OnLeft;
            }
        }
    }

    void OnCollisionExit2D(Collision2D collision)
    {
        if (collision.gameObject.tag != "Platform")
            return;

        if (collision.contacts[0].normal.y > 0)
        {
            _onGroundStatus = OnGroundStatus.InAir;
        }
        else
        {
            _onWallStatus = OnWallStatus.None;
        }
    }

}
\$\endgroup\$
  • 1
    \$\begingroup\$ Just a few quick pointers - in C#, you usually use camelCase for private members and PascalCase for public members. However, since Unity uses camelCase for both, it's hard to recommend a consistent naming convention in regards to your private members and public properties that expose them (as opposed to private members that aren't exposed, which you name without an underscore). Finally, you should always use brackets on an if-statement. If not for readability, then for maintainability. Also, I'm assuming the excessive newlines are caused by copy-pasting. If not, you should use less. \$\endgroup\$ – Ivo Coumans Feb 3 '16 at 13:40
2
\$\begingroup\$

You don't need this directive in the code you have given us.

using System.Collections;

It's conventional to use Pascal casing to name private constants in a file, so

private const float minSpeed = 1f;

private const float maxSpeed = 5f;

private const float minJump = 1f;

private const float maxJump = 10f;

should be:

private const float MinSpeed = 1f;

private const float MaxSpeed = 5f;

private const float MinJump = 1f;

private const float MaxJump = 10f;

In C# it's convention that private instance fields start with an underscore.

[SerializeField]

private float _speedModifier;

[SerializeField]

private float _jumpModifier;

I would also consider writing 'getter' and 'setter' methods for each field instead of serializing them.


I believe public properties also use Pascal case conventionally.

public OnGroundStatus onGroundStatus

{

    get { return _onGroundStatus; }

}

public OnWallStatus onWallStatus

{

    get { return _onWallStatus; }

}

If you preferred you could use auto-properties to set these instead.

public OnGroundStatus OnGroundStatus { get; private set; }

public OnWallStatus OnWallStatus { get; private set; }

I don't see a usage of these properties in your code.

private bool isHorizontalStill

{

    get { return (rigidbody2D.velocity.x == 0); }

}

private bool isVerticalStill

{

    get { return (rigidbody2D.velocity.y == 0); }

}
| improve this answer | |
\$\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.