Am I using too many if else statements? Is it bad?

I have realized that I use the if else statements a lot. For example here is my Enemy script:

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4.  
5. public class Enemy : MonoBehaviour
6. {
7.     [SerializeField] float hp;
8.     [SerializeField] float spawnChance;
9.     [SerializeField] int scoreToSpawn;
10.     [SerializeField] int spawnLimit;
11.     [SerializeField] GameObject deathVFX = null;
12.     [SerializeField] AudioClip explosionSFX = null;
13.     [SerializeField] AudioClip hitSFX = null;
14.     [SerializeField] GameObject star = null;
15.     [SerializeField] AudioClip starCollectionSound = null;
16.     [SerializeField] GameObject deathAnimation = null;
17.  
18.     PlayerFire pf;
19.  
20.     private void Start()
21.     {
22.         if (GameObject.FindGameObjectsWithTag(tag).Length - 1 >= spawnLimit && !CompareTag("FluffBall"))
23.         {
24.             Debug.Log(tag);
25.             EnemySpawner.count--;
26.             Destroy(gameObject);
27.         }
28.  
29.         pf = FindObjectOfType<PlayerFire>();
30.     }
31.  
32.     private void OnTriggerEnter2D(Collider2D collision)
33.     {
34.         if(collision.gameObject.GetComponent<Projectile>())
35.         {
36.             Projectile projectile = collision.gameObject.GetComponent<Projectile>();          
37.             GameObject hitVFX = Instantiate(projectile.getHitVFX(), collision.gameObject.transform.position, Quaternion.identity);
38.             AudioSource.PlayClipAtPoint(hitSFX, Camera.main.transform.position, 4);
39.             Destroy(collision.gameObject);
40.             Destroy(hitVFX, 2f);
41.            
42.             recieveDamage(projectile.getDamage());
43.         }
44.     }
45.  
46.     void recieveDamage(float damage)
47.     {
48.         hp -= damage;
49.  
50.         if (hp <= 0)
51.         {
52.             if (GetComponent<EnemyProjectileMovement>() == null && FindObjectOfType<PlayerMovement>() != null)
53.             {
54.                 ScoreManager.addToScore(1);
55.  
56.                 if(CompareTag("BigParrot"))
57.                 {
58.                     Currency.addStars(1);
59.                     GameObject dAnimation = Instantiate(deathAnimation, new Vector2(transform.position.x + 0.5f, transform.position.y), Quaternion.identity);
60.                     Destroy(dAnimation, 2f);
61.                 }
62.  
63.                 if (ScoreManager.currentScore % 5 == 0)
64.                 {
65.                     GameObject starAnimation = Instantiate(star, transform.position, Quaternion.identity);
66.                     AudioSource.PlayClipAtPoint(starCollectionSound, Camera.main.transform.position, 0.5f);
67.                     Destroy(starAnimation, 2f);
68.                     Currency.addStars(1);
69.                 }
70.  
71.             }
72.  
73.             explode();                                  
74.         }  
75.        
76.         else
77.         {
78.             pf.shakeCamera(0.05f, 0.05f);
79.         }
80.     }
81.  
82.     void explode()
83.     {
84.         GameObject explodeVFX = Instantiate(deathVFX, transform.position, Quaternion.identity);
85.         Destroy(explodeVFX, 3f);
86.         if(!CompareTag("FluffBall"))
87.         {
88.             EnemySpawner.count--;
89.         }  
90.         AudioSource.PlayClipAtPoint(explosionSFX, Camera.main.transform.position, 0.6f);
91.         pf.shakeCamera(0.3f, 0.05f);
92.         Destroy(gameObject);
93.     }
94.  
95.     public float getSpawnChance()
96.     {
97.         return spawnChance;
98.     }
99.  
100.     public int getScoreToSpawn()
101.     {
102.         return scoreToSpawn;
103.     }
104.  
105.     public int getSpawnLimit()
106.     {
107.         return spawnLimit;
108.     }
109.  
110.     public float getHealth()
111.     {
112.         return hp;
113.     }
114.  
115.     public void setSpawnLimit(int spawnLimit)
116.     {
117.         this.spawnLimit = spawnLimit;
118.     }
119. }

Is it that bad? I heard a lot of awful stuff about using too many if statements so I just want to know if I'm doing is wrong or not?

 

Hi,

This question was asked a while ago and has a bunch of responses: https://forum.unity.com/threads/using-if-statements-too-much.651295/

J

 

Doesn't look like something out of the ordinary. What you should avoid however is calling GetComponent and FindObjectOfType so often because they are slow. You could cache objects/components by adding a couple global variables and initializing them in the start function. In case of the OnTriggerEnter2D function you could add the object that collides to a dictionary with the Collision2D component as key, and the Projectile component as value. This way you don't have to call GetComponent each time you need access to a component. Anyway, here is an example.

