Conditionals Start New Lines

ID

csharp.conditionals_start_new_lines

Severity

high

Remediation Complexity

medium

Remediation Risk

low

Remediation Effort

medium

Resource

Code Smell

Language

CSharp

Tags

code_smell, readability, suspicious-control-flow

Description

Reports an if statement that starts on the very line where the preceding if statement ended, so that a closing brace and the next condition share a line.

The test is positional: the if begins on the line where its immediately preceding sibling statement ended, and that sibling is itself an if. A genuine else if is never reported — it belongs to the outer if rather than following it — and neither is an if preceded by any other kind of statement, because only a neighbouring conditional creates the ambiguity this rule is about.

Rationale

Two conditionals written on one line look like a single construct. A reader scanning the left margin sees one decision and takes the second condition for an else if, all the more so because a closing brace is exactly what an else if normally follows. They are independent statements: both conditions are evaluated, and both bodies run when both hold.

That makes the formatting harmful in either direction. If the two conditions were meant to be alternatives, the missing else is a live bug and the layout is hiding it. If they were meant to be independent, the layout hides that the first branch falls through into the second — precisely the fact a reader needs in order to reason about what happens when both conditions are true. A line break costs nothing and settles the question for good.

public class Router
{
    public string Route(int code, bool urgent)
    {
        if (code == 1)
        {
            Log("one");
        } if (code == 2)                     // FLAW - reads as else if; both conditions run
        {
            Log("two");
        }

        if (urgent)
        {
            Log("urgent");
        }
        if (code > 100)                      // OK - on its own line, clearly independent
        {
            Log("large");
        }

        if (code == 3)
        {
            return "three";
        }
        else if (code == 4)                  // OK - a real else branch
        {
            return "four";
        }

        return "none";
    }

    private void Log(string message) { }
}

Remediation

Decide which of the two readings is intended and write it. If the conditions are alternatives, join them with else — that also stops the second condition being evaluated once the first has matched. If they are genuinely independent checks, move the second if onto its own line so the fall-through is visible. Where a chain of independent conditions on one value is really a dispatch, a switch statement or a switch expression states that better than either form.

References