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:

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

public class Enemy : MonoBehaviour
{
    [SerializeField] float hp;
    [SerializeField] float spawnChance;
    [SerializeField] int scoreToSpawn;
    [SerializeField] int spawnLimit;
    [SerializeField] GameObject deathVFX = null;
    [SerializeField] AudioClip explosionSFX = null;
    [SerializeField] AudioClip hitSFX = null;
    [SerializeField] GameObject star = null;
    [SerializeField] AudioClip starCollectionSound = null;
    [SerializeField] GameObject deathAnimation = null;

    PlayerFire pf;

    private void Start()
    {
        if (GameObject.FindGameObjectsWithTag(tag).Length - 1 >= spawnLimit && !CompareTag("FluffBall"))
        {
            Debug.Log(tag);
            EnemySpawner.count--;
            Destroy(gameObject);
        }

        pf = FindObjectOfType<PlayerFire>();
    }

    private void OnTriggerEnter2D(Collider2D collision)
    {
        if(collision.gameObject.GetComponent<Projectile>())
        {
            Projectile projectile = collision.gameObject.GetComponent<Projectile>();          
            GameObject hitVFX = Instantiate(projectile.getHitVFX(), collision.gameObject.transform.position, Quaternion.identity);
            AudioSource.PlayClipAtPoint(hitSFX, Camera.main.transform.position, 4);
            Destroy(collision.gameObject);
            Destroy(hitVFX, 2f);
           
            recieveDamage(projectile.getDamage());
        }
    }

    void recieveDamage(float damage)
    {
        hp -= damage;

        if (hp <= 0)
        {
            if (GetComponent<EnemyProjectileMovement>() == null && FindObjectOfType<PlayerMovement>() != null)
            {
                ScoreManager.addToScore(1);

                if(CompareTag("BigParrot"))
                {
                    Currency.addStars(1);
                    GameObject dAnimation = Instantiate(deathAnimation, new Vector2(transform.position.x + 0.5f, transform.position.y), Quaternion.identity);
                    Destroy(dAnimation, 2f);
                }

                if (ScoreManager.currentScore % 5 == 0)
                {
                    GameObject starAnimation = Instantiate(star, transform.position, Quaternion.identity);
                    AudioSource.PlayClipAtPoint(starCollectionSound, Camera.main.transform.position, 0.5f);
                    Destroy(starAnimation, 2f);
                    Currency.addStars(1);
                }

            }

            explode();                                  
        }   
       
        else
        {
            pf.shakeCamera(0.05f, 0.05f);
        }
    }

    void explode()
    {
        GameObject explodeVFX = Instantiate(deathVFX, transform.position, Quaternion.identity);
        Destroy(explodeVFX, 3f);
        if(!CompareTag("FluffBall"))
        {
            EnemySpawner.count--;
        }  
        AudioSource.PlayClipAtPoint(explosionSFX, Camera.main.transform.position, 0.6f);
        pf.shakeCamera(0.3f, 0.05f);
        Destroy(gameObject);
    }

    public float getSpawnChance()
    {
        return spawnChance;
    }

    public int getScoreToSpawn()
    {
        return scoreToSpawn;
    }

    public int getSpawnLimit()
    {
        return spawnLimit;
    }

    public float getHealth()
    {
        return hp;
    }

    public void setSpawnLimit(int spawnLimit)
    {
        this.spawnLimit = spawnLimit;
    }
}

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: Using If Statements too much?

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.

Dictionary<Collider2D, Projectile> collisionDictionary = new Dictionary<Collider2D, Projectile>();

