Resolved Could this script be improved ?

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.  

 

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.

 

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.

 

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

 

Yeah, it would... just set xPhp to zero before the loop.

 

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.

 

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.

 

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

 

Yes, I use all other values. As you see in last line.
Now, with this script all is good. Thank you StarManta.