pointless variables?

Hello,

I am new to coding and following a tutorial, and I noticed he creates private variables in a method that only get used once. Is this best practice or is it just his style and unnecessary?

Here is an example that he uses for spawning a powerup.

Code (CSharp):

1. IEnumerator SpawnPowerupRoutine()
2.    {
3.        while (player != NULL)
4.        {
5.            Vector3 postToSpawn = New Vector3(Random.Range(-8f, 8f), 7, 0);
6.            Instantiate(powerupPrefab, postToSpawn, Quaternion.identity);
7.            yield return new WaitForSeconds(Random.Range(3, 8));
8.        }
9.    }

to me it seems that there is no reason to add that variable, postToSpawn, and so i built it like this

Code (CSharp):

1.  IEnumerator SpawnPowerup()
2.     {
3.         while (player != NULL)
4.         {
5.             Instantiate(powerupPrefab, new Vector3(Random.Range(-8f, 8f), 7, 0), Quaternion.identity);
6.             yield return new WaitForSeconds(Random.Range(3, 8));
7.         }
8.     }

Again, I am very new to coding and just curious as to why he does it this way. and if i should adopt this habit and create localized variables also, or if i can just keep chugging along on my own rails.

 

Cramming more things into fewer things isn't better.

The main reason to arrange code on separate lines like this is to make debugging easier. If you get an exception on the tutorial's line 5 here, you've got that one chunk of code to analyze to find it. In your code, that same exception would point you to your line 5, where you'd have to dig through 3x as many things to find the issue. (Some exceptions are not terribly specific, like NullReferenceException for example, that makes this kind of thing vital to narrowing down the issue.)

Another reason to separate things like this is that it creates self-documenting code. I can't necessarily figure out at a glance what that random value in your code means, but looking at the top example, I know right away that that's the post to spawn, because the variable name says so.

Having longer lines also means that some portion of your code may leak off the side of your IDE's window, which has the effect of making it harder to see and read quickly.

 

Okay that makes sense. Thank you very much for the quick reply. I've dealt with NullPointerExceptions in my job (I mostly work with SQL so i have to speak with a dev on these), they are a pain.

I am glad i asked as early as i did on this project/tutorial. Now i will go do some code cleanup!

 

Sorry, i have one more question, Since these are routines, would it be better to create a variable for the whole class since the Variables are identical, or should I still keep them independent so they continue to be self documenting withing the routine?

Code (CSharp):

1.     IEnumerator SpawnEnemy()
2.     {
3.         while (_stopSpawn == false)
4.         {
5.             Vector2 postToSpawn = new Vector2(Random.Range(-9.1f, 9.1f), 7);
6.             GameObject newEnemy = Instantiate(_enemyPrefab, postToSpawn, Quaternion.identity);
7.             newEnemy.transform.parent = _enemyContainer.transform;
8.             yield return new WaitForSeconds(5.0f);
9.         }
10.        
11.     }
12.  
13.     IEnumerator SpawnPowerup()
14.     {
15.         while (_stopSpawn == false)
16.         {
17.             Vector2 postToSpawn = new Vector2(Random.Range(-9.1f, 9.1f), 7);
18.             Instantiate(_tripleShotPrefab, postToSpawn, Quaternion.identity);
19.             yield return new WaitForSeconds(Random.Range(7f, 15f));
20.         }
21.     }

 

There are a bunch of ways to structure this. In general it's a good idea to write it to avoid repeated code as much as possible is recommended. That whole randomness line is repeated, so something like:

Code (csharp):

1. IEnumerator SpawnEnemy()
2. {
3.        while (_stopSpawn == false) {
4.             GameObject newEnemy = Instantiate(_enemyPrefab, GetPostToSpawn(), Quaternion.identity);
5.             // ......
6. }
7.  
8. private Vector2 GetPostToSpawn() {
9.     return new Vector2(Random.Range(-9.1f, 9.1f), 7);
10. }

You may also want to replace your 9.1's and your 7 with some public class variables. That'll allow you to reuse this script without changing the code.

 

Okay, Thank you very much @StarManta !!

 

Cramming everything into one line is bad bad bad in my book.

I call that "hairy lines of code" and it is an abomination against good engineering and reasoning about your code.

In fact, I have a special article to help people afflicted by this sickness:

http://plbm.com/?p=248

Let the compiler take care of it... it will do a better job (on average) than any human can anyway. Focus on making your code bulletproof and easy to debug and reason about.

 

Regarding "extra" variables, they aren't really extra. Say you have x+y+z. The computer can only add 2 numbers at a time. It adds x+y and stores that in a secret variable, then adds z. It you decide to write it as A=x+y; B=A+z;, that's not any slower or more extra. It looks weird. but say you have cat+dogs+chairs. It might look nicer as: int animals=cats+dogs; then animals+chairs

The general rule is not to try to write code you think the computer will like, since it's so hard to know. Just write code that looks good to you.

 

hmm, so let me know if i am on base with this, I'm not too sure how you can know what to put for debug.

Code (CSharp):

1.  
2. //This is a variable playerObject that looks for a GameObject that has the "Player" tag
3. GameObject playerObject = GameObject.FindWithTag("Player");
4.  
5. //If it can't find an object with the "Player" tag run this
6. if (playerObject == null)
7. {
8. Debug.Log("FindWithTag returned null for Player tag");
9. }
10.  
11. //this is getting the stats component from the player, so to get this far that means the player was found
12. CharacterStats stats = playerObject.GetComponent<CharacterStats>();
13.  
14. //So here it found the object with player tag, but it is missing stats
15. if (stats == null)
16. {
17. Debug.Log("playerObject.GetComponent returned null");
18. }
19. stats.AddToStrength(5);

 

Okay, i don't know if im sure what you mean, but...

When adding x+y+z the computer is going to do temporaryVariable = x+y, temporaryVariable +z.

So instead of relying on the computer to store the first selection as a temporary answer, we can go ahead and code out the individual segments, so there is more written code, but the computer already processes it that way, so we are just creating more self-documenting like StarManta had mentioned.

That means we are not causing more processor time on the computer, and we are creating an easier breakdown of the code when reviewing and sharing?

 

Going with StarManta's suggestion of breaking out common code into

GetPostToSpawn()

gives you the opportunity to put a single Debug.Log() in that function to print the location selected, and it avoids duplicate code, both of which are good steps.

 

Correct. Computer time is cheap. CPUs get faster and faster every year. Choosing positions during the start of a wave (or when an enemy spawns) is a tiny fraction of work, so even IF the computer was unable to produce the same code (and that's a big if), it is still more valuable to you as a human to have code you can read tomorrow, six months from now, six years from now, etc. or that another person could read and understand.

 

Past many years, CPU don't get much faster anymore, as decade ago. It just gets more cores.
And most games, use single core. Just saying.

 

I tried really hard to figure out how i could accomplish this. And i am not sure how i would go about doing that. When i use this as a method that can be called, i am not sure how to get it to show the spawn location the the game object being instantiated. I build an empty game object that i am using as a spawn manager, so i can get consistent spawning of objects. This is handling enemy spawn and powerup spawns. When i try to get the position, it always returns the position of the span manager.

Code (CSharp):

1.  IEnumerator SpawnEnemy()
2.     {
3.         while (_stopSpawn == false)
4.         {
5.             GameObject newEnemy = Instantiate(_enemyPrefab, GetPosToSpawn(), Quaternion.identity);
6.             newEnemy.transform.parent = _enemyContainer.transform;
7.             yield return new WaitForSeconds(5.0f);
8.         }
9.        
10.     }
11.  
12.     IEnumerator SpawnPowerup()
13.     {
14.         while (_stopSpawn == false)
15.         {
16.             GameObject newPowerup = Instantiate(_tripleShotPrefab, GetPosToSpawn(), Quaternion.identity);
17.             yield return new WaitForSeconds(Random.Range(7f, 15f));
18.         }
19.     }
20.  
21.     private Vector2 GetPosToSpawn()
22.     {
23.         Debug.Log("Object Spawned at " + gameObject.transform.position);
24.         return new Vector2(Random.Range(-9.1f, 9.1f), 7);
25.     }

 

That's because line 23 does exactly that: prints the position of the gameObject that this script is located on!

You don't want that. You want to choose the position, report it, then return it.

Again, use extra variables, break it apart, like so:

Code (csharp):

1.    private Vector2 GetPosToSpawn()
2.     {
3.         // choose position
4.         Vector2 position = Vector2(Random.Range(-9.1f, 9.1f), 7);
5.         // report position
6.         Debug.Log("GetPosToSpawn() has chosen position = " + position);
7.         // return position
8.         return position;
9.     }

I would not normally put such silly comments in there but I wanted to correlate it to what I wrote above it.

 

Okay, Thank you so much Kurt-Dekker. i am glad you broke it down with the comments it really helps me to understand! I really appreciate the help. Better to learn good habits early on.

 

A good compiler will detect and inline these kind of things anyways and remove the variable if it isn't needed.

Im not sure which compiler they use in the current Unity versions, so it may or may not do these kind of things in a Unity build, so if you want to make sure you have to check the (IL) code, make sure to use Release mode if you want to check the IL code, because it will not happen during Debug builds.