Conditionals Start New Lines
ID |
javascript.conditionals_start_new_lines |
Severity |
high |
Remediation Complexity |
medium |
Remediation Risk |
low |
Remediation Effort |
medium |
Resource |
Code Smell |
Language |
JavaScript |
Tags |
code-style, control-flow, readability |
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 purely 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 out of reach — it is the body of the outer statement’s else, a descendant rather than a sibling — and so is a dangling 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, because only a neighbouring conditional creates the else ambiguity.
Two conditionals packed onto one line (if (a) first(); if (b) second();) are reported by the same test: the second one still does not start its own line.
Rationale
// Bad
if (isExpired(token)) {
refresh(token);
} if (isRevoked(token)) {
reject(token);
}-
The two conditionals look like one construct. A reader scanning the left margin sees a single decision and takes the second condition for an
else if— the more so because a closing brace is exactly what anelse ifnormally follows. -
They are independent statements. Both conditions are evaluated, and both bodies run when both hold. If the author meant them as alternatives, the missing
elseis a live bug that the formatting conceals. -
If the author meant them as independent checks, the formatting hides that the first branch falls through into the second — which is exactly the fact a reader needs in order to reason about the combination.
Either reading is a problem, and a line break costs nothing and settles the question. That is why this is reported as a defect and not as a formatting preference: the layout makes two different programs indistinguishable.
Remediation
Put the second conditional on its own line — and if the branches really are alternatives, say so with else.
// Good — independent checks, and it now looks like it
if (isExpired(token)) {
refresh(token);
}
if (isRevoked(token)) {
reject(token);
}
// Good — alternatives, and it now behaves like it
if (isExpired(token)) {
refresh(token);
} else if (isRevoked(token)) {
reject(token);
}