Code (csharp):

1. Dictionary<Collider2D, Projectile> collisionDictionary = new Dictionary<Collider2D, Projectile>();
2.  
3. private void OnTriggerEnter2D(Collider2D collision)
4. {
5.     //Will only be set to true if a Projectile component is found
6.     bool doDamage = false;
7.  
8.     //Check whether dictionary already contains an entry of this Collider2D, if not then add it to the dictionary
9.     if(!collisionDictionary.ContainsKey(collision)
10.     {
11.         Projectile projectile = collision.gameObject.GetComponent<Projectile>();
12.    
13.         //Add Projectile to dictionary if there is a Projectile component on the game object that collided
14.         if(projectile != null)
15.         {
16.             collisionDictionary[collision] = projectile;
17.             doDamage = true;
18.         }
19.     }    
20.     else
21.     {
22.         doDamage = true;
23.     }
24.  
25.     if(doDamage)
26.     {
27.         Projectile projectile = collisionDictionary[collision];
28.         GameObject hitVFX = Instantiate(projectile.getHitVFX(), collision.gameObject.transform.position, Quaternion.identity);
29.         AudioSource.PlayClipAtPoint(hitSFX, Camera.main.transform.position, 4);
30.         Destroy(collision.gameObject);
31.         Destroy(hitVFX, 2f);
32.        
33.         recieveDamage(projectile.getDamage());
34.     }
35. }

 

Thanks! I will do some research on the dictionary so I can understand better.

 

Dictionaries work with a key/value pair. Unlike an array where you use an integer to get access to a value in it, a dictionary allows you to use pretty much anything as a key to get the value. Since all you have in the OnTriggerEnter2D is a Collision2D object, you can use it as a key to obtain the Projectile (value) from the dictionary. It's much faster than using GetComponent each time because you only do it once when you add the Projectile to the dictionary. If you use something a lot it's always a good thing to have quick access to it and either make a reference to it when the game starts, or make a reference at a later point. If you have to get a reference more often by using GetComponent each time, you waste resources because it takes time to look up and it needs to allocate memory again which also takes time. This weighs much heavier than a few if statements and especially if you have lots of objects and lots of GetComponent or FindObject calls it scales very bad.

 

Yea that makes sense. Thanks I will watch some tutorials on how to implement them

 

I blame the documentation because there are so many of these bad practices just to show how easy Unity is (and it actually is in my opinion). We've all been there when we just wanted to do something quick and later found out the performance isn't so great when the project grows. The few extra lines of code you need to write to fix it are worth it.

 

You mean that Dr. Phil episode? As usual, he doesn't know what he's talking about. Seriously, the internet is full of random idiots copying other random idiots. Programming advice without an explanation is just noise. Some of the time it _was_ real advice, but now mangled by people who didn't understand it.

A common, real, too-many-ifs is about using them because you don't know arrays: if(n==0) x=4; else if (n==1) x=7; else if(n==2) x=12; ... . That's not really advice though. The actual advice is "learn arrays. Use them for lists". More recently, the quit-if-null operator was made to replace certain IF's:

transform?.renderer?.material.?color

instead of if(transform!=null) if(transform.renderer!=null)... . People who love those things can say that everyone else is using too many IF's for null-checks (we're not -- in real situations we need the IF's so we can fix missing transforms or renderers).

 

That's probably not even worth the effort. GetComponent has been optimized long time ago and its performance isn't that bad anymore.
Sure, don't call it multiple times in a row, cache some references here and there if you know you're going to use it a lot or that it's not going to change frequently.

Anyway, if you add the dictionary, you'll either need to let every enemy know that a bullet was shot or use a single dictionary that everyone involved in that system needs access to.
And what's worth it when there isn't even pooling involved? Without poolin you'll be doing extra work without any real benefit.

This is certainly belonging into the category "premature optimization".

 

Unrelated to the question, but I'm wondering why you're using typical getters/setters...

Code (CSharp):

1. public float getSpawnChance()
2. {
3.    return spawnChance;
4. }
5. public int getScoreToSpawn()
6. {
7.    return scoreToSpawn;
8. }
9. public int getSpawnLimit()
10. {
11.    return spawnLimit;
12. }
13. public float getHealth()
14. {
15.    return hp;
16. }
17. public void setSpawnLimit(int spawnLimit)
18. {
19.    this.spawnLimit = spawnLimit;
20. }

...instead of C# properties, which would clean up the code a bit more:

Code (CSharp):

1. public float SpawnChance => spawnChance;
2. public int ScoreToSpawn => scoreToSpawn;
3. public int SpawnLimit { get => spawnLimit; set => spawnLimit = value; }
4. public float Health => hp;

Is it just preference, I guess?

 

You go ahead and use GetComponent all you like. I'll continue to tell people to cache their references because it's little effort and it will always be faster than having to look up the reference each time. Also what I suggested doesn't break his current functionality, it'll do exactly the same so I'm not sure what you are trying to argue about.

 

Well, I do the same - but only if it's appropriate.
If you cannot even tell that there's going to be a benefit in a particular scenario, why all the effort?

And noone said what you suggested breaks his system.

 

Oh I never heard of these properties. I only learned getters and setters so I used them lol

 

I'm laughing, but really at myself. That doesn't look like too much use of if/else. You'll know it gets too much when it impacts your ability to maintain the code. You'll know it when it happens.

For example, this method of mine from my custom networking system has grown a bit out of control, troublesome to maintain, so is currently sitting at the top of my refactoring todo list:

Code (csharp):

1.         public bool ConnectionUpkeep()
2.         {
3.  
4.             if (Connected)
5.             {
6.  
7.                 //Sort incoming TransportMessages by MessageID
8.                 int channel = 0;
9.                 foreach (List<TransportMessage> lt in incomingTransportMessages.ToArray())
10.                 {
11.                     if (lt.Count > 1)
12.                     {
13.                         incomingTransportMessages[channel] = lt.OrderBy(o => o.MessageID).ToList<TransportMessage>();
14.                     }
15.                     channel++;
16.                 }
17.  
18.                 //Check pending Ack messages for timeout to resend
19.                 channel = 0;
20.                 foreach (List<PendingAck> pendingAckChannelList in sentAwatingAck)
21.                 {
22.                     int numberResent = 0;  //Only resend a small number at a time
23.                     int maxResent = pendingAckChannelList.Count / 5;  //Scale up the maxResent if the pending acks are stacking up
24.                     if (maxResent < 20)  //Set minimum to 20
25.                     {
26.                         maxResent = 20;
27.                     }
28.                     if (maxResent > 60)  //Set maximum to 60
29.                     {
30.                         maxResent = 60;
31.                     }
32.                     foreach (PendingAck pendingAck in pendingAckChannelList)
33.                     {
34.                         if (Time.time > pendingAck.LastSent + ConnectionClientServerFrontEnd.Channels[channel].NoACKResendTime)
35.                         {
36.                             //Resend message
37.  
38.                             pendingAck.LastSent = Time.time;
39.                             pendingAck.Retries++;
40.  
41.                             Debug.Log("Resending " + pendingAck.Message.MessageID.ToString());
42.                             ConnectionClientServerFrontEnd.Channels[channel].SendTransportMessage(pendingAck.Message, EncryptionKey);
43.                             keepAliveLastSent = Time.time;  //Update last keep alive time
44.                             numberResent++;
45.                         }
46.  
47.                         if (numberResent >= maxResent)
48.                         {
49.                             break;
50.                         }
51.                     }
52.                     channel++;
53.                 }
54.  
55.                 //Upkeep of incoming messages
56.  
57.                 //Incoming transport messages (Converts to MidLevelMessages)
58.                 channel = -1;  //Start at -1 because we immediately increment to 0
59.                 MidLevelMessage mlm;
60.                 foreach (List<TransportMessage> transportList in incomingTransportMessages)
61.                 {
62.                     channel++;  //Used primarily as the channel number here
63.  
64.                     if (transportList.Count > 0)
65.                     {
66.  
67.                         if (ConnectionClientServerFrontEnd.Channels[channel].Reliable)
68.                         {
69.                             //For ordered and reliable channel we need to hold messages if there is a missing message ID
70.                             foreach (TransportMessage t in transportList.ToArray())
71.                             {
72.                                 if (t.MessageType == (int)TransportMessage.TransportMessageType.Ack)
73.                                 {
74.                                     removePendingAck(t, channel);
75.                                     incomingTransportMessages[channel].Remove(t);
76.                                 }
77.                                 else
78.                                 {
79.                                     if (t.MessageID <= lastTransportMessageIDReceived[channel])  //Means we have a duplicate message
80.                                     {
81.                                         incomingTransportMessages[channel].Remove(t);  //Remove duplicate message
82.                                         Debug.Log("Removed Duplicate " + t.MessageID.ToString() + " " + t.MessageType.ToString() + " " + t.Payload.Length.ToString());
83.                                     }
84.                                     else
85.                                     {
86.                                         if (t.MessageID > lastTransportMessageIDReceived[channel] + 1)
87.                                         {
88.                                             break;  //We don't want to loop through the entire transport message list if we are missing a message
89.                                         }
90.                                         else  //Means t.MessageID should equal lastTransportMessageIDReceived[looper] + 1
91.                                         {
92.                                             try
93.                                             {
94.                                                 lastTransportMessageIDReceived[channel] = t.MessageID;
95.                                                 if (t.Fragmented)
96.                                                 {
97.                                                     if (t.MessageType == (int)TransportMessage.TransportMessageType.Standard)
98.                                                     {
99.                                                         addFragment(t, channel);
100.                                                     }
101.                                                 }
102.                                                 else
103.                                                 {
104.                                                     if (t.MessageType == (int)TransportMessage.TransportMessageType.Standard)
105.                                                     {
106.                                                         mlm = new MidLevelMessage();
107.                                                         mlm.CreateFromTransportMessage(t);
108.                                                         incomingMidLevelMessages[channel].Add(mlm);
109.                                                         lastMidLevelMessageIDReceived[channel] = mlm.MidLevelMessageID;
110.  
111.                                                     }
112.  
113.                                                 }
114.                                             }
115.                                             catch (Exception e)
116.                                             {
117.                                                 Debug.LogError("JCGNetwork hit exception processing transport message for channel " + channel.ToString() + " ConnectionUpkeep JCGConnection.cs, stacktrace follows");
118.                                                 Debug.LogError(e.StackTrace);
119.                                             }
120.                                             incomingTransportMessages[channel].Remove(t);
121.                                         }
122.                                     }
123.                                 }
124.                             }
125.  
126.                         }
127.                         else
128.                         {
129.                             //For unreliable we just need to drop any message that arrives out of order
130.                             foreach (TransportMessage t in transportList.ToArray())
131.                             {
132.                                 if (t.MessageID > lastTransportMessageIDReceived[channel])
133.                                 {
134.                                     try
135.                                     {
136.                                         lastTransportMessageIDReceived[channel] = t.MessageID;
137.                                         if (t.Fragmented)
138.                                         {
139.                                             if (t.MessageType == (int)TransportMessage.TransportMessageType.Standard)
140.                                             {
141.                                                 addFragment(t, channel);
142.                                             }
143.                                         }
144.                                         else  //Means not fragmented
145.                                         {
146.                                             if (t.MessageType == (int)TransportMessage.TransportMessageType.Standard)
147.                                             {
148.                                                 mlm = new MidLevelMessage();
149.                                                 mlm.CreateFromTransportMessage(t);
150.                                                 incomingMidLevelMessages[channel].Add(mlm);
151.                                                 lastMidLevelMessageIDReceived[channel] = mlm.MidLevelMessageID;
152.                                             }
153.                                         }
154.                                     }
155.                                     catch (Exception e)
156.                                     {
157.                                         Debug.LogError("JCGNetwork hit exception processing transport message for channel " + channel.ToString() + " ConnectionUpkeep JCGConnection.cs, stacktrace follows");
158.                                         Debug.LogError(e.StackTrace);
159.                                     }
160.                                 }
161.                                 incomingTransportMessages[channel].Remove(t);
162.                             }
163.                         }
164.                     }
165.                 }
166.  
167.                 //End incoming transport message upkeep
168.  
169.  
170.                 //Start upkeep of incoming MidLevelMessages (converts to HighLevelMessages)
171.                 channel = -1;
172.                 foreach (List<MidLevelMessage> midLevelMessageList in incomingMidLevelMessages)
173.                 {
174.                     channel++;
175.                     if (midLevelMessageList.Count > 0)
176.                     {
177.                         foreach (MidLevelMessage midLevelMessage in midLevelMessageList)
178.                         {
179.                             try
180.                             {
181.                                 List<HighLevelMessage> highLevelMessages = midLevelMessage.CreateHighLevelMessages();
182.                                 foreach (HighLevelMessage highLevelMessage in highLevelMessages)
183.                                 {
184.                                     incomingHighLevelMessages[channel].Add(highLevelMessage);
185.                                     //Debug.Log("HighLevelMessage Received: " + System.Text.Encoding.UTF8.GetString(highLevelMessage.Payload));
186.                                 }
187.                             }
188.                             catch (Exception e)
189.                             {
190.                                 Debug.LogError("JCGNetwork hit exception processing MidLevelMessages for channel " + channel.ToString() + " ConnectionUpkeep JCGConnection.cs, stacktrace follows");
191.                                 Debug.LogError(e.StackTrace);
192.                             }
193.                         }
194.                     }
195.                     incomingMidLevelMessages[channel].Clear();  //Clear the MidLevelMessages list now that we have processed it
196.                 }
197.  
198.  
199.             }
200.  
201.             //Means we have not received any message for long enough that we should disconnect
202.             if (timeLastMessageReceived + IdleTimeBeforeDisconnect < Time.time)
203.             {
204.                 Disconnect(true);  //Disconnect this connection and make attempt at sending a disconnect message
205.                 Debug.Log("JCGConnection disconnect due to idle timeout");
206.             }
207.  
208.             return Connected;
209.         }

 

@Suddoha sorry I overlooked something and I see your point now. It indeed makes no sense to store a reference to a component if you're going to destroy the game object anyway. You're right that it would matter if he actually had a pool of objects and reuse them. I stand corrected.