8
\$\begingroup\$

I did this game as part of a game jam and I was hoping I could get some pointers on how to improve my code. I feel like the code is sloppy but have no idea of the correct way to do something.

Whilst I understand a game jam isn't necessarily the place for refactoring I'd like to learn to make cleaner code and a better foundation for this and future projects.

I mean, somethings I'm doing without really understanding why (eg. static keyword -- something to do with instances not having their own variable?) but they work so I keep doing them. They'll probably form really bad habits so I'd like to nip them in the bud. I really appreciate any direction you can give me.

Here's my Scene setup to help visual the game.

enter image description here

And here are my classes (Please let me know if there's a better way to post this code):

Movement.cs -- I know that's a bad name

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

namespace Player
{
public class Movement : MonoBehaviour
{
    public GameObject [] MovementPoint;
    [Header ("Looting")]
    public float lootTime;
    public bool isFrozen;
    [Space (10)]
    [Header ("Audio Clips")]
    public AudioClip lootPickUp;
    public AudioClip bankPoints;
    public AudioClip move;
    [Space (10)]

    public GameObject iceberg;

    private Vector3 playerPos;
    private Vector3 playerScale;
    private int currentMovementPoint;
    private float timer;

    private int pointsToAdd;
    private ScoreKeeper scoreUI;
    private ScoreKeeper scoreKeeper;
    private GameManager gameManager;
    private static AudioSource audioSource;

    void Start ()
    {
        scoreKeeper = FindObjectOfType<ScoreKeeper> ();
        audioSource = GetComponent<AudioSource> ();
        gameManager = FindObjectOfType<GameManager> ();

        ResetPlayer (2);
    }

    public void ResetPlayer (int chosenSpawnPoint)
    {
        Debug.Log ("3");
        isFrozen = false;
        currentMovementPoint = chosenSpawnPoint;
        playerPos = MovementPoint [currentMovementPoint].transform.position;
        transform.position = playerPos;
        playerScale = transform.localScale;
    }

    void Update ()
    {
        if (!gameManager.isPaused && !isFrozen)
            MovePlayer ();
    }

    private void BankPoints ()
    {
        pointsToAdd = ScoreKeeper.bankedPoints;
        scoreKeeper.UpdateScoreText (pointsToAdd); // update score UI
        PlayAudioClip (lootPickUp);
    }


    private void MovePlayer ()
    {
        if (Input.GetKeyDown (KeyCode.RightArrow) && currentMovementPoint < MovementPoint.Length - 1)
        {
            currentMovementPoint++;
            playerPos = MovementPoint [currentMovementPoint].transform.position;
            transform.position = playerPos;
            playerScale.x *= -1;
            transform.localScale = playerScale;

            //CreateRandomPitch ();
            //audioSource.pitch = audioPitchRNG;
            PlayAudioClip (move);
        }
        if (Input.GetKeyDown (KeyCode.LeftArrow) && currentMovementPoint > 0)
        {
            currentMovementPoint--;
            playerPos = MovementPoint [currentMovementPoint].transform.position;
            transform.position = playerPos;
            playerScale.x *= -1;
            transform.localScale = playerScale;

            //CreateRandomPitch ();
            //audioSource.pitch = audioPitchRNG;
            PlayAudioClip (move);

        }
        if (currentMovementPoint == MovementPoint.Length - 1 && ScoreKeeper.bankedPoints > 0)
        {
            scoreKeeper.UpdateScoreText (ScoreKeeper.bankedPoints);
            ScoreKeeper.bankedPoints = 0;
            PlayAudioClip (bankPoints);
        }
    }


    public static void PlayAudioClip (AudioClip audioClip)
    {
        audioSource.clip = audioClip;
        audioSource.PlayOneShot (audioClip);
    }
}
}

ScoreKeeper.cs

using UnityEngine;
using UnityEngine.UI;

public class ScoreKeeper : MonoBehaviour
{
private Text scoreText;
private Spawner [] spawners;
public static int score;
public static int bankedPoints;

void Start ()
{
    scoreText = GetComponent<Text> ();
    spawners = FindObjectsOfType<Spawner> ();
    if (scoreText) Reset ();
    //UpdateScoreText (39);// Debug
}

public int BankPoints (int pointsToBank)
{
    bankedPoints += pointsToBank;
    return bankedPoints;
}

public void UpdateScoreText (int pointsToAdd)
{
    score += pointsToAdd;
    scoreText.text = score.ToString ();

    if (score > 10 && score / 10 >= Projectile.timeStep)
    {
        Projectile.timeBetweenMoving -= Projectile.speedIncrease;
        Projectile.timeStep++;

        foreach (Spawner spawner in spawners)
        {
            spawner.spawnRate -= 0.3f;
        }
    }
}

public void Reset ()
{
    if (scoreText)
    {
        score = 0;
        scoreText.text = score.ToString ();
    }
}
}

GameManager.cs

using System.Collections;
 using System.Collections.Generic;
 using UnityEngine;
 using UnityEngine.SceneManagement;
 using System;
 using UnityEngine.UI;

 public class GameManager : MonoBehaviour
{
public static int playerLives;
public bool isPaused;
public GameObject gameOverGraphic;
public GameObject livesText;
private Text livesTextComponent;

void Start ()
{
    SetUpFreshLevel ();
}


void Update ()
{
    if (Input.GetKeyDown (KeyCode.R))
    {
        ResetGame ();
    }

    if (Input.GetKeyDown (KeyCode.P))
    {
        PauseGame ();
    }

    if (playerLives == 0)
    {
        GameOver ();
    }
}

public void GameOver ()
{
    Time.timeScale = 0;
    gameOverGraphic.SetActive (true);
}

public void UpdateLives ()
{
    livesTextComponent.text = "LIVES: " + playerLives.ToString ();
}

void ResetGame ()
{
    SetUpFreshLevel ();
    SceneManager.LoadScene (SceneManager.GetActiveScene ().buildIndex);
}

private void SetUpFreshLevel ()
{
    Time.timeScale = 1;
    gameOverGraphic.SetActive (false);
    playerLives = 3;
    livesTextComponent = livesText.GetComponent<Text> ();
    UpdateLives ();
}

void PauseGame ()
{
    isPaused = !isPaused;

    if (isPaused)
    {
        Time.timeScale = 0;
    }
    else Time.timeScale = 1;
}
}

Spawner.cs (this whole class has to be reworked so I can regulate the spawns better so there aren't walls of snowflakes or empty gaps all the time just have no idea how to do that right now)

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

public class Spawner : MonoBehaviour
{
public float chanceForSpawn;
public float chanceForGoodSpawn;

public GameObject goodProjectile;
public GameObject badProjectile;

private float timer;
private float initialSpawnDelay;
public float spawnRate = 1.5f;
private bool canSpawn = false;

private void Start ()
{
    initialSpawnDelay = Random.Range (0, 1.0f);
}

void Update ()
{
    timer += Time.deltaTime;

    if (timer >= spawnRate)
    {
        StartCoroutine (BeginSpawnProcess (initialSpawnDelay));
        timer = 0;
    }
}

private IEnumerator BeginSpawnProcess (float waitTime)
{
    yield return new WaitForSeconds (waitTime);
    SpawnChance ();

    if (canSpawn)
    {
        GameObject projectileType = DecideProjectileType ();
        SpawnProjectile (projectileType);
        canSpawn = false;
    }
}

private bool SpawnChance ()
{
    float spawnRNG = Random.value;

    if (spawnRNG <= chanceForSpawn)
    {
        canSpawn = true;
    }
    else canSpawn = false;

    return canSpawn;
}

private GameObject DecideProjectileType ()
{
    float spawnRNG = Random.value;

    if (spawnRNG <= chanceForGoodSpawn)
    {
        return goodProjectile;
    }
    else
    {
        return badProjectile;
    }
}

private void SpawnProjectile (GameObject projectile)
{
    GameObject newProjectile = Instantiate (projectile, transform.position, Quaternion.identity) as GameObject;
    newProjectile.transform.parent = transform;
}

}

Projectile.cs

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

[RequireComponent (typeof (AudioSource))]
public class Projectile : MonoBehaviour
{
private int movementDistance = 2;
private float timer;
private int distanceMoved;
private int maxDistanceMoved = 4;
public static float timeBetweenMoving = 0.7f;
public static float speedIncrease = 0.15f;
public static int timeStep = 1;
public float points;

private Collider2D collider2D;
public Sprite collisionEffect;
private SpriteRenderer spriteRenderer;

public AudioClip pickupCollided;
public AudioClip pickupLost;
public AudioClip move;

private AudioSource audioSource;
private ScoreKeeper scoreKeeper;
private Movement player;
private GameManager gameManager;

//private Transform projectileRot;
private int [] startRot = { 0, 30, 60, 90, 120, 150, 180, 210, 240, 270, 300, 330 };

private void Awake ()
{
    transform.Rotate (0, 0, startRot [Random.Range (0, startRot.Length)]);
}

void Start ()
{
    gameManager = FindObjectOfType<GameManager> ();
    audioSource = GetComponent<AudioSource> ();
    scoreKeeper = FindObjectOfType<ScoreKeeper> ();
    spriteRenderer = GetComponent<SpriteRenderer> ();
    collider2D = GetComponent<Collider2D> ();
    player = FindObjectOfType<Movement> ();
    timer = 0f;
}

void Update ()
{
    timer += Time.deltaTime;
    if (timer > timeBetweenMoving)
    {
        Move ();
    }
    if (distanceMoved >= maxDistanceMoved)
        Destroy (gameObject);
}

private void Move ()
{
    transform.position += Vector3.down * movementDistance;
    timer = 0;
    distanceMoved++;

    transform.Rotate (0, 0, 45);
}

private void PlaySound (AudioClip clipToPlay)
{
    audioSource.clip = clipToPlay;
    audioSource.PlayOneShot (clipToPlay);
}

private void OnTriggerEnter2D (Collider2D other)
{
    if (other.gameObject.tag == "Player")
    {
        Movement.PlayAudioClip (pickupCollided);

        if (gameObject.tag == "BadProjectile")
        {
            if (GameManager.playerLives > 1)
            {
                Debug.Log ("1");
                ScoreKeeper.bankedPoints = 0;
                player.isFrozen = true;
                StartCoroutine (PlayerDeath ());
                GameManager.playerLives--;
                gameManager.UpdateLives ();
            }
            else
            {
                GameManager.playerLives--;
                player.isFrozen = true;
                gameManager.UpdateLives ();
                gameManager.GameOver ();
            }
        }
        else if (gameObject.tag == "GoodProjectile")
        {
            Destroy (gameObject, 0.3f);
            scoreKeeper.BankPoints (1);
            spriteRenderer.sprite = collisionEffect;
            transform.position += new Vector3 (0, 0.5f, 0);
            transform.rotation = Quaternion.identity;
            collider2D.enabled = false;
        }
    }
}
private IEnumerator PlayerDeath ()
{
    Debug.Log ("2");

    float respawnDelay = 1f;
    Time.timeScale = 0.1f;
    float pauseEndTime = Time.realtimeSinceStartup + respawnDelay;

    while (Time.realtimeSinceStartup < pauseEndTime)
    {
        yield return 0;
    }
    player.ResetPlayer (4);
    Time.timeScale = 1;
    Debug.Log ("4");
    Destroy (gameObject);
}
}
\$\endgroup\$
  • \$\begingroup\$ While not familiar with C# myself, you may be interested in using a linter to enforce consistent code style, as a generic piece of advice. A quick google search brings up two for C# (StyleCop, SonarLint), though they seem to be strictly Visual Studio integrations. Sorry I wasn't of much help beyond this. \$\endgroup\$ – JoshuaRLi Aug 8 '17 at 0:14
5
\$\begingroup\$

First of all, public member's name should be PascalCased. So you should rename things like these

public AudioClip lootPickUp;
public AudioClip bankPoints;
public AudioClip move;

to

public AudioClip LootPickUp;
public AudioClip BankPoints;
public AudioClip Move;

And it would be better to turn them into properties:

public AudioClip LootPickUp { get; set; }

PauseGame method not only pauses a game but also resumes it. So either give another name to the method or split it to two ones like PauseGame and ResumeGame.

Also this code

if (isPaused)
{
    Time.timeScale = 0;
}
else Time.timeScale = 1;

can be simplified to

Time.timeScale = isPaused ? 0 : 1;

Avoid repetitions of code. In MovePlayer method you have this fragment

playerPos = MovementPoint [currentMovementPoint].transform.position;
transform.position = playerPos;
playerScale.x *= -1;
transform.localScale = playerScale;

//CreateRandomPitch ();
//audioSource.pitch = audioPitchRNG;
PlayAudioClip (move);

repeated twice. Extract it to a method and call this method in your ifs.


SpawnChance method

private bool SpawnChance ()
{
    float spawnRNG = Random.value;

    if (spawnRNG <= chanceForSpawn)
    {
        canSpawn = true;
    }
    else canSpawn = false;

    return canSpawn;
}

can be rewritten as

private bool SpawnChance
{
    return Random.value <= chanceForSpawn;
}

as well as DecideProjectileType method.


Don't use spaces before opening brackets

public GameObject [] MovementPoint;
[Space (10)]
void Start ()
FindObjectOfType<ScoreKeeper> ();
ResetPlayer (2);

If you are adding something to string you don't need to call ToString since it will be called automatically. So this code

"LIVES: " + playerLives.ToString ();

can be rewritten as

"LIVES: " + playerLives;

It's often better to use semantically named methods instead of mathematical expressions. For example, this code

playerScale.x *= -1;

can be extracted to method like

private void TurnPlayerAround()
{
    playerScale.x *= -1;
}

and then be used every time you need to turn player around.


You should always avoid magic constants like 10 in this code

if (score > 10 && score / 10 >= Projectile.timeStep)

Define a constant

private const int TimeStepSize = 10;

and use it in your code

if (score > TimeStepSize && score / TimeStepSize >= Projectile.timeStep)

Also if timeStep is always a positive number you don't need the first condition (score > TimeStepSize)

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for taking the time to analyse my code. \$\endgroup\$ – Battle Cat Aug 9 '17 at 5:06
  • \$\begingroup\$ They are some great tips. MovePlayer was one I found last night and refactored out to remove duplicate so it's good to see I was on the right track there having read your comment now. Regarding the last point about timeStep it was because the TimeStepSize being changed before it hit a minimum threshold, which I didn't want. Eg. I only wanted it to start happening after it got to 10. Does that make sense, or does your point still apply? I just know that when I added that condition I got the outcome I was looking for. \$\endgroup\$ – Battle Cat Aug 9 '17 at 5:12
  • \$\begingroup\$ To clarify the timeStep thing further, it's basically the difficulty modifier that ramps up the speed for every 10 acorns the player banks. It's not implemented well because I couldn't figure out how to make it more robust in the time I had (ie scale better or on a more granular level rather than every 10). \$\endgroup\$ – Battle Cat Aug 9 '17 at 5:18
  • \$\begingroup\$ @BattleCat 10 (as other literals) is a magic constant. Looking at it some people will not understand what it means, some people will understand but after taking some time. So you should define named constant with this value. It will make the condition much more clear. It is especially important if you have the same value used multiple times across the code. Constant allows you to change value in one place only rather than go through all code and change values manually :) \$\endgroup\$ – Maxim Aug 9 '17 at 5:47
  • \$\begingroup\$ Hey @Maxim! I tried out your tip on condensing the bool SpawnChance but I realised that doesn't update the bool canSpawn anymore, which I check against in other parts. With your optimisation would I have to evaluate if(SpawnChance) each time instead? Also, I marked your reply as the answer since you were the only one kind enough to take the time to reply. :) \$\endgroup\$ – Battle Cat Aug 10 '17 at 6:22

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.