2
\$\begingroup\$

I've been messing around with some stuff in Unity and are now working with 5 classes in my project. As I am very new to the engine, and even C#, I believe I made a rather messy code, even though it works as desired.

I would love your guys personal opinions on how to clean up and organize my code, which I hope is okay to post about here.

I am currently for the experience of it, creating a 2D game using 3D objects:

screenshot

CollisionScript.cs:

using UnityEngine;
using System.Collections;
using UnityEngine.UI;

public class CollisionScript : MonoBehaviour {
    public int scorePoints;
    public int healthAmount;
    public Text scoreUI;
    public Text health;
    public GameObject points;
    public GameObject player;
    public Camera mainCamera;
    public GameObject obstacle;
    public GameObject obstacle1;
    Vector3 randomPos;
    Vector3 randomPos2;
    Vector3 randomPos3;
    float speed = 1.0f;
    float obstacleSpeedX = 1.5f;
    float obstacleSpeedY = 1.5f;

    public void RandomFunction() {
        float screenX = Random.Range(2.0f, mainCamera.pixelWidth - 2.0f);
        float screenY = Random.Range(2.0f, mainCamera.pixelHeight - 2.0f);
        float screenZ = 10.0f;
        Vector3 position = mainCamera.ScreenToWorldPoint(new Vector3(screenX, screenY, screenZ));

        float screenX2 = Random.Range(4.0f, mainCamera.pixelWidth - 4.0f);
        float screenY2 = Random.Range(4.0f, mainCamera.pixelHeight - 4.0f);
        float screenZ2 = 10.0f;
        Vector3 position2 = mainCamera.ScreenToWorldPoint(new Vector3(screenX2, screenY2, screenZ2));

        float screenX3 = Random.Range(4.0f, mainCamera.pixelWidth - 4.0f);
        float screenY3 = Random.Range(4.0f, mainCamera.pixelHeight - 4.0f);
        float screenZ3 = 10.0f;
        Vector3 position3 = mainCamera.ScreenToWorldPoint(new Vector3(screenX3, screenY3, screenZ3));

        randomPos2 = position2;

        if (scorePoints == 1) {
            randomPos = position;
            randomPos3 = position3;
        }
    }

    void Start() {
        obstacle.SetActive (false);
        obstacle1.SetActive (false);
    }

    void Update() {
        if (scorePoints == 5) {
            obstacle.SetActive (true);
            obstacle.transform.position = randomPos;
        } else if (scorePoints == 10) {
            obstacle1.SetActive (true);
            obstacle1.transform.position = randomPos3;
        } else if (scorePoints >= 15) {
            obstacle.transform.position += Vector3.right * obstacleSpeedX * Time.deltaTime;
            obstacle1.transform.position += Vector3.up * obstacleSpeedY * Time.deltaTime;
        }

        if (obstacle.transform.position.x <= -8.9f) {
            obstacleSpeedX = 1.5f;
        }

        if (obstacle.transform.position.x >= 8.9f) {
            obstacleSpeedX = -1.5f;
        }

        if (obstacle1.transform.position.y <= -5.0f) {
            obstacleSpeedY = 1.5f;
        } 

        if (obstacle1.transform.position.y >= 5.0f) {
            obstacleSpeedY = -1.5f;
        }

    }

    public void pointPosition() {
        points.transform.position = randomPos2;
    }
}

LethalCollisionScript.cs:

using UnityEngine;
using System.Collections;
using UnityEngine.UI;

public class LethalCollisionScript : CollisionScript {

    void OnTriggerEnter2D(Collider2D coll) {
        healthAmount -= 1;
        player.transform.position = new Vector3(0,0,0);

        if (healthAmount >= 0) {
            health.text = "HEALTH: " + healthAmount;
        }

        if (healthAmount <= -1) {
            Application.LoadLevel (0);
        }
    }
}

ObstacleScript.cs:

using UnityEngine;
using System.Collections;

public class ObstacleScript : MonoBehaviour {

    // Update is called once per frame
    void Update () {
        transform.Rotate (new Vector3 (0, 0, 90) * Time.deltaTime * 2);
    }
}

PlayerScript.cs:

using UnityEngine;
using System.Collections;

public class PlayerScript : MonoBehaviour {

    public float speed = 1.0f;
    public Camera mainCameraPS;

    // Use this for initialization
    void Start () {

    }

    // Update is called once per frame
    void Update () {
        if (Input.GetKey (KeyCode.LeftArrow)) {
            transform.position += Vector3.left * speed * Time.deltaTime;
        }
        if (Input.GetKey (KeyCode.RightArrow)) {
            transform.position += Vector3.right * speed * Time.deltaTime;
        }
        if (Input.GetKey (KeyCode.UpArrow)) {
            transform.position += Vector3.up * speed * Time.deltaTime;
        }
        if (Input.GetKey (KeyCode.DownArrow)) {
            transform.position += Vector3.down * speed * Time.deltaTime;
        }

        if (transform.position.x <= -8.4f)
            transform.position = new Vector3 (-8.4f, transform.position.y, transform.position.z);
        else if (transform.position.x >= 8.4f)
            transform.position = new Vector3(8.4f, transform.position.y, transform.position.z);

        if (transform.position.y <= -4.6f)
            transform.position = new Vector3 (transform.position.x, -4.6f, transform.position.z);
        else if (transform.position.y >= 4.6f)
            transform.position = new Vector3 (transform.position.x, 4.6f, transform.position.z);
    }
}

PointCollisionScript.cs:

using UnityEngine;
using System.Collections;

public class PointCollisionScript : CollisionScript {

    void OnTriggerEnter2D(Collider2D coll) {

        RandomFunction ();
        pointPosition ();

        scorePoints += 1;
        scoreUI.text = "SCORE: " + scorePoints.ToString ();
    }
}

My main concern is regarding when extending classes, it's required I have to pull down objects into the script as public variables on EVERY objects I used a sub-class or main class on. (There must be a better way).

I'm also almost certain I should divide my code into even more classes for organizing and maybe create some sort of manager for my "global" variables?

\$\endgroup\$
3
\$\begingroup\$

CollisionScript.cs

  • Remove speed and other unused fields.
  • Place empty line between public and private fields, place empty lines between groups of fields. Programming is 80% reading code, so squeeze maximum readability from your code by taking seriously formatting, code style...
  • ... and naming:
    • Field names could be clearer. For example, scoreUIscoreText and healthhealthText.
    • RandomFunction() says nothing about it (except that it uses random). It should be GenerateRandomPositions() or something like that.
    • The class itself doesn't seem to have a depictive name. It should be something like CollistionHandlerBase.
    • pointPosition()PutPointAtPosition().
  • No need to maintain public Camera mainCamera field, use Camera.main instead.
  • Methods are rather big, but I don't think it's worth it refactor them, because game logic will change many times in near future. Instead, make them at least 2x times for understandable.
  • Use var keywords whenever possible.

LethalCollisionScript.cs, ObstacleScript.cs, PointCollisionScript.cs

  • Rename the class to LethalCollisionHandler, etc.

PlayerScript.cs

  • Managable enough messinness level for now.
  • Rename to something like PlayerCharacter.

Overall

  1. After you're done with things above, you can start more global refactoring. It's quite brain-heavy job, don't do it until unused variables and bad names are stopped from obscuring the picture.
  2. Take each class and analyze what it does overall. Separate most obvious/big/stable responsibilities to new classes. Move UI-related logic to separate class, etc.

The main concern

This problem is usually called “dependency management”. And it's a probably biggest pain of Unity game programmers. Different approaches are equally possible, but you should use not more than couple of top simplest among them for now.

  • Expose fields to editor only when nesseccary. Obtain as much as possible in code logic. (Use GetComponent<Desired_type_here>(), etc.)
  • Tag the player GameObject with built-in “Player” tag. Get player in Start() methods using GameObject.FindWithTag("Player");
  • Create GameController MonoBehaviour. Keep common dependencies there. Other scripts can get needed data from it. Tag it with built-in “GameController” tag.
  • Game Conroller acts as director above all other scripts, but keep its logic minimalistic. Don't add any logic to it without neccessity, even “for 5 minutes”, or you'll lose control over it, and it'll bloat.

More or less like this:

public class GameController : MonoBehaviour
{
    public GameObject Player;
    public UIManager UIManager;

    void Awake()
    {
        Player = GameObject.FindWithTag("Player");
        UIManager = GameObject.FindObjectOfType<UIManager>();
    }
}

Then:

public class Some_of_Game_Scripts : MonoBehaviour
{
    GameController gameController;
    UIManager uiManager;

    void Start()
    {
        gameController = GameObject.FindWithTag("GameController");
        uiManager = gameController.UIManager;
    }

    void Update()
    {
        uiManager.SetStatusText_Or_Something("Some text");
        if (something)
            gameController.ResetGame();
    }
}
\$\endgroup\$
  • \$\begingroup\$ Great answer, helps me a lot! :-) Haven't got to tagging until this very weekend and I now see how much it does! :-) \$\endgroup\$ – FrederikStenberg Mar 7 '16 at 12:31

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.