Conditionals Start New Lines
ID |
php.conditionals_start_new_lines |
Severity |
high |
Remediation Complexity |
medium |
Remediation Risk |
low |
Remediation Effort |
medium |
Resource |
Code Smell |
Language |
Php |
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 elseif is never reported — the
parser makes it part of the if it belongs to rather than a following sibling — and neither is a
braceless if ($a) if ($b) …, where the inner conditional is the outer one’s body. An if
preceded by any other kind of statement is not reported either, because only a neighbouring
conditional creates the ambiguity this rule is about.
Two conditionals packed onto one line — if ($a) first(); if ($b) second(); — are reported by the
same test: the second still does not start its own line.
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 elseif, all the more so because a
closing brace is exactly what an elseif 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 as
alternatives, the missing else is a live bug and the layout is hiding it. If they were meant as
independent checks, 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.
<?php
class OrderRouter
{
public function route(int $code, bool $urgent): string
{
if ($code === 1) {
$this->note('one');
} if ($code === 2) { // FLAW - reads as elseif; both conditions run
$this->note('two');
}
if ($urgent) {
$this->note('urgent');
}
if ($code > 100) { // OK - on its own line, clearly independent
$this->note('large');
}
if ($code === 5) {
$this->note('five');
} elseif ($code === 6) { // OK - a real else branch
$this->note('six');
}
return implode(',', $this->log);
}
}Remediation
Decide which of the two readings is intended and write it. If the conditions are alternatives, join
them with elseif — 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 match expression states that better than either form.