Need advice on cleaning up this messy script (C#)

Hello, novice scripter here, requesting advice and help in cleaning this script. I started with this script for my TD game a couple months ago, and it started small. But it grew larger and messy, and now it is filled with just too much duplicate code which is never good.

So what the script does is it selects platforms. On those platforms towers are placed and you need to press a key to control a tower. You can only control one tower at the time, and I have second (even bigger and messier) script that deselects the currently controlled tower when you press other keys. However, there are just 3 platforms right now. In the end I'm going to have a maximum of 10. So it would be quite ridiculous to duplicate code 10 times for the final version. So I need to find a way to only use the same code once, but still have it work for a maximum of 10 keys and 10 towers & platforms in the scene. I have looked in to a few tutorials, but I don't know which way to go exactly, and that's why I need advice. With that said here is the script, before it gets any worse than it already is. (Note: I have an azerty keyboard, so that's where the temporary A,Z & E come from if you're wondering)

Code (CSharp):

1. using UnityEngine;
2. using System.Collections;
3.  
4. public class Platformselect : MonoBehaviour {
5.  
6.     public GameObject selectedPlatform;
7.     public GameObject deselectedPlatform;
8.     public Camera controlledCamera;
9.     public Camera deControlledCamera;
10.     public GameObject overviewCam;
11.     public PlatformGeneral platformGeneral;
12.  
13.  
14.     void Start () {
15.    
16.  
17.         if(overviewCam.activeInHierarchy == false)
18.         {
19.             overviewCam.SetActive(true);
20.         }
21.  
22.     }
23.  
24.     void Update () {
25.    
26.         if(Input.GetKeyUp(KeyCode.A))
27.         {
28.             Debug.Log("key 1 pressed");
29.  
30.             platformGeneral = GameObject.FindWithTag("Platform1").GetComponent<PlatformGeneral>();
31.             if(platformGeneral.recieveInput == true){
32.                 selectedPlatform = GameObject.FindWithTag("Platform1");
33.                 TurretGeneral turretGeneral = selectedPlatform.GetComponentInChildren<TurretGeneral>();
34.                 turretGeneral.currentlyControlled = true;
35.                 controlledCamera = selectedPlatform.GetComponentInChildren<Camera>();
36.                 controlledCamera.enabled = true;
37.                 selectedPlatform.GetComponentInChildren<AudioListener>().enabled = true;
38.  
39.                 overviewCam.SetActive(false);
40.  
41.                 MouseLook[] mouseLooks = selectedPlatform.GetComponentsInChildren<MouseLook>();
42.                 foreach (MouseLook mLook in mouseLooks){
43.                     mLook.enabled = true;
44.                
45.                 }
46.            
47.             }
48.             else {
49.                 Debug.Log("That platform can't be selected");
50.             }
51.         }
52.  
53.         if(Input.GetKeyUp(KeyCode.Z))
54.         {
55.             Debug.Log("key 2 pressed");
56.            
57.             platformGeneral = GameObject.FindWithTag("Platform2").GetComponent<PlatformGeneral>();
58.             if(platformGeneral.recieveInput == true){
59.                 selectedPlatform = GameObject.FindWithTag("Platform2");
60.                 TurretGeneral turretGeneral = selectedPlatform.GetComponentInChildren<TurretGeneral>();
61.                 turretGeneral.currentlyControlled = true;
62.                 controlledCamera = selectedPlatform.GetComponentInChildren<Camera>();
63.                 controlledCamera.enabled = true;
64.                 selectedPlatform.GetComponentInChildren<AudioListener>().enabled = true;
65.  
66.                 overviewCam.SetActive(false);
67.  
68.                 MouseLook[] mouseLooks = selectedPlatform.GetComponentsInChildren<MouseLook>();
69.                 foreach (MouseLook mLook in mouseLooks){
70.                     mLook.enabled = true;
71.                 }
72.            
73.             }
74.             else {
75.                 Debug.Log("That platform can't be selected");
76.             }
77.         }
78.    
79.         if(Input.GetKeyUp(KeyCode.E))
80.         {
81.             Debug.Log("key 3 pressed");
82.            
83.            
84.             platformGeneral = GameObject.FindWithTag("Platform2").GetComponent<PlatformGeneral>();
85.             if(platformGeneral.recieveInput == true){
86.                 selectedPlatform = GameObject.FindWithTag("Platform3");
87.                 TurretGeneral turretGeneral = selectedPlatform.GetComponentInChildren<TurretGeneral>();
88.                 turretGeneral.currentlyControlled = true;
89.                 controlledCamera = selectedPlatform.GetComponentInChildren<Camera>();
90.                 controlledCamera.enabled = true;
91.                 selectedPlatform.GetComponentInChildren<AudioListener>().enabled = true;
92.  
93.                 overviewCam.SetActive(false);
94.  
95.                 MouseLook[] mouseLooks = selectedPlatform.GetComponentsInChildren<MouseLook>();
96.                 foreach (MouseLook mLook in mouseLooks){
97.                     mLook.enabled = true;
98.                 }
99.             }
100.        
101.             else {
102.                 Debug.Log("That platform can't be selected");
103.             }
104.         }
105.     }
106. }
107.  

Happy scripting times to all!

 

Any time you have repeated copy/paste code, make it into a separate function. Then just call the function.

--Eric

 

Right, that would surely work for this script. Thanks. That does mean I have to make 10 key press if-statements, and call the same function in them. Pass trough data on what key was pressed and what platform to look for through parameters, and it would work I hope. I won't have time to change it today, but I fear that this would be trickier to do for the second script, which basically does what this one does reversed. It checks first what specific platform is controlled (via those tags) at all times and then it checks if any selection keys, other than the one from the platform itself, is pressed to deselect the current platform, allowing this script to select a new one. I'll think about this and change my scripts and come back when I have any further problems.

 

yes definitely put all that duplicate code into another function inside the class (functions that exist within classes are called Methods)
http://msdn.microsoft.com/en-us/library/ms173114.aspx

i don't know your programming history, but if you haven't already, i would highly recommend just going through the entire MSDN C# programming guide. that's what i did when i started unity and since using unity is basically fully dependent on your understanding of C# i can't imagine trying to learn unity without studying C# by itself as well.

 

Thanks for that link! I've been scripting for 4 months now, I watched all the unity scripting tutorials multiple times, but I haven't really read an entire guide. So far I've been just looking things up, primarily videos, see how other people do similar things to what I need. And I learn a lot that way, and I learn fast.

By the way, this is an old script. I haven't touched it in two months. At the time then I was happy that it worked, not caring about how I would handle it in the future. Today my scripts are clean and good. I was just afraid to completely redo an existing large script without messing it up. I'm not a gamedev yet, I'm just doing this as a hobby, more to try things out and see how it all works. So I only work on this in my spare time.

 

You can map the changing parts of the data (turret specific details) into an object. In this case an array or list of objects. Then you can make a simple class or data structure that "binds" each key code value to the appropriate turret object. you end up with a simple loop through KeyTurret bindings if any of their key codes are currently being pressed process that turret.

 

Alright, I managed to clean it by a big chunk. It was a lot easier than I thought, it only took a couple minutes.

Although there is still a small amount of duplicate code. Here's what Update now looks like:

Code (CSharp):

1. void Update () {
2.    
3.         if(Input.GetKeyUp(KeyCode.A)){
4.             Debug.Log ("key 1 pressed");
5.             if(GameObject.FindWithTag("Platform1") != null){
6.             Select(1, "Platform1");
7.             }
8.        
9.         }
10.         if(Input.GetKeyUp(KeyCode.Z)){
11.             Debug.Log ("key 2 pressed");
12.             if(GameObject.FindWithTag("Platform2") != null){
13.                 Select(2, "Platform2");
14.             }
15.            
16.         }
17.         if(Input.GetKeyUp(KeyCode.E)){
18.             Debug.Log ("key 3 pressed");
19.             if(GameObject.FindWithTag("Platform3") != null){
20.                 Select(3, "Platform3");
21.             }
22.            
23.         }
24.         if(Input.GetKeyUp(KeyCode.R)){
25.             Debug.Log ("key 4 pressed");
26.             if(GameObject.FindWithTag("Platform4") != null){
27.                 Select(4, "Platform4");
28.             }
29.            
30.         }
31.     }
32.  
33.  
34.      public void Select (int platformNumber, string pTag){
35.    
36.         platformGeneral = GameObject.FindWithTag(pTag).GetComponent<PlatformGeneral>();
37.         if(platformGeneral.recieveInput == true){
38.             selectedPlatform = GameObject.FindWithTag(pTag);
39.             TurretGeneral turretGeneral = selectedPlatform.GetComponentInChildren<TurretGeneral>();
40.             turretGeneral.currentlyControlled = true;
41.             controlledCamera = selectedPlatform.GetComponentInChildren<Camera>();
42.             controlledCamera.enabled = true;
43.             selectedPlatform.GetComponentInChildren<AudioListener>().enabled = true;
44.  
45.             overviewCam.SetActive(false);
46.  
47.             MouseLook[] mouseLooks = selectedPlatform.GetComponentsInChildren<MouseLook>();
48.             foreach (MouseLook mLook in mouseLooks){
49.                 mLook.enabled = true;
50.            
51.             }
52.        
53.         }
54.         else {
55.             Debug.LogWarning("That platform can't be selected");
56.         }
57.     }

It's alright now, and I'm pleased for now. But the second script will be more difficult.

I think I get most of what you mean here, although I haven't done anything like this before, so if you are able to find an example, that would be excellent.

 

So I still need to do the second (and last) script. I'm going to do the same thing, but I'm not sure how to handle key detection for the if statements.

What I'm trying to do is

Code (CSharp):

1. if(selectedPlatformNmbr = 1){
2.                 if(Input.GetButtonUp(buttons 1-10)){
3.                  if(keypressed != "Button1"){
4.                     //call function
5.                 }
6.             }
7.            
8.         }

So how do I best go about detecting if any keys between InputButtons 1-10 were pressed? And then detecting which of these keys was pressed (so I can check if it wasn't the key to select the current platform)? I'm aware that KeyCode[] is a thing, but that is not the same for Input.GetButton .

 

You'd use a string array, and loop through the array, checking GetButtonUp for each entry.

--Eric