What do you think of my code?

News & General Discussion
,
1

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using UnityEngine.UI;
using TMPro;
public class ResourceManager : MonoBehaviour
{
// setting all resources
private float resourceTroops;
private float resourceArmour;
private float resourceAircraft;
private float resourceVL;
//remember to put victory resource
public TextMeshProUGUI troopText;
public TextMeshProUGUI ArmourText;
public TextMeshProUGUI AircraftText;
// the following sets popups
private bool changeResources;

private bool _toggleDefPopup;
public GameObject defaultPopup;
public GameObject negPopup;
public GameObject posPopup;
public GameObject notPopup;
public GameObject vicPopup;
public GameObject losPopup;
public GameObject imagePopup;



private bool _toggleCommanderPopup;
public GameObject HeinzG;
public GameObject OttoD;
public GameObject ErichVM;

private bool _togglePlatforms;

// this is victory/lose gameobjects
public GameObject vic1;
public GameObject vic2;
public GameObject vic3;
public GameObject vic4;
public GameObject vic5;
public GameObject los1;
public GameObject los2;
public GameObject los3;
public GameObject los4;
public GameObject los5;

private int digit;

// images list
public List<GameObject> images;
public int imageSelector;
private int randomValue;

void Start() {
    resourceTroops = 100;
    resourceArmour = 15;
    resourceAircraft = 120;
    troopText.text = ""+ resourceTroops;
    ArmourText.text = "" + resourceArmour;
    AircraftText.text = "" + resourceAircraft;

    resourceVL = 0;
}
/*
// these will be parametres for loss atk
int armourAL = 12;
int troopAL = 80;
// these will be parametres for win atk
int armourAW = 8;
int troopAW = 50;

// these will be parametres for win reorg
int troopR = 220;
int armourR = 30;
int aircraftR = 50;

// these will be parametres for loss reorg
int troopRL = 20;
int armourRL = 5;
int aircraftRL = 10;
*/
[SerializeField]
private void Update()
{
    // this is victory/lose gameobject

    if (resourceVL >= 5) {
        vic1.SetActive(true);
        los1.SetActive(false);
        los2.SetActive(false);
        los3.SetActive(false);
        los4.SetActive(false);
        if (resourceVL >= 10) {
            vic2.SetActive(true);
            if (resourceVL >= 15) {
                vic3.SetActive(true);
                    if (resourceVL >= 20) {
                        vic4.SetActive(true);
                        if (resourceVL >= 25) {
                            vic5.SetActive(true);
                            vicPopup.SetActive(true);
                            defaultPopup.SetActive(true);
                        }
                    }
            }
        }
    } else {
        if (resourceVL <= -5) {
            los1.SetActive(true);

            vic1.SetActive(false);
            vic2.SetActive(false);
            vic3.SetActive(false);
            vic4.SetActive(false);
            if (resourceVL <= -10) {
                los2.SetActive(true);
                if (resourceVL <= -15) {
                    los3.SetActive(true);
                        if (resourceVL <= -20) {
                            los4.SetActive(true);
                            if (resourceVL <= -25) {
                                los5.SetActive(true);
                                losPopup.SetActive(true);
                                defaultPopup.SetActive(true);
                            }
                        }
                    }
                }
            }
        }

    // this is for attack popups
    

    if(Input.GetKeyDown(KeyCode.A))
    {
        changeResources = !changeResources;
        
        defaultPopup.SetActive(true);
        digit = Random.Range(0, 101);
        if (resourceTroops >= 50) {
            // images
            imagePopup.SetActive(true);
            int randomValue = Random.Range(0, images.Count);
            int imageSelector = randomValue;
            GameObject selectedimage = images[imageSelector];
            selectedimage.SetActive(true);

            if(digit <= 50/(resourceArmour*0.2)){
                // play negative screen and resource loss
            
                negPopup.SetActive(true);
                resourceArmour = resourceArmour*0.4f;
                resourceTroops = resourceTroops*0.4f;
                ArmourText.text = "" + (resourceArmour);
                troopText.text = "" + (resourceTroops);
                resourceVL = resourceVL - 5;
     
            } else {
                // play positive screen and resource loss
                posPopup.SetActive(true);
                resourceArmour = resourceArmour*0.5f;
                resourceTroops = resourceTroops*0.5f;
                ArmourText.text = "" + (resourceArmour);
                troopText.text = "" + (resourceTroops);
                resourceVL = resourceVL + 3;
            }
        } else {
            notPopup.SetActive(true);
        }
    }
    if(Input.GetKeyDown(KeyCode.Return))
    {
        changeResources = !changeResources;
        
        posPopup.SetActive(false);
        negPopup.SetActive(false);
        notPopup.SetActive(false);
        vicPopup.SetActive(false);
        losPopup.SetActive(false);
        imagePopup.SetActive(false);
    }
    
    // this is for reorgansise popup

    if(Input.GetKeyDown(KeyCode.R))
    {
        changeResources = !changeResources;
        
        defaultPopup.SetActive(true);
        digit = Random.Range(0, 101);
        if(digit <= 70){
            // play negative screen and resource loss
            negPopup.SetActive(true);
            resourceArmour = resourceArmour + 5;
            resourceTroops = resourceTroops + 50;
            resourceAircraft = resourceAircraft + 10;

            ArmourText.text = "" + (resourceArmour);
            troopText.text = "" + (resourceTroops);
            AircraftText.text = "" + (resourceAircraft);
            resourceVL = resourceVL - 1;
        } else {
            // play positive screen and resource loss
            posPopup.SetActive(true);
            resourceArmour = resourceArmour + 15;
            resourceTroops = resourceTroops + 80;
            resourceAircraft = resourceAircraft + 40;
            ArmourText.text = "" + (resourceArmour);
            troopText.text = "" + (resourceTroops);
            AircraftText.text = "" + (resourceAircraft);
        }
    }

    // Air strike
    if(Input.GetKeyDown(KeyCode.S))
    {
        changeResources = !changeResources;
        
        defaultPopup.SetActive(true);
        digit = Random.Range(0, 101);
        if (resourceAircraft >= 60) {
            if(digit <= 50/(resourceAircraft*0.4)){
                // play negative screen and resource loss
            
                negPopup.SetActive(true);
                resourceAircraft = resourceAircraft*0.3f;
                AircraftText.text = "" + (resourceAircraft);
                resourceVL = resourceVL - 2;
            } else {
                // play positive screen and resource loss
                posPopup.SetActive(true);
                resourceAircraft = resourceAircraft*0.5f;
                AircraftText.text = "" + (resourceAircraft);
                resourceVL = resourceVL + 3;
            }
        } else {
            notPopup.SetActive(true);
        }
    }
    
}

}

2

I think you need to learn to use arrays/collections.

3

idk what it does, but way to many variables, calling that many isnt readable well, and isnt well optimized and will run slower then using an array like what spiney199 mentioned

4

I think your code is a tightly coupled nightmare with so much hard coding in it that it’s going to be a nightmare to deal with if you need to change anything.

For instance, this:

public GameObject HeinzG;
public GameObject OttoD;
public GameObject ErichVM;

Why are these hardcoded? Why are they GameObjects instead of classes specific to this unit type? What do these even do? They’re not referenced anywhere in your code. In fact, most of what’s in here isn’t referenced anywhere else. What is the purpose of this code? It seems like it’s responsible for a lot, but how it’s used is unclear at best.

5

It’s probably worth going over some tutorials or studying c# a bit more. I found the Unity tasks tutorial has a good overview.

Probably already been mentioned but this class is doing a bit much. You’ll find it can be easier to split the classes and use more lists. It looks like you’re familiar with lists as you use them for images. There are also a lot of magic numbers that you’ll want to avoid. For example if (resourceVL >= 10) { requires you to update the code. It might be better if you make these values public and maybe move them into a scriptable object. This way you can easily modify and swap them e.g. difficulty config changes resource levels for rankings.

Some of the code looks like it shows a star rating based on your resource level and triggers a victory popup at a high enough resource level. You could change it to something like:

    // display the number of gameobjects based on rating e.g. star rating 2/5
    public class Rating : MonoBehaviour 
    {
        public List<GameObject> ratings;
        public void SetRating(int rating)
        {
            rating = Mathf.Clamp(rating, 0, ratings.Count);
            for (var i = 0; i< rating; i++)
            {
                ratings[i].SetActive(i >= rating);
            }
        }
    }

    // data about resource ranks e.g. 10 => lv2, 15=>lv3, 20=>lv4, 25=>victory
    [CreateAssetMenu(fileName = "ResourceRankingAsset", menuName = "Scriptable Objects/ResourceRankingAsset")]
    public class ResourceRankingAsset : ScriptableObject
    {
        public int[] rankResourceLevels;
        public int GetRank(int resourceLevel) // ...
        public bool IsVictory(int resourceLevel) // ...
    }

    public class ResourceManager : MonoBehaviour
    {
        public VictoryPopup victoryPopup;
        public ResourceRankingAsset resourceRankingAsset;
        public Rating victoryRating;
        public Rating defeatRating;
        int resourceLevel;
        bool playing;

        public void SetResourceLevel(int level)
        {
            resourceLevel = level;
            victoryRating.SetRating(resourceRankingAsset.GetRank(level));
            if (resourceRankingAsset.IsVictory(level))
            {
                playing = false;
                victoryPopup.Show();
            }
            // ...
        }
    }