Discussion Need help cleaning up code.

I currently have coded a basic script that I am using on multiple game objects in my project.
I want to be able to reference and change the variables for each individual game object (code below).
I also want to add even more ships in the future which will use the same script.

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4.  
5. public class ShipBehavior : MonoBehaviour
6. {
7.    
8.     //methods to change all variables
9.     public int getLives()
10.     {
11.         return ship_lives;
12.     }
13.  
14.     public void setLives(int value)
15.     {
16.         ship_lives = value;
17.     }
18.  
19.     public int getTotalDmg()
20.     {
21.         return totaldamage;
22.     }
23.  
24.     public void setTotalDmg(int value)
25.     {
26.         totaldamage=value;
27.     }
28.  
29.     public int getHealth()
30.     {
31.         return ship_lives;
32.     }
33.  
34.     public void setHealth(int value)
35.     {
36.         ship_lives = value;
37.     }
38.  
39.     public int getRegen()
40.     {
41.         return regen_lives;
42.     }
43.  
44.     public void setRegen(int value)
45.     {
46.         regen_lives = value;
47.     }
48.  
49.     public int getBaseDmg()
50.     {
51.         return dmg;
52.     }
53.  
54.     public void setBaseDmg(int value)
55.     {
56.         dmg = value;
57.     }
58.     ShipBehavior ship1_script;
59.     ShipBehavior ship2_script;
60.     private int ship_lives = 1000;
61.     private int regen_lives = 10;
62.     private int dmg = 20;
63.     private int totaldamage = 0;
64.     private bool dead = false;
65.  
66.     private void heal()
67.     {
68.         if(ship_lives>0)
69.             ship_lives += regen_lives;
70.     }
71.  
72.        
73.  
74.     // Start is called before the first frame update
75.     void Start()
76.     {
77.         ship1_script = GameObject.Find("ship1").GetComponent<ShipBehavior>();
78.         ship2_script = GameObject.Find("ship2").GetComponent<ShipBehavior>();
79.     }
80.  
81.     // Update is called once per frame
82.     void Update()
83.     {
84.         if (ship_lives <= 0)
85.         {
86.             dead = true;
87.         }
88.  
89.         //key events
90.         //used for mainly testing
91.         if (Input.GetKeyDown("q"))
92.         {
93.             ship1_script.setBaseDmg(ship1_script.getBaseDmg() + 1);
94.         }
95.  
96.         if (Input.GetKeyDown("a"))
97.         {
98.             ship1_script.setBaseDmg(ship1_script.getBaseDmg() - 1);
99.         }
100.  
101.         if (Input.GetKeyDown("w"))
102.         {
103.             ship1_script.setRegen(ship1_script.getRegen() + 1);
104.         }
105.  
106.         if (Input.GetKeyDown("s"))
107.         {
108.             ship1_script.setRegen(ship1_script.getRegen() - 1);
109.         }
110.  
111.         if (Input.GetKeyDown("e"))
112.         {
113.             ship2_script.setBaseDmg(ship2_script.getBaseDmg() + 1);
114.         }
115.  
116.         if (Input.GetKeyDown("d"))
117.         {
118.             ship2_script.setBaseDmg(ship2_script.getBaseDmg() - 1);
119.         }
120.  
121.         if (Input.GetKeyDown("r"))
122.         {
123.             ship2_script.setRegen(ship2_script.getRegen() + 1);
124.         }
125.  
126.         if (Input.GetKeyDown("f"))
127.         {
128.             ship2_script.setRegen(ship2_script.getRegen() - 1);
129.         }
130.     }
131.    
132.     //used to set up a Coroutine which deals with constant damage and hp changes to each ship
133.     void OnTriggerEnter(Collider other)
134.     {
135.         StartCoroutine(OnTrigger(other));
136.     }
137.  
138.     IEnumerator OnTrigger(Collider other)
139.     {
140.         //extremely messy code
141.         //I cannot just directly change the damage on this script
142.         //as it will affect both ships and not just one, the reason why there are 2 if statements and separate script objects for each one
143.  
144.         if (other.name == "ship2")
145.         {
146.             while (!dead&&ship2_script.getLives()>=0)
147.             {
148.                 yield return new WaitForSeconds(1f);
149.                 //adds randomization to damage output
150.                 int damage_output = dmg + Random.Range(1, 11);
151.                 //Debug.Log(damage_output+" done to ship2");
152.                 totaldamage += damage_output;
153.                 ship2_script.setLives(ship2_script.getLives() - damage_output);
154.                 heal();
155.             }
156.         }
157.         //same code as above just modified for ship1
158.         else if (other.name == "ship1")
159.         {
160.             while (!dead && ship1_script.getLives() >= 0)
161.             {
162.                 yield return new WaitForSeconds(1f);
163.                 int damage_output = dmg + Random.Range(1, 11);
164.                 totaldamage += damage_output;
165.                 //Debug.Log(damage_output + " done to ship1");
166.                 ship1_script.setLives(ship1_script.getLives() - damage_output);
167.                 heal();
168.             }
169.         }
170.     }
171. }

