Confusion with targetUnit field in ShootAction

Here is the function GetValidActionGridPositionList inside the ShootAction class.

 public List<GridPosition> GetValidActionGridPositionList(GridPosition unitGridPosition)
    {
        List<GridPosition> validActionGridPositionList = new();

        for (int x = -maxShootDistance; x <= maxShootDistance; x++)
        for (int z = -maxShootDistance; z <= maxShootDistance; z++)
        {
            GridPosition possibleGridPosition = unitGridPosition + new GridPosition(x, z);
            
            // Grid pos is invalid, or doesn't have any unit
            if (!LevelGrid.Instance.GridPosIsValid(possibleGridPosition) ||
                !LevelGrid.Instance.GridPosHasAnyUnit(possibleGridPosition))
                continue;
            
            int distance = unitGridPosition.Distance(possibleGridPosition);
            
            // Grid pos is too far away
            if (distance > maxShootDistance)
                continue;

            Unit theTargetUnit = LevelGrid.Instance.GetUnitAtGridPos(possibleGridPosition);
            
            // Both units are on the same 'team'
            if (theTargetUnit.IsEnemy() == unit.IsEnemy())
                continue;

            Vector3 unitWorldPosition = LevelGrid.Instance.GetWorldPos(unitGridPosition);
            Vector3 shootDir = theTargetUnit.GetWorldPosition() - unitWorldPosition.normalized;
            
            const float unitShoulderHeight = 1.7f;
            bool obstacleHit = Physics.Raycast(unitWorldPosition + Vector3.up * unitShoulderHeight,
                shootDir,
                Vector3.Distance(unitWorldPosition, theTargetUnit.GetWorldPosition()),
                obstaclesLayerMask);

            if (obstacleHit)
                continue;
            
            validActionGridPositionList.Add(possibleGridPosition);
        }

        return validActionGridPositionList;
    }

I have it almost identical as Hugo´s except for one modification I made on purpose. I changed this variable name Unit theTargetUnit = LevelGrid.Instance.GetUnitAtGridPos(possibleGridPosition); from targetUnit to theTargetUnit (couldnt come up with a better name).
The reason I did this is related to my question. Before renaming, my IDE warned me that the name targetUnit was hiding a field with the same name in the class. That is indeed the case.

So my question is, why did we declare a variable in the scope of only this method with the same name? I see two options:

  1. This variable is not related to the field of the class, in that case I would prefer for it to have a different name, to avoid this confusion
  2. This variable is exactly the same as the field in the class, in that case I would not make it a new variable in this scope, and just assign the field of the class.

Thats basically my question, is the case one of the above, or other that I couldnt think of.

Thanks!

For the purposes of the method itself, either targetUnit or theTargetUnit will work, since we’re declaring this variable within the method, the local version will be used and the method works as intended. This is because local variables will always hide instance variables of the same name.
You are right that this can become confusing, and a common mistake made by new programmers is to declare the variable type (thus creating a local variable) within a method when they really did intend to access the instance variable. A great example of this is

Health health;

void Awake()
{
    Health health = GetComponent<Health>();
}

void Update()
{
    Debug.Log($"Health is currently {health}");  // this will throw a null reference
}

In this case, we’re not using this value outside of the method, it’s just a placeholder as we iterate through the possible GridPositions. No errors or odd logic will occur because of masking the instance variable.

That being said, it is often easier to read the code and avoid confusion when we use the same name in an instance and as a local in the method. If you’re more comfortable using a unique name, then not only is there no harm in doing so, but you might thank yourself in six months when you’re going over the code an miss that this is a local variable masking the instance variable.

Perfectly clear. Thanks!

This is one of the reasons we (not everyone, but at least my team) prefix class level variables with an underscore.

3 Likes

This topic was automatically closed 24 hours after the last reply. New replies are no longer allowed.

Privacy & Terms