private void OnTriggerEnter2D(Collider2D collision)
{
    //Will only be set to true if a Projectile component is found
    bool doDamage = false;

    //Check whether dictionary already contains an entry of this Collider2D, if not then add it to the dictionary
    if(!collisionDictionary.ContainsKey(collision)
    {
        Projectile projectile = collision.gameObject.GetComponent<Projectile>();
    
        //Add Projectile to dictionary if there is a Projectile component on the game object that collided
        if(projectile != null)
        {
            collisionDictionary[collision] = projectile;
            doDamage = true;
        }
    }    
    else
    {
        doDamage = true;
    }

    if(doDamage)
    {
        Projectile projectile = collisionDictionary[collision];
        GameObject hitVFX = Instantiate(projectile.getHitVFX(), collision.gameObject.transform.position, Quaternion.identity);
        AudioSource.PlayClipAtPoint(hitSFX, Camera.main.transform.position, 4);
        Destroy(collision.gameObject);
        Destroy(hitVFX, 2f);
       
        recieveDamage(projectile.getDamage());
    }
}
1 Like

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.

1 Like

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…

public float getSpawnChance()
{
   return spawnChance;
}
public int getScoreToSpawn()
{
   return scoreToSpawn;
}
public int getSpawnLimit()
{
   return spawnLimit;
}
public float getHealth()
{
   return hp;
}
public void setSpawnLimit(int spawnLimit)
{
   this.spawnLimit = spawnLimit;
}

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

public float SpawnChance => spawnChance;
public int ScoreToSpawn => scoreToSpawn;
public int SpawnLimit { get => spawnLimit; set => spawnLimit = value; }
public float Health => hp;

Is it just preference, I guess?

2 Likes

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:

        public bool ConnectionUpkeep()
        {

            if (Connected)
            {

                //Sort incoming TransportMessages by MessageID
                int channel = 0;
                foreach (List<TransportMessage> lt in incomingTransportMessages.ToArray())
                {
                    if (lt.Count > 1)
                    {
                        incomingTransportMessages[channel] = lt.OrderBy(o => o.MessageID).ToList<TransportMessage>();
                    }
                    channel++;
                }

                //Check pending Ack messages for timeout to resend
                channel = 0;
                foreach (List<PendingAck> pendingAckChannelList in sentAwatingAck)
                {
                    int numberResent = 0;  //Only resend a small number at a time
                    int maxResent = pendingAckChannelList.Count / 5;  //Scale up the maxResent if the pending acks are stacking up
                    if (maxResent < 20)  //Set minimum to 20
                    {
                        maxResent = 20;
                    }
                    if (maxResent > 60)  //Set maximum to 60
                    {
                        maxResent = 60;
                    }
                    foreach (PendingAck pendingAck in pendingAckChannelList)
                    {
                        if (Time.time > pendingAck.LastSent + ConnectionClientServerFrontEnd.Channels[channel].NoACKResendTime)
                        {
                            //Resend message

                            pendingAck.LastSent = Time.time;
                            pendingAck.Retries++;

                            Debug.Log("Resending " + pendingAck.Message.MessageID.ToString());
                            ConnectionClientServerFrontEnd.Channels[channel].SendTransportMessage(pendingAck.Message, EncryptionKey);
                            keepAliveLastSent = Time.time;  //Update last keep alive time
                            numberResent++;
                        }

                        if (numberResent >= maxResent)
                        {
                            break;
                        }
                    }
                    channel++;
                }

                //Upkeep of incoming messages

                //Incoming transport messages (Converts to MidLevelMessages)
                channel = -1;  //Start at -1 because we immediately increment to 0
                MidLevelMessage mlm;
                foreach (List<TransportMessage> transportList in incomingTransportMessages)
                {
                    channel++;  //Used primarily as the channel number here

                    if (transportList.Count > 0)
                    {

                        if (ConnectionClientServerFrontEnd.Channels[channel].Reliable)
                        {
                            //For ordered and reliable channel we need to hold messages if there is a missing message ID
                            foreach (TransportMessage t in transportList.ToArray())
                            {
                                if (t.MessageType == (int)TransportMessage.TransportMessageType.Ack)
                                {
                                    removePendingAck(t, channel);
                                    incomingTransportMessages[channel].Remove(t);
                                }
                                else
                                {
                                    if (t.MessageID <= lastTransportMessageIDReceived[channel])  //Means we have a duplicate message
                                    {
                                        incomingTransportMessages[channel].Remove(t);  //Remove duplicate message
                                        Debug.Log("Removed Duplicate " + t.MessageID.ToString() + " " + t.MessageType.ToString() + " " + t.Payload.Length.ToString());
                                    }
                                    else
                                    {
                                        if (t.MessageID > lastTransportMessageIDReceived[channel] + 1)
                                        {
                                            break;  //We don't want to loop through the entire transport message list if we are missing a message
                                        }
                                        else  //Means t.MessageID should equal lastTransportMessageIDReceived[looper] + 1
                                        {
                                            try
                                            {
                                                lastTransportMessageIDReceived[channel] = t.MessageID;
                                                if (t.Fragmented)
                                                {
                                                    if (t.MessageType == (int)TransportMessage.TransportMessageType.Standard)
                                                    {
                                                        addFragment(t, channel);
                                                    }
                                                }
                                                else
                                                {
                                                    if (t.MessageType == (int)TransportMessage.TransportMessageType.Standard)
                                                    {
                                                        mlm = new MidLevelMessage();
                                                        mlm.CreateFromTransportMessage(t);
                                                        incomingMidLevelMessages[channel].Add(mlm);
                                                        lastMidLevelMessageIDReceived[channel] = mlm.MidLevelMessageID;

                                                    }

                                                }
                                            }
                                            catch (Exception e)
                                            {
                                                Debug.LogError("JCGNetwork hit exception processing transport message for channel " + channel.ToString() + " ConnectionUpkeep JCGConnection.cs, stacktrace follows");
                                                Debug.LogError(e.StackTrace);
                                            }
                                            incomingTransportMessages[channel].Remove(t);
                                        }
                                    }
                                }
                            }

                        }
                        else
                        {
                            //For unreliable we just need to drop any message that arrives out of order
                            foreach (TransportMessage t in transportList.ToArray())
                            {
                                if (t.MessageID > lastTransportMessageIDReceived[channel])
                                {
                                    try
                                    {
                                        lastTransportMessageIDReceived[channel] = t.MessageID;
                                        if (t.Fragmented)
                                        {
                                            if (t.MessageType == (int)TransportMessage.TransportMessageType.Standard)
                                            {
                                                addFragment(t, channel);
                                            }
                                        }
                                        else  //Means not fragmented
                                        {
                                            if (t.MessageType == (int)TransportMessage.TransportMessageType.Standard)
                                            {
                                                mlm = new MidLevelMessage();
                                                mlm.CreateFromTransportMessage(t);
                                                incomingMidLevelMessages[channel].Add(mlm);
                                                lastMidLevelMessageIDReceived[channel] = mlm.MidLevelMessageID;
                                            }
                                        }
                                    }
                                    catch (Exception e)
                                    {
                                        Debug.LogError("JCGNetwork hit exception processing transport message for channel " + channel.ToString() + " ConnectionUpkeep JCGConnection.cs, stacktrace follows");
                                        Debug.LogError(e.StackTrace);
                                    }
                                }
                                incomingTransportMessages[channel].Remove(t);
                            }
                        }
                    }
                }

                //End incoming transport message upkeep


                //Start upkeep of incoming MidLevelMessages (converts to HighLevelMessages)
                channel = -1;
                foreach (List<MidLevelMessage> midLevelMessageList in incomingMidLevelMessages)
                {
                    channel++;
                    if (midLevelMessageList.Count > 0)
                    {
                        foreach (MidLevelMessage midLevelMessage in midLevelMessageList)
                        {
                            try
                            {
                                List<HighLevelMessage> highLevelMessages = midLevelMessage.CreateHighLevelMessages();
                                foreach (HighLevelMessage highLevelMessage in highLevelMessages)
                                {
                                    incomingHighLevelMessages[channel].Add(highLevelMessage);
                                    //Debug.Log("HighLevelMessage Received: " + System.Text.Encoding.UTF8.GetString(highLevelMessage.Payload));
                                }
                            }
                            catch (Exception e)
                            {
                                Debug.LogError("JCGNetwork hit exception processing MidLevelMessages for channel " + channel.ToString() + " ConnectionUpkeep JCGConnection.cs, stacktrace follows");
                                Debug.LogError(e.StackTrace);
                            }
                        }
                    }
                    incomingMidLevelMessages[channel].Clear();  //Clear the MidLevelMessages list now that we have processed it
                }


            }

            //Means we have not received any message for long enough that we should disconnect
            if (timeLastMessageReceived + IdleTimeBeforeDisconnect < Time.time)
            {
                Disconnect(true);  //Disconnect this connection and make attempt at sending a disconnect message
                Debug.Log("JCGConnection disconnect due to idle timeout");
            }

            return Connected;
        }

@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.