Object spawning script Unity

Question

I'm trying to make a random object spawning script in Unity.

Bellow is the code, any suggestions for improvement / changes? I'm new to both Unity and C#.

// Game objects and Transforms
public Transform playerTransform;
public Transform[] obstaclePrefab;
// spawn params
public float minYDicstane = 6.0f;
public float maxYDistance = 11.0f;
private float boxPositionY;
public float minXDistance = 0.0f;
public float maxXDistance = 3.0f;
private float boxPositionX;
private float minSpawnTime;
private float maxSpawnTime;
private float spawnTime;
private float timeCounter;
// Distances
private float playerDistance;
private float boxDistance;
private float ySpread;
private void Start() 
{
 // Count sequence
 minSpawnTime = 3.0f;
 maxSpawnTime = 8.0f;
 // Count timer
 timeCounter = 0;
 spawnTime = Random.Range(minSpawnTime, maxSpawnTime);
}
private void Update() 
{
 // Count the random spawn time
 timeCounter += Time.deltaTime;
 Debug.Log ("Spawn Time: " + spawnTime + " spawnCount: " + timeCounter);
 if(timeCounter >= spawnTime) 
 {
 // boxDistanceFromPlayer
 playerDistance = playerTransform.position.y;
 // Box spawn distance
 // X position
 boxPositionX = Random.Range(minXDistance, maxXDistance);
 boxPositionX = (boxPositionX-1)*2.0f;
 // Y Position
 boxPositionY = playerDistance + Random.Range(minYDicstane, maxYDistance);
 // Select box color
 int boxColor = Random.Range (0, 4);
 // Let the boxes awake!!!
 Instantiate (obstaclePrefab [boxColor], new Vector2 (boxPositionX, boxPositionY), Quaternion.identity);
 // Make new random spawn time
 spawnTime = Random.Range (minSpawnTime, maxSpawnTime);
 timeCounter = 0;
 }
}

Code after changes, using coroutines

// Game objects and Transforms
public Transform playerTransform;
public GameObject[] obstaclePrefab;
// spawn params
public float minYDicstane = 6.0f;
public float maxYDistance = 11.0f;
public float minXDistance = 0.0f;
public float maxXDistance = 3.0f;
public float minSpawnTime = 2.0f;
public float maxSpawnTime = 5.0f;
public float spawnTime = 4.0f;
IEnumerator SpawnBoxes() 
{
 while (true) 
 {
 float boxPositionY;
 float boxPositionX;
 //Distances
 float playerDistance;
 // Player position
 playerDistance = playerTransform.position.y;
 // Box position
 boxPositionX = Random.Range(minXDistance, maxXDistance);
 boxPositionY = playerDistance + Random.Range(minYDicstane, maxYDistance);
 // Select box
 GameObject box = obstaclePrefab[Random.Range(0, obstaclePrefab.Length - 1)];
 // Instantiate box
 Instantiate (box, new Vector2 (boxPositionX, boxPositionY), Quaternion.identity);
 // Coroutine random amount of time
 yield return new WaitForSeconds(Random.Range(minSpawnTime, maxSpawnTime));
 } 
}
private void Start() 
{
 StartCoroutine(SpawnBoxes());
}

• pacmaninbw: Welcome to code review! I hope you get some good answers.

Answer 1

public Transform[] obstaclePrefab;

That's wrong. Prefabs are of type GameObject, not Transform.

---

private float minSpawnTime;
private float maxSpawnTime;

Why did you make these private? They should have been public with default values - just as all the others.

private float boxPositionY;
private float boxPositionX;
// Distances
private float playerDistance;
private float boxDistance;
private float ySpread;

These shouldn't even been private, but local to the Update() method, as their values are never reused.

---

// Select box color
int boxColor = Random.Range(0, 4);

This is very likely to break. Better read the array length instead of hard coding it:

// Select box color
int boxColor = Random.Range(0, obstaclePrefab.Length - 1);

Or just get rid of boxColor all together:

// Select box
GameObject box = obstaclePrefab[Random.Range(0, obstaclePrefab.Length - 1)];

---

// Count the random spawn time
timeCounter += Time.deltaTime;
if(timeCounter >= spawnTime) 
{
 // Make new random spawn time
 spawnTime = Random.Range (minSpawnTime, maxSpawnTime);
 timeCounter = 0;
}

That's one way to do it. The cleaner method would have been to handle this in a Coroutine and then use WaitForSeconds. Right now, your code runs every single frame, without actually doing anything useful.

---

Debug.Log ("Spawn Time: " + spawnTime + " spawnCount: " + timeCounter);

Be careful when you log. Logging when something spawns? OK. But spamming a log entry every single frame? Waste of resources.

---

boxPositionX = (boxPositionX-1)*2.0f;

What is this line supposed to do? That should have been directly computed into minXDistance and maxXDistance, so this line is obsolete.

---

If this is actually supposed to be C#, the whole script you posted should actually have wrapped in a regular C# class:

using UnityEngine;
using System.Collections;
public class ScriptName : MonoBehaviour {
 // <---- Your stuff goes here
}

Did you just omit this when posting your code here, or did you actually write your scripts without it?

• Jsf: Thank you for the feedback :) i just removed the "Boilerplate", when i posted the code here.
• Denis: but local to the Update() method, as their values are never reused. allocating memory every single frame ? That doesn't sounds pretty efficient they are good as they are. Logging is almost never a waste of resources because it will be removed when the application is ready to ship anyway. How can you know if the variables should be private or not by seeing only 1 class ? Most of those private variables seems to be calculated from the class or picked as random which basically cant be done from the inspector thus should stay as private, exposing everything from a class is generally a bad idea.
• Ext3h: @denis He simply exported variables from the function scope to private, without any reuse. Allocating a couple of floats on the stack instead of heap doesn't add any measurable cost, that's why I wrote to properly sort the variables into public and method local scope where they belong, and retain the private modifier only for those variables where state is stored.
• Jsf: @Ext3h Thanks for the feedback / help on the topic. I have now done some changes to the code using coroutine.
• AlexGeorg: Little correction: Prefabs can (at least in the meantime?) be any component you want as long as it is in the root object of your prefab. That is useful - when you are always interested in a specific script (or even transform) it avoids an indirection (.transform) or a GetComponent<> call.