Search Unity

  1. Good news ✨ We have more Unite Now videos available for you to watch on-demand! Come check them out and ask our experts any questions!
    Dismiss Notice

Resolved Could this script be improved ?

Discussion in 'Scripting' started by Quast, Jun 17, 2020.

  1. Quast

    Quast

    Joined:
    Jul 5, 2015
    Posts:
    521
    Hi.
    I wrote a simple script to see how many player are dead in my array. It do the job but I see it bad and maybe will cost some frames later. Could this be improved ?
    Code (CSharp):
    1.     public GameObject[] player;
    2.  
    3.     public int xPhp,xPhp0,xPhp1,xPhp2;
    4.  
    5.     // Use this for initialization
    6.     void Start()
    7.     {
    8.         player = GameObject.FindGameObjectsWithTag("Player");
    9.     }
    10.  
    11.     // Update is called once per frame
    12.     void Update()
    13.     {
    14.         if (player.Length == 1)
    15.         {
    16.             if (player[0].GetComponent<PlayerCtrl>().PlayerHealthy <= 0)
    17.             {
    18.                 xPhp0 = 1;
    19.             }
    20.             else
    21.             {
    22.                 xPhp0 = 0;
    23.             }
    24.  
    25.         }
    26.         if (player.Length == 2)
    27.         {
    28.             if (player[0].GetComponent<PlayerCtrl>().PlayerHealthy <= 0)
    29.             {
    30.                 xPhp0 = 1;
    31.             }
    32.             else
    33.             {
    34.                 xPhp0 = 0;
    35.             }
    36.  
    37.             if (player[1].GetComponent<PlayerCtrl>().PlayerHealthy <= 0)
    38.             {
    39.                 xPhp1 = 1;
    40.             }
    41.             else
    42.             {
    43.                 xPhp1 = 0;
    44.             }
    45.  
    46.         }
    47.         if (player.Length == 3)
    48.         {
    49.             if (player[0].GetComponent<PlayerCtrl>().PlayerHealthy <= 0)
    50.             {
    51.                 xPhp0 = 1;
    52.             }
    53.             else
    54.             {
    55.                 xPhp0 = 0;
    56.             }
    57.             if (player[1].GetComponent<PlayerCtrl>().PlayerHealthy <= 0)
    58.             {
    59.                 xPhp1 = 1;
    60.             }
    61.             else
    62.             {
    63.                 xPhp1 = 0;
    64.  
    65.             }
    66.             if (player[2].GetComponent<PlayerCtrl>().PlayerHealthy <= 0)
    67.             {
    68.                 xPhp2 = 1;
    69.             }
    70.             else
    71.             {
    72.                 xPhp2 = 0;
    73.  
    74.             }
    75.         }
    76.         xPhp = xPhp0 + xPhp1 + xPhp2;
    77.    }
    78.  
     
  2. Kurt-Dekker

    Kurt-Dekker

    Joined:
    Mar 16, 2013
    Posts:
    9,085
    I would use an array to iterate the individual
    xPhp#
    variables, then tally them up to
    xPhp
    afterwards.

    Also, if players will mostly be "looked at" by your code as a PlayerCtrl, make it an array of PlayerCtrl instead of an array of GameObject in Start(), save yourself some runtime .GetComponent<>() calls.

    Also, if something is marked public, that implies someone else either sets it or needs it. You are setting the players array in Start() so unless someone else needs it to, keep it private. Also, if the
    xPhp#
    variables are just intermediates, make them private too, or if you are iterating with an array, just tally it, don't keep the results.
     
  3. StarManta

    StarManta

    Joined:
    Oct 23, 2006
    Posts:
    8,009
    The easiest way to improve the script would be with a loop. You can get rid of xPhp0, xPhp1, etc, and drastically shorten your Update function:
    Code (csharp):
    1. void Update() {
    2.    for (int p=0; p<player.Length; p++) {
    3.       if (player[p].GetComponent<PlayerCtrl>().PlayerHealthy <= 0) {
    4.          xPhp++;
    5.       }
    6.    }
    7. }
    It's not likely to "cost frames", but it will definitely cause headaches if you ever, say, add one additional player, or if you need to adjust what happens per player object in this loop.
     
    PraetorBlue and Kurt-Dekker like this.
  4. mgear

    mgear

    Joined:
    Aug 3, 2010
    Posts:
    6,240
    ^ That would keep incrementing the xPhp all the time though? (if 1 or more players are dead)

    to avoid getcomponent, array could be type of that script (and would need to adjust Start() to find those player scripts)
    Code (CSharp):
    1. public PlayerCtrl>[] player;
    or to avoid update loop completely,
    could try something where PlayerCtrl script sends message (when health value goes below 0).
    to some main game/manager script, which keeps count of the alive and dead players.
     
  5. Kurt-Dekker

    Kurt-Dekker

    Joined:
    Mar 16, 2013
    Posts:
    9,085
    Yeah, it would... just set xPhp to zero before the loop.
     
    StarManta likes this.
  6. Quast

    Quast

    Joined:
    Jul 5, 2015
    Posts:
    521
    For the second point, Yes you right. But my C# language is not that good. so I'm following my basic rules that easy to understand and readable. In case this will save me some frames I will change it. Thank you ^-^.
    For third point, again you right. I use public to see the result. Later I will change public to privet when I see I done from it.
    Thank you for your replay.
    xPhp++;
    this will not set value between 0-1.
    I understand your point and yes I can do that. But if I could improve this will be good.
    Before the loop ! I did not understand you.
     
  7. WarmedxMints

    WarmedxMints

    Joined:
    Feb 6, 2017
    Posts:
    1,034
    There really isn't any need to check each frame if they are alive or not. You could just use an event or call a method and have the PlayerHeathly component report when its dead or not.
     
  8. StarManta

    StarManta

    Joined:
    Oct 23, 2006
    Posts:
    8,009
    Let me ask: do you use all the other values? (xPhp0, xPhp1, xPhp2) I wrote this on the assumption that you were only using them as a means to get the total number of dead players for the main xPhp variable.
    If you do use them, you should replace them with an int[] array, which you create every time you run this code:
    Code (csharp):
    1. public int[] xPhpParts;
    2. void Update() {
    3.    xPhp = 0; //per the correct suggestion above
    4.    xPhpParts = new int[player.Length];
    5.    for (int p=0; p<player.Length; p++) {
    6.       if (player[p].GetComponent<PlayerCtrl>().PlayerHealthy <= 0) {
    7.          xPhpParts[p] = 1;
    8.          xPhp++;
    9.       }
    10.    }
    11. }
     
    Quast likes this.
  9. Quast

    Quast

    Joined:
    Jul 5, 2015
    Posts:
    521
    Yes, I use all other values. As you see in last line.
    Now, with this script all is good. Thank you StarManta.
     
unityunity