2D Unity game using 3D objects, gather points and avoid obstacles

Question

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?

Answer 1

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, scoreUI→scoreText and health→healthText.
  • 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();
 }
}

• FrederikStenberg: Great answer, helps me a lot! :-) Haven't got to tagging until this very weekend and I now see how much it does! :-)