Problems with the NextFreePosition() method

Philip_Thomsen · July 25, 2017, 5:22pm

I have watched through the whole video lots of times now and I can’t figure out what’s causing the problem.
It say that there is an error with my NextFreePosition.

Assets/Scripts/EnemySpawner.cs(84,15): error CS0161: `EnemySpawner.NextFreePosition()’: not all code paths return a value

I am using Microsoft Visual Studio btw. I don’t know if there it is different from Mono Develop. Just thought I would let you know if there is.

Here is my Script:

public GameObject enemyPrefab;
public float width = 10f;
public float height = 5f;
public float speed = 5f;
public float spawnDelay = 0.5f;

private bool movingRight = false;
private float xMax;
private float xMin;

void Start()
{
    float distanceToCamera = transform.position.z - Camera.main.transform.position.z;
    Vector3 leftBoundary = Camera.main.ViewportToWorldPoint(new Vector3(0, 0, distanceToCamera));
    Vector3 rightBoundary = Camera.main.ViewportToWorldPoint(new Vector3(1, 1, distanceToCamera));
    xMax = rightBoundary.x;
    xMin = leftBoundary.x;

    SpawnEnemies();
}

public void OnDrawGizmos()
{
    Gizmos.DrawWireCube(transform.position, new Vector3(width, height));
}

void SpawnUntilFull()
{
    Transform freePosition = NextFreePosition();
    if (freePosition)
    {
        GameObject enemy = Instantiate(enemyPrefab, freePosition.position, Quaternion.identity) as GameObject;
        enemy.transform.parent = freePosition;
    }
    if (NextFreePosition())
    {
        Invoke("SpawnUntilFull", spawnDelay);
    }
}

public void SpawnEnemies()
{
    foreach (Transform child in transform)
    {
        GameObject enemy = Instantiate(enemyPrefab, child.transform.position, Quaternion.identity) as GameObject;
        enemy.transform.parent = child;
    }
}

void Update()
{
    if (movingRight)
    {
        transform.position += Vector3.right * speed * Time.deltaTime;
    }
    else
    {
        transform.position += Vector3.left * speed * Time.deltaTime;
    }

    float rightEdgeOfFormation = transform.position.x + (0.5f * width);
    float leftEdgeOfFormation = transform.position.x - (0.5f * width);

    if (leftEdgeOfFormation < xMin){
        movingRight = true;
    } else if (rightEdgeOfFormation > xMax){
        movingRight = false;
    }

    if (AllMembersDead())
    {
        Debug.Log("members dead");
    }

}

Transform NextFreePosition(){
    foreach (Transform childPositionGameObject in transform){
        if (childPositionGameObject.childCount == 0){
            return childPositionGameObject;
        }
    }
}

bool AllMembersDead()
{
    foreach (Transform childPositionGameObject in transform)
    {
        if (childPositionGameObject.childCount > 0)
        {
            return false;
        }
    }
    SpawnEnemies();
    return true;
}

Dieedi · July 25, 2017, 8:29pm

I’ll try to answer but @rob would be more precise than me …

Once a method has a return type, in your case Transform, you have to return a value in each case.
Yet, you return a value only in your foreach loop and so if your loop doesn’t work or if your condition in the loop isn’t valid, there is no other possibility to return a value.

Exemple :

Bool IsValid (value) {
   if (value == true) {
           return true;
    } else {
        return false;
    }
}

in this case, the method will always return something.

Rob · July 25, 2017, 10:09pm

That is exactly the issue.

I am personally not a fan of having multiple points of return in methods, I think it’s a little bit messy and prone to causing problems…

Alternative;

Whilst it may add a couple more lines of code, I would personally write this out more fully, initially, just as a stub;

bool IsValid(value)
{
    bool result;    // boolean values always default to false so no need to set this here

    return result;
}

So, as a practice or habit to get into, what you have now is the framework of a method which will always return a bool value.

Obviously, at the moment it will also always return false because we haven’t added anything in the middle yet. Your habit should really then include finishing writing the method, before you forget, and then wonder why you always get false

If you are unable to complete it straight away, pop a…

// TODO: I must complete this method

… in the middle… or better still, use something that will definitely cause it to stop running, like this;

bool IsValid(value)
{
    bool result;    // boolean values always default to false so no need to set this here

    throw new NotImplementedException("You really should finish writing the 'IsValid()' method!");

    return result;
}

The above will cause an error to occur as soon as that line is hit.

Now, assuming you practice good habits and continue writing your code, you could have this;

bool IsValid(value)
{
    bool result;    // boolean values always default to false so no need to set this here

    if (value > 0)    // note, this is just an example "condition"
    {
        result = true;
    }

    return result;
}

In the above, because we can rely on the default state of a bool value being false we do not need to worry about adding an else if or else to the if statement to set it to false. We are only interested in the is valid condition here.

Whether the value that is passed is valid or not, this method will always return a bool, true or false.

So, to put that into context with the NextFreePosition() method;

Transform NextFreePosition()
{
    Transform result = null;

    foreach (Transform childPositionGameObject in transform)
    {
        if (childPositionGameObject.childCount == 0){
            result = childPositionGameObject;    // populates the result variable with the valid Transform
            break;    // exits the foreach loop
        }
    }

    return result;
}

In the above example, result is defined as a Transform and set to equal null. This method will now either return a valid Transform, or one that equals null, meaning that you can add some additional validation if you want/need to outside of this method in the calling code for example.

The break; statement simply exits out of the foreach loop, otherwise, you will iterate through more times than is necessary, whereas in the previous code, you exited using the return statement.

To me, whilst the above is a couple of lines of code longer, it is very clear to see exactly what is going on, and what is/will be returned under which conditions.

If you search the interwebs you may find some arguments for either way, it will come down to your personal preference at the end of the day, I prefer to have easy readability and reduced likelihood of errors

Hope the above is of use, some additional links which may be of use below.