Loop At Most One Iteration

ID

csharp.loop_at_most_one_iteration

Severity

high

Remediation Complexity

trivial

Remediation Risk

low

Remediation Effort

low

Resource

Reliability

Language

CSharp

Tags

control-flow, loop, reliability

Description

Reports a for, foreach, while or do loop whose body ends in an unconditional break, return, throw or continue. The first pass always transfers control, so the loop cannot iterate as written.

Rationale

A loop is written to repeat. When the last statement of its body is a jump that no condition guards, the repetition never happens: the body runs once and control leaves. The usual cause is a jump that belongs inside an if but was typed one indentation level too high, which turns "handle every element" into "handle the first element" without any compiler complaint.

A trailing continue is a milder variant of the same mistake — it does not cut the loop short, it simply has no effect, because control was already about to reach the end of the body. Either way the statement misleads whoever reads the loop next.

Only the last statement of the body is examined, so a guarded exit — the ordinary idiom — is never reported. That includes a break inside an if, a break that closes a switch section, a throw inside a try that catches, and a jump that belongs to an inner loop. Wrappers that guard nothing are looked through: a jump ending a using block, a lock block or a try with no catch clause still ends the first iteration and is reported.

A loop whose body holds a conditional continue targeting it is never reported at all. That continue is direct evidence that the loop was written to run more than once, so the trailing jump is the deliberate end of the find-first idiom rather than a misplaced statement.

public string FirstHandler(string[] handlers)
{
    foreach (var handler in handlers)      // FLAW — returns on the first element; the rest are dead
    {
        return handler;
    }
    return null;
}

public void ScanOnce(int[] values)
{
    for (int i = 0; i < values.Length; i++)  // FLAW — the break was meant to be conditional
    {
        Log(values[i]);
        break;
    }
}

public string ReadFirst(string[] paths)
{
    foreach (var path in paths)            // FLAW — the using block guards nothing
    {
        using (var reader = new StreamReader(path))
        {
            return reader.ReadLine();
        }
    }
    return null;
}

public void ScanUntilNegative(int[] values)
{
    foreach (var value in values)          // OK — the break is guarded
    {
        if (value < 0) break;
        Log(value);
    }
}

public string FirstAccepted(Handler[] handlers, string key)
{
    foreach (var handler in handlers)      // OK — the guarded continue proves the loop iterates
    {
        if (!handler.Accepts(key)) continue;
        return handler.Name;
    }
    return null;
}

public void Classify(int[] values)
{
    foreach (var value in values)          // OK — the break closes the switch section
    {
        switch (value)
        {
            case 0:
                Log("zero");
                break;
            default:
                Log("other");
                break;
        }
    }
}

Remediation

Decide what the jump was meant to do. If the loop should stop early on some condition, move the jump into an if that expresses it. If only the first element was ever wanted, drop the loop and index or query the collection directly (for example a FirstOrDefault call), which states the intent plainly. A trailing continue can simply be deleted.

References