Loop Counter Wrong Direction

ID

csharp.loop_counter_wrong_direction

Severity

high

Remediation Complexity

trivial

Remediation Risk

low

Remediation Effort

low

Resource

Reliability

Language

CSharp

Tags

control-flow, loop, reliability

Description

Reports a for loop whose counter moves away from the bound its condition tests — the condition caps the counter from above while the update decrements it, or bounds it from below while the update increments it.

Rationale

Each of the three clauses is plausible on its own, so the defect only becomes visible when they are read together. for (int i = 0; i < upperBound; i--) never approaches upperBound: the counter walks away from it. An integral counter does not loop forever — it wraps around at the limit of its type and eventually satisfies the condition — but it takes billions of iterations to get there, which is indistinguishable from a hang, and every iteration in between works on an index the author never intended.

Only updates whose direction is certain are considered: ++ and -- in either position, and += / -= against a positive integer literal. A step whose sign is unknown (i += step) and a loop with no update clause are left alone, as is a converging loop where the condition compares two counters that the update moves towards each other.

public int SumDown(int[] values, int upperBound)
{
    int total = 0;
    for (int i = 0; i < upperBound; i--)            // FLAW — i walks away from upperBound
    {
        total += values[i];
    }
    return total;
}

public void Forward(int limit)
{
    for (int i = 0; i < limit; i++)              // OK — the counter approaches the bound
    {
        Log(i);
    }
}

public void Converging(int limit)
{
    for (int i = 0, j = limit; i < j; i++, j--)  // OK — both ends move towards each other
    {
        Log(i + j);
    }
}

public void UnknownStep(int limit, int step)
{
    for (int i = 0; i < limit; i += step)        // OK — the sign of step is not known here
    {
        Log(i);
    }
}

Remediation

Decide which clause is wrong and fix that one. To count up towards an upper bound, the update must increase the counter (i++); to count down towards a lower bound, it must decrease it (for (int i = upperBound; i > 0; i--)). When the loop walks an indexable collection in reverse, the conventional form is for (int i = values.Length - 1; i >= 0; i--); foreach over a reversed sequence removes the counter altogether.

References