Enemy patrol script using NavMeshAgent

I'm piecing together an Enemy script to attach to my characters. This is the portion that related the a game objects ability to patrol to given locations. Basically, patrolPoisitions are exactly what they say; they are defined in the editor. This is only the pertinent portion of the code, so there might be some straggler variables. I think I deleted them all though.

I am still very new to game development as well as Unity, so are there any suggestions on how to improve this code?

```
public float BaseAcceleration = 10.0f;
public float BaseSpeed = 5.0f;
public float IncreasedAccelerationFactor = 10.0f;
public float IncreasedSpeedFactor = 5.0f;
public float AlertCoolDownTime = 5.0f;
public float AlertDistance = 15.0f;
public float AttackDistance = 10.0f;
public float StoppingDistance = 0.0f;

private NavMeshAgent navMeshAgent = null;
public List patrolPositions = new List();
public float PatrolHoldTime = 0.0f;
private Vector3 StartPosition;
private int patrolPositionIndex = 0;
private bool isAtPatrolPosition = false;
private Vector3 CurrentDestination;

void Start () {
navMeshAgent = GetComponent ();
StartPosition = transform.position;
}

void Update () {
if (CurrentTarget) {
navMeshAgent.SetDestination (CurrentTarget.position);
} else {
Patrol ();
}

SetNavMeshProperties ();
}

#region Patrol Methods
void Patrol() {
Vector3 Position = transform.position;
if (patrolPositions != null && patrolPositions.Count > 1) {
Vector3 Destination = patrolPositions [patrolPositionIndex];
if (Destination != Position && CurrentDestination != Destination) {
Debug.Log (String.Format ("Patrol plotting route to {0}", Destination.ToString ()));
navMeshAgent.SetDestination (Destination);
CurrentDestination = Destination;

Your indentation and Java-style braces make it very hard to follow. Compare this:

void Patrol() {
            Vector3 Position = transform.position;
            if (patrolPositions != null && patrolPositions.Count > 1) {
                    Vector3 Destination = patrolPositions [patrolPositionIndex];
                    if (Destination != Position && CurrentDestination != Destination) {
                            Debug.Log (String.Format ("Patrol plotting route to {0}", Destination.ToString ()));
                            navMeshAgent.SetDestination (Destination);
                            CurrentDestination = Destination;
                            isAtPatrolPosition = false;
                    } else if (Destination == Position && !isAtPatrolPosition) {
                            isAtPatrolPosition = true;
                            Debug.Log (String.Format ("Patrol waiting {0} seconds", PatrolHoldTime));
                            StartCoroutine (Wait (PatrolHoldTime));
                    }
            } else if (Position != StartPosition) {
                    navMeshAgent.SetDestination (StartPosition);
            }
    }

to that:

void Patrol() 
{
    Vector3 Position = transform.position;
    if (patrolPositions != null && patrolPositions.Count > 1) 
    {
        Vector3 Destination = patrolPositions[patrolPositionIndex];
        if (Destination != Position && CurrentDestination != Destination) 
        {
             Debug.Log(String.Format("Patrol plotting route to {0}", Destination.ToString()));
             navMeshAgent.SetDestination(Destination);
             CurrentDestination = Destination;
             isAtPatrolPosition = false;
        } 
        else if (Destination == Position && !isAtPatrolPosition) 
        {
             isAtPatrolPosition = true;
             Debug.Log(String.Format("Patrol waiting {0} seconds", PatrolHoldTime));
             StartCoroutine(Wait(PatrolHoldTime));
        }
    } 
    else if (Position != StartPosition) 
    {
        navMeshAgent.SetDestination(StartPosition);
    }
}

I'd remove this comment:

// Use this for initialization

And I'd avoid #region blocks - you're not showing your entire class, but regions are typically a sign that too many things are going on, and can often be extracted into their own class (or method, when the region is defined within a method's body).

I don't understand why a method called Wait would return an IEnumerator, and it seems a bit counter-intuitive that Wait would have the side-effect of changing the patrolPositionIndex - could the method have a more descriptive name?

These initializations are redundant, since they're initializing to the default value for their type:

private NavMeshAgent navMeshAgent = null;

public float StoppingDistance = 0.0f;

public float PatrolHoldTime = 0.0f;

private int patrolPositionIndex = 0;

private bool isAtPatrolPosition = false;

StackExchange Code Review Q#73357, answer score: 4