3
\$\begingroup\$

I have these scripts I made that work absolutely perfectly as I need them to. Is there any improvements that can be done to these scripts and make them more efficient for Mobile Platforms? Or even just combine them into one script?

These 4 scripts are just about identical except that each one controls a different obstacle and I have it that way so I can have more control over each individual obstacle. Does that make sense? (For instance, I want one obstacle to start spawning 10 seconds into the game and another to spawn RIGHT when the game starts).

Obstacle 1:

using UnityEngine;
using System.Collections;

[System.Serializable] // Serialize this so it looks neater in the inspector 

public class Obstacle1 // Hammer Obstacle 
{

    public GameObject hammer; // The first obstacle gameobject. This is attached in the inspector.
    public Vector3 spawnHPosValues; // Where the first obstacle will be spawned at on the X,Y,Z plane. 
    public int hCount; // This is the count of the first obstacle in a given wave. //
    public float hSpawnWait; // Time in seconds between next wave of obstacle 1.
    public float hStartGameWait; // Time in seconds between when the game starts and when the first obstacle start spawning.
    public float hWaveSpawnWait; // Time in seconds between waves when the next wave of obstacle 1 will spawn.
}


public class SpawnHammers : MonoBehaviour {

    public Obstacle1 obstacle1; // Reference to the Obstacle 1 class //

    void Start () {

        StartCoroutine (SpawnHammer ()); // at the beginning of the game, start to function SpawnHammer() //

}

    IEnumerator SpawnHammer () {

        yield return new WaitForSeconds(obstacle1.hStartGameWait);
        while (true)
        {

            for (int i = 0; i < obstacle1.hCount; i++) {

                Vector3 spawnPosition = new Vector3 (Random.Range(-obstacle1.spawnHPosValues.x, obstacle1.spawnHPosValues.x), 
                                                     obstacle1.spawnHPosValues.y, 
                                                     obstacle1.spawnHPosValues.z); // I choose where I spawn the obstacles on the X,Y,Z planes. Only the X
                                                                                   // plane will be random
                Quaternion spawnRotation = Quaternion.identity; // Spawn with NO rotation
                Instantiate (obstacle1.hammer, spawnPosition, spawnRotation); // Bring the object into the scene
                yield return new WaitForSeconds(obstacle1.hSpawnWait); // Spawn a new obstacle
            }
            yield return new WaitForSeconds (obstacle1.hWaveSpawnWait); // Start the new wave of obstacles
        }
    }
}

Obstacle 2:

using UnityEngine;
using System.Collections;

[System.Serializable] 

public class Obstacle2 // Road Barrier Obstacle 
{

    public GameObject roadBarrier; // The second obstacle gameobject. This is attached in the inspector.
    public Vector3 spawnRBPosValues; // Where the second obstacle will be spawned at on the X,Y,Z plane. 
    public int rbCount; // This is the count of the second obstacle in a given wave. //
    public float rbSpawnWait; // Time in seconds between next wave of obstacle 2.
    public float rbStartGameWait; // Time in seconds between when the game starts and when the second obstacle will start spawning.
    public float rbWaveSpawnWait; // Time in seconds between waves when the next wave of obstacle 2 will spawn.
}


public class SpawnRoadBarriers : MonoBehaviour {

    public Obstacle2 obstacle2;

    void Start () {

        StartCoroutine (SpawnRoadBarrier ());

    }

    IEnumerator SpawnRoadBarrier () {

        yield return new WaitForSeconds(obstacle2.rbStartGameWait);
        while (true)
        {

            for (int i = 0; i < obstacle2.rbCount; i++) {

                Vector3 spawnPosition = new Vector3 (Random.Range(-obstacle2.spawnRBPosValues.x, obstacle2.spawnRBPosValues.x), 
                                                     obstacle2.spawnRBPosValues.y, 
                                                     obstacle2.spawnRBPosValues.z); 
                Quaternion spawnRotation = Quaternion.identity; 
                Instantiate (obstacle2.roadBarrier, spawnPosition, spawnRotation);
                yield return new WaitForSeconds(obstacle2.rbSpawnWait);
            }
            yield return new WaitForSeconds (obstacle2.rbWaveSpawnWait);
        }
    }
}

Obstacle 3:

using UnityEngine;
using System.Collections;

[System.Serializable]

public class Obstacle3 // Cone Obstacle
{
    public GameObject cone; // The third obstacle gameobject. This is attached in the inspector.
    public Vector3 spawnCPosValues; // Where the third obstacle will be spawned at on the X,Y,Z plane. 
    public int cCount; // This is the count of the third obstacle in a given wave.
    public float cSpawnWait; // Time in seconds between next wave of obstacle 3.
    public float cStartGameWait; // Time in seconds between when the game starts and when the third obstacle start spawning.
    public float cWaveSpawnWait; // Time in seconds between waves when the next wave of obstacle 3 will spawn.
}

public class SpawnCones : MonoBehaviour {

    public Obstacle3 obstacle3;

    void Start () {

        StartCoroutine (SpawnCone ());
    }


    IEnumerator SpawnCone () {

        yield return new WaitForSeconds(obstacle3.cStartGameWait);
        while (true)
        {

            for (int i = 0; i < obstacle3.cCount; i++) {

                Vector3 spawnPosition = new Vector3 (Random.Range(-obstacle3.spawnCPosValues.x, obstacle3.spawnCPosValues.x), 
                                                     obstacle3.spawnCPosValues.y, 
                                                     obstacle3.spawnCPosValues.z);
                Quaternion spawnRotation = Quaternion.Euler(0f,0f,90f); // rotate 90 degrees on the Z axis   
                Instantiate (obstacle3.cone, spawnPosition, spawnRotation);
                yield return new WaitForSeconds(obstacle3.cSpawnWait);
            }
            yield return new WaitForSeconds (obstacle3.cWaveSpawnWait);
        }
    } 
}

Obstacle 4:

using UnityEngine;
using System.Collections;

[System.Serializable]

public class Obstacle4 // Barrel Obstacle
{
    public GameObject barrel; // The fourth obstacle gameobject. This is attached in the inspector.
    public Vector3 spawnBPosValues; // Where the fourth obstacle will be spawned at on the X,Y,Z plane. 
    public int bCount; // This is the count of the fourth obstacle in a given wave.
    public float bSpawnWait; // Time in seconds between next wave of obstacle 4.
    public float bStartGameWait; // Time in seconds between when the game starts and when the fourth obstacle start spawning.
    public float bWaveSpawnWait; // Time in seconds between waves when the next wave of obstacle 4 will spawn.
}

public class SpawnBarrels : MonoBehaviour {

    public Obstacle4 obstacle4;

    void Start () {

        StartCoroutine (SpawnBarrel ());
    }


    IEnumerator SpawnBarrel () {

        yield return new WaitForSeconds(obstacle4.bStartGameWait);
        while (true)
        {

            for (int i = 0; i < obstacle4.bCount; i++) {

                Vector3 spawnPosition = new Vector3 (Random.Range(-obstacle4.spawnBPosValues.x, obstacle4.spawnBPosValues.x), 
                                                     obstacle4.spawnBPosValues.y, 
                                                     obstacle4.spawnBPosValues.z);
                Quaternion spawnRotation = Quaternion.Euler(0f,0f,90f); // rotate 90 degrees on the Z axis   
                Instantiate (obstacle4.barrel, spawnPosition, spawnRotation);
                yield return new WaitForSeconds(obstacle4.bSpawnWait);
            }
            yield return new WaitForSeconds (obstacle4.bWaveSpawnWait);
        }
    } 
}
\$\endgroup\$
  • \$\begingroup\$ @Jamal did you edit my post? \$\endgroup\$ – Stephen George May 15 '16 at 3:01
  • \$\begingroup\$ Yes I did. Why? \$\endgroup\$ – Jamal May 15 '16 at 3:03
  • \$\begingroup\$ I just got this notification and got curious lol @Jamal \$\endgroup\$ – Stephen George May 15 '16 at 23:33
3
\$\begingroup\$
  1. Bracket alignment. For some reason, all obstacle classes and while-loops are written with vertically aligned bracket, while the MonoBehaviours are egyptians.

    [Coding style] create a consistent look to the code, so that readers can focus on content, not layout. - C# Coding Convention

    And, the commonly agreed convention for this one is vertically aligned,

  2. Naming.

    1. Do not abbreviate the variable name... Go for the full name, with exception of some short lambda functions', event handlers' parameters and for-loops' variables.

      At first, I thought they were some kind of hungarian notation, and had trouble figuring out what kind of handles are they? It was until I saw the next obstacle class that I realize it was h for Hammer!

      public class Obstacle1 // Hammer Obstacle
      {
          public GameObject hammer; // The first obstacle gameobject. This is attached in the inspector.
          public Vector3 spawnHPosValues; // Where the first obstacle will be spawned at on the X,Y,Z plane.
          public int hCount; // This is the count of the first obstacle in a given wave. //
          public float hSpawnWait; // Time in seconds between next wave of obstacle 1.
          public float hStartGameWait; // Time in seconds between when the game starts and when the first obstacle start spawning.
          public float hWaveSpawnWait; // Time in seconds between waves when the next wave of obstacle 1 will spawn.
      }
      

      So, do use the full name, like spawnHammerPosition, hammerCount and so on... It does irk a bit, but I will refactor this with other Obstacles later.

    2. In this bit of code

      var spawnRotation = Quaternion.identity; // Spawn with NO rotation
      Instantiate(hammer.GameObject, spawnPosition, spawnRotation); // Bring the object into the scene
      

      Instead of spawnRotation, you could have gone with noRotation and gotten rid of the comment.

      var noRotation = Quaternion.identity;
      Instantiate(hammer.GameObject, spawnPosition, noRotation); // Bring the object into the scene
      

      Or, if you are proficient enough, just inline it.

    3. Normally, you should use PascalCase for public menbers, and not camelCase. However, Unity3D seems favor the latter. Therefore, it is up to you to decide. (Quaternion.identity *twiches eye*)

    4. When your method name is already SpawnObstacles, you don't also have to name the local variable spawnPosition, just position should suffice.
    5. Class name should never be plurial, and never a verb or verb+complement.

      This distinguishes type names from methods, which are named with verb phrases. - Names of Classes, Structs, and Interfaces

      SpawnHammers -> HammerSpawner

  3. I don't understand, why you have 4 different classes with the same properties and copy-pasta comments, and, most importantly, shareing the same role of obstacle.

    Combine them into a single class, subclass if you had to keep the class name for distinction :

    [Serializable]
    public abstract class Obstacle
    {
        public GameObject gameObject;
        public Vector3 spawnPosition;
        public int spawnCount;
        public float spawnWait;
        public float startGameWait;
        public float waveSpawnWait;
    }
    
    public class Hammer : Obstacle { }
    public class RoadBarrier : Obstacle { }
    public class Cone : Obstacle { }
    public class Barrel : Obstacle { }
    
  4. Try keep the line width not too long and don't align the parameters :

    // compare these two :
    Vector3 spawnPosition = new Vector3(Random.Range(-cone.spawnCPosValues.x, cone.spawnCPosValues.x),
                                        cone.spawnCPosValues.y,
                                        cone.spawnCPosValues.z);
    Vector3 spawnPosition = new Vector3(
        Random.Range(-cone.spawnCPosValues.x, cone.spawnCPosValues.x),
        cone.spawnCPosValues.y,
        cone.spawnCPosValues.z);
    
  5. As you have already noticed, all 4 spawners' implementation are quite repetitive, namely the waiting part. However, each obstacle has its own spawn rotation and position. So, we will have to extract the former while isolating the latter :

    public abstract class ObstacleSpawnerBase : MonoBehaviour
    {
        public Obstacle obstacle;
    
        void Start()
        {
            StartCoroutine(SpawnObstacles());
        }
    
        // the repeated pattern in SpawnXXXXs has been extracted to here
        IEnumerator SpawnObstacles()
        {
            yield return new WaitForSeconds(obstacle.startGameWait);
    
            while (true)
            {
                foreach (var obstacle in SpawnObstaclesImpl())
                    yield return new WaitForSeconds(obstacle.spawnWait);
    
                yield return new WaitForSeconds(obstacle.waveSpawnWait);
            }
        }
    
        // with the actual implementation detail separated for customization
        protected abstract IEnumerable<WaitForSeconds> SpawnObstaclesImpl();
    }
    

    And the concrete obstacle spawner classes :

    public class HammerSpawner : ObstacleSpawnerBase
    {
        protected override IEnumerable SpawnObstaclesImpl()
        {
            for (int i = 0; i < obstacle.spawnCount; i++)
            {
                // randomize x-axis
                var position = new Vector3(
                    Random.Range(-obstacle.spawnPosition.x, obstacle.spawnPosition.x),
                    obstacle.spawnPosition.y,
                    obstacle.spawnPosition.z);
    
                // bring the object into the scene
                yield return Instantiate(obstacle.gameObject, position, Quaternion.identity);
            }
        }
    }
    public class RoadBarrierSpawner : ObstacleSpawnerBase
    {
        protected override IEnumerable SpawnObstaclesImpl()
        {
            for (int i = 0; i < obstacle.spawnCount; i++)
            {
                var position = new Vector3(
                    Random.Range(-obstacle.spawnPosition.x, obstacle.spawnPosition.x),
                    obstacle.spawnPosition.y,
                    obstacle.spawnPosition.z);
    
                yield return Instantiate(obstacle.gameObject, position, Quaternion.identity);
            }
        }
    }
    public class ConeSpawner : ObstacleSpawnerBase
    {
        protected override IEnumerable SpawnObstaclesImpl()
        {
            for (int i = 0; i < obstacle.spawnCount; i++)
            {
                var position = new Vector3(
                    Random.Range(-obstacle.spawnPosition.x, obstacle.spawnPosition.x),
                    obstacle.spawnPosition.y,
                    obstacle.spawnPosition.z);
    
                // rotate 90 degrees on the Z axis 
                var rotation = Quaternion.Euler(0f, 0f, 90f);
    
                yield return Instantiate(obstacle.gameObject, position, rotation);
            }
        }
    }
    public class BarrelSpawner : ObstacleSpawnerBase
    {
        protected override IEnumerable SpawnObstaclesImpl()
        {
            for (int i = 0; i < obstacle.spawnCount; i++)
            {
                var position = new Vector3(
                    Random.Range(-obstacle.spawnPosition.x, obstacle.spawnPosition.x),
                    obstacle.spawnPosition.y,
                    obstacle.spawnPosition.z);
    
                // rotate 90 degrees on the Z axis 
                var rotation = Quaternion.Euler(0f, 0f, 90f);
    
                yield return Instantiate(obstacle.gameObject, position, rotation);
            }
        }
    }
    

    Normally, I would use generic here, but Unity3D seems to have some problems with generic MonoBehaviour, so I left it out. However, do give them a try :

    public abstract class ObstacleSpawnerBase<TObstacle> : MonoBehaviour
        where TObstacle : Obstacle
    {
        // ...
    }
    public class HammerSpawner : ObstacleSpawnerBase<Hammer> { /*...*/ }
    public class RoadBarrierSpawner : ObstacleSpawnerBase<RoadBarrier> { /*...*/ }
    public class ConeSpawner : ObstacleSpawnerBase<Cone> { /*...*/ }
    public class BarrelSpawner : ObstacleSpawnerBase<Barrel> { /*...*/ }
    
\$\endgroup\$
  • 1
    \$\begingroup\$ @StephenGeorge Yes, twice. It was a busy day at work today. Generally, you should have 1 class per file. However, some exceptions can be made. Like the 4 obstacle classes here are relatively short in lines of code and are closely related to ObstacleSpawnerBase, so you could choose to place them in a single file : 'ObstacleSpawners.cs` \$\endgroup\$ – Xiaoy312 May 16 '16 at 20:24
  • 1
    \$\begingroup\$ I don't really check my email often. But, you can leave your related questions here, and I'll update my answer. \$\endgroup\$ – Xiaoy312 May 16 '16 at 20:40
  • 1
    \$\begingroup\$ Easy one, simple copy-pasting mistake. Yeah, it should be ObstacleSpawnerBase. \$\endgroup\$ – Xiaoy312 May 16 '16 at 20:49
  • 1
    \$\begingroup\$ Just upload them to imgur and link the album? \$\endgroup\$ – Xiaoy312 May 16 '16 at 21:54
  • 1
    \$\begingroup\$ IEnumerable<T> is living under Namespace: System.Collections.Generic. I don't know why it is complaining about CS0308, "The non-generic type-or-method 'identifier' cannot be used with type arguments"... Anyways you should change IEnumerable<WaitForSeconds> to IEnumerable in ObstacleSpawnerBase. And if that didn't work, force it to recompile, or restart unity3d. if all fail, try fix #2 : gist.github.com/Xiaoy312/f600c48d814982bccac1ed80586efbff \$\endgroup\$ – Xiaoy312 May 16 '16 at 22:22
0
\$\begingroup\$

You only need one Obstacle class, and then you can create four instances of it. Your "Spawn" methods can be extracted into a single method, with parameters to handle the differences. For example, because spawnRotation not the same for each obstacle, you can have that be a parameter or a variable in the Obstacle class.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.