I believe that as I add more ships and more variables to this script, the script will become impossible to work with.
Any sort of help or advice on how to clean up the code and make it more scalable would be appreciated.
I also do not know if just 1 script is a good idea for so many game objects, so any sort of other approach to scripting or structuring the code would be appreciated.

Note: I do know about Scriptable Objects but I am unsure if creating many for each ship and referencing those values instead of keeping variables would be a better idea.

 

1. learn about collections (arrays, lists, etc.)

2. think in terms of Components that somethihg might have
- ability to spawn / die
- ability to maneuver
- take damage
- deal damage
- something else?

Compose your ships out of separate components. It's The Unity Way(tm).

They certainly could be used to configure a ship, when plugged into a MonoBehaviour on that ship.

 

Thank you for your response,
I am still a little unsure on how to format my code.

I am unsure what you mean when you talk about components, do you mean the different parts attached to gameObjects, or something else? I may be misunderstanding, but I do not believe there is a way to assign actual properties to the gameObjects themselves, without some sort of script taking those values as input.

Additionally, I am unsure if just 1 main script suffices for controlling a large amount of gameObjects.
I could create 100 ships and store each of their scripts, and then manually reference those scripts in the code and use the get and set methods to change each ship's variables as required.
However, I do not know if this is the best way to set up interactions between ships.

This code specifically seems bad.

Code (CSharp):

1.  
2.         if (other.name == "ship2")
3.         {
4.             while (!dead&&ship2_script.getLives()>=0)
5.             {
6.                 yield return new WaitForSeconds(1f);
7.                 //adds randomization to damage output
8.                 int damage_output = dmg + Random.Range(1, 11);
9.                 //Debug.Log(damage_output+" done to ship2");
10.                 totaldamage += damage_output;
11.                 ship2_script.setLives(ship2_script.getLives() - damage_output);
12.                 heal();
13.             }
14.         }
15.         //same code as above just modified for ship1
16.         else if (other.name == "ship1")
17.         {
18.             while (!dead && ship1_script.getLives() >= 0)
19.             {
20.                 yield return new WaitForSeconds(1f);
21.                 int damage_output = dmg + Random.Range(1, 11);
22.                 totaldamage += damage_output;
23.                 //Debug.Log(damage_output + " done to ship1");
24.                 ship1_script.setLives(ship1_script.getLives() - damage_output);
25.                 heal();
26.             }
27.         }

I would have to continuously keep on adding if statements every time a ship manually gets added to the game before runtime. Additionally, if a ship gets instantized during runtime, I do not think this method of checking what collided with what would work as the instantized ships would have the all have the same name.

I am also relatively new to unity, so sorry if I seem to miss something obvious.
Again, any help is greatly appreciated.

 

That's exactly how it works.

I'll say it again: learn about collections. This means that

ship1, ship2, ship3

gets replaced with a single variable called

ships

(the collection) that contains ALL the ships.

Any multi-enemy tutorial out there will use this approach. If you want a random twinsticker game to look at, see the DemoTwinSticker scene, particularly the Enemy1 script, in my proximity buttons project.

proximity_buttons is presently hosted at these locations:

https://bitbucket.org/kurtdekker/proximity_buttons

https://github.com/kurtdekker/proximity_buttons

https://gitlab.com/kurtdekker/proximity_buttons

https://sourceforge.net/projects/proximity-buttons/
https://gitlab.com/kurtdekker/datasacks

 

Ok, thank you for your time, I will try to implement this system into my code.

 

You might want to consider first understanding the basic process of adding a single script to many objects so they all run by that one script.

For instance, stick this script on as many different GameObjects (Cubes or Spheres or whatever) as you like and press PLAY:

Code (csharp):

1. using UnityEngine;
2.  
3. // @kurtdekker
4.  
5. public class Move : MonoBehaviour
6. {
7.     public Vector3 Velocity;
8.  
9.     void Reset ()
10.     {
11.         Velocity = Random.onUnitSphere;    
12.     }
13.    
14.     void Update ()
15.     {
16.         transform.position += Velocity * Time.deltaTime;
17.     }
18. }

 

Most important is to remove ship1_script and ship2_script from ShipBehaviour. You can create a separate GameObject with a TestInput-component where you put all your key events.

Code (CSharp):

1. public class TestInput: MonoBehaviour
2. {
3.     ShipBehavior ship1_script;
4.     ShipBehavior ship2_script;
5.      
6.     // Start is called before the first frame update
7.     void Start()
8.     {
9.         ship1_script = GameObject.Find("ship1").GetComponent<ShipBehavior>();
10.         ship2_script = GameObject.Find("ship2").GetComponent<ShipBehavior>();
11.     }
12.     // Update is called once per frame
13.     void Update()
14.     {
15.         //key events
16.         //used for mainly testing
17.         if (Input.GetKeyDown("q"))
18.         {
19.             ship1_script.setBaseDmg(ship1_script.getBaseDmg() + 1);
20.         }
21.         // and all other if-statements
22.     }
23. }

The ShipBehaviour has to be the same code for each ship. This also applies to your collison code. It could look like this (Note, that I haven't fully analyzed your code, just replaced the ship2_script). Then it doesn't matter, whether there are 2 ships or multiple.

Code (CSharp):

1. var otherShip = other.getComponent<ShipBehaviour>()          
2. while (!dead&&otherShip .getLives()>=0)
3. {
4.   yield return new WaitForSeconds(1f);
5.   //adds randomization to damage output
6.   int damage_output = dmg + Random.Range(1, 11);
7.   //Debug.Log(damage_output+" done to "+other.name);
8.   totaldamage += damage_output;
9.   otherShip.setLives(ship2_script.getLives() - damage_output);
10.   heal();
11. }

And at last, a minor thing: The getters and setters pollute the code a lot. I would prefer public properties or just make the members public.

 

Break things up into multiple components so you end up with leaner, more focused classes that follow the single-responsibility principle. One component should not be responsible for input detection, collision detection, health management and so forth.

Configuring UnityEvents and serialized fields in general through the Inspector allows for more flexible and reusable code.

Code (CSharp):

1. // Add one to each ship GameObject
2. public class Ship : MonoBehaviour
3. {
4.     private const int MAX_HEALTH = 1000;
5.  
6.     [SerializeField]
7.     private int health = MAX_HEALTH;
8.  
9.     private bool dead => health <= 0;
10.  
11.     public void Damage(int amount)
12.     {
13.         health = Mathf.Max(health - amount, 0);
14.     }
15.  
16.     public void Heal(int amount)
17.     {
18.         if(!dead)
19.         {
20.             health = Mathf.Min(health + amount, MAX_HEALTH);
21.         }
22.     }
23. }}

Code (CSharp):

1. // Add one to each ship GameObject
2. public class InputTrigger : MonoBehaviour
3. {
4.     [SerializeField]
5.     private KeyMapping[] keyMappings = new KeyMapping[0]; // Map Q to Collidable.AdjustBaseDamage using the Inspector etc.
6.  
7.     private void Update()
8.     {
9.         foreach(var mapping in keyMappings)
10.         {
11.             if(Input.GetKeyDown(mapping.Key))
12.             {
13.                 mapping.Action.Invoke();
14.             }
15.         }
16.     }
17.  
18.     [Serializable]
19.     private class KeyMapping
20.     {
21.         [SerializeField] private KeyCode key = KeyCode.None;
22.         [SerializeField] private UnityEvent action = new UnityEvent();
23.  
24.         public KeyCode Key => key;
25.         public UnityEvent Action => action;
26.     }
27. }

Code (CSharp):

1. // Add one to each ship GameObject
2. public class Collidable : MonoBehaviour
3. {
4.     private const int MAX_DAMAGE = 1000;
5.  
6.     [SerializeField]
7.     private UnityEvent<int> OnCollisionDamage;
8.  
9.     [SerializeField, Min(0)]
10.     private int collisionBaseDamage;
11.     [SerializeField, Min(0)]
12.     private int collisionDamageRandomMin;
13.     [SerializeField, Min(0)]
14.     private int collisionDamageRandomMax;
15.     [SerializeField, Min(0f)]
16.     private float damageInterval = 1f;
17.  
18.     private float damageTimer = 1f;
19.  
20.     public void AdjustBaseDamage(int amount) => collisionBaseDamage = Mathf.Clamp(collisionBaseDamage + amount, 0, MAX_DAMAGE);
21.  
22.     private void OnTriggerStay(Collider other)
23.     {
24.         damageTimer += Time.deltaTime;
25.  
26.         if(damageTimer < damageInterval)
27.         {
28.             return;
29.         }
30.  
31.         int damageAmount = collisionBaseDamage + Random.Range(collisionDamageRandomMin, collisionDamageRandomMax + 1);
32.         onCollision.Invoke(damageAmount);
33.         damageTimer = 0f;
34.     }
35.  
36.     private void OnTriggerExit => damageTimer = 0f;
37. }

The above code isn't functionally 1:1 with your original code, but I think you can get the basic idea.