Skip to content

Commit 557b34f

Browse files
dereuromarkdereuromark
committed
PhpCollective.ControlStructures.UnneededElse
1 parent f6402a6 commit 557b34f

File tree

6 files changed

+579
-3
lines changed

6 files changed

+579
-3
lines changed

‎PhpCollective/Sniffs/ControlStructures/UnneededElseSniff.php‎

Lines changed: 240 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,240 @@
1+
<?php
2+
3+
/**
4+
* MIT License
5+
* For full license information, please view the LICENSE file that was distributed with this source code.
6+
*/
7+
8+
namespace PhpCollective\Sniffs\ControlStructures;
9+
10+
use PHP_CodeSniffer\Files\File;
11+
use PhpCollective\Sniffs\AbstractSniffs\AbstractSniff;
12+
13+
/**
14+
* Detects and removes unneeded else blocks when all previous branches have early returns.
15+
*
16+
* Too much else is considered a code-smell and can often be resolved by returning early.
17+
* When all if/elseif branches end with return statements (or other early exits like throw, exit, break, continue),
18+
* the else block is unnecessary and can be removed to reduce nesting.
19+
*
20+
* Example:
21+
* ```php
22+
* // Before:
23+
* if ($condition) {
24+
* return 'foo';
25+
* } else {
26+
* $bar = 'baz';
27+
* }
28+
*
29+
* // After:
30+
* if ($condition) {
31+
* return 'foo';
32+
* }
33+
* $bar = 'baz';
34+
* ```
35+
*
36+
* Based on PSR2R\Sniffs\ControlStructures\UnneededElseSniff by Mark Scherer
37+
*/
38+
class UnneededElseSniff extends AbstractSniff
39+
{
40+
/**
41+
* @inheritDoc
42+
*/
43+
public function register(): array
44+
{
45+
return [T_ELSE, T_ELSEIF];
46+
}
47+
48+
/**
49+
* @inheritDoc
50+
*/
51+
public function process(File $phpcsFile, $stackPtr): void
52+
{
53+
$tokens = $phpcsFile->getTokens();
54+
55+
// Check that ALL preceding branches in the if/elseif chain have early exits
56+
if (!$this->allPrecedingBranchesHaveEarlyExits($phpcsFile, $stackPtr)) {
57+
return;
58+
}
59+
60+
$fix = $phpcsFile->addFixableError(
61+
'Unneeded ' . $tokens[$stackPtr]['type'] . ' detected.',
62+
$stackPtr,
63+
'UnneededElse',
64+
);
65+
66+
if (!$fix) {
67+
return;
68+
}
69+
70+
if ($tokens[$stackPtr]['code'] === T_ELSEIF) {
71+
$this->fixElseIfToIf($phpcsFile, $stackPtr);
72+
73+
return;
74+
}
75+
76+
if (empty($tokens[$stackPtr]['scope_opener']) || empty($tokens[$stackPtr]['scope_closer'])) {
77+
return;
78+
}
79+
80+
$phpcsFile->fixer->beginChangeset();
81+
82+
$prevIndex = $phpcsFile->findPrevious(T_WHITESPACE, $stackPtr - 1, null, true);
83+
if ($prevIndex === false) {
84+
$phpcsFile->fixer->endChangeset();
85+
86+
return;
87+
}
88+
89+
$line = $tokens[$prevIndex]['line'];
90+
91+
for ($i = $prevIndex + 1; $i < $stackPtr; $i++) {
92+
$phpcsFile->fixer->replaceToken($i, '');
93+
}
94+
95+
$phpcsFile->fixer->addNewline($prevIndex);
96+
97+
$phpcsFile->fixer->replaceToken($stackPtr, '');
98+
99+
$nextScopeStartIndex = $tokens[$stackPtr]['scope_opener'];
100+
$nextScopeEndIndex = $tokens[$stackPtr]['scope_closer'];
101+
102+
for ($i = $stackPtr + 1; $i < $nextScopeStartIndex; $i++) {
103+
$phpcsFile->fixer->replaceToken($i, '');
104+
}
105+
106+
$prevEndIndex = $phpcsFile->findPrevious(T_WHITESPACE, $nextScopeEndIndex - 1, null, true);
107+
108+
$phpcsFile->fixer->replaceToken($nextScopeStartIndex, '');
109+
$phpcsFile->fixer->replaceToken($nextScopeEndIndex, '');
110+
111+
for ($i = $prevEndIndex + 1; $i < $nextScopeEndIndex; $i++) {
112+
$phpcsFile->fixer->replaceToken($i, '');
113+
}
114+
115+
// Fix indentation
116+
for ($i = $nextScopeStartIndex + 1; $i < $prevEndIndex; $i++) {
117+
if ($tokens[$i]['line'] === $line || $tokens[$i]['type'] !== 'T_WHITESPACE') {
118+
continue;
119+
}
120+
121+
$this->outdent($phpcsFile, $i);
122+
}
123+
124+
$phpcsFile->fixer->endChangeset();
125+
}
126+
127+
/**
128+
* Check if all preceding branches in the if/elseif chain have early exits.
129+
*
130+
* @param \PHP_CodeSniffer\Files\File $phpcsFile
131+
* @param int $stackPtr
132+
*
133+
* @return bool
134+
*/
135+
protected function allPrecedingBranchesHaveEarlyExits(File $phpcsFile, int $stackPtr): bool
136+
{
137+
$tokens = $phpcsFile->getTokens();
138+
139+
// Walk backwards through the if/elseif chain
140+
$currentPtr = $stackPtr;
141+
$iterations = 0;
142+
$maxIterations = 100; // Safety limit to prevent infinite loops
143+
144+
while ($iterations < $maxIterations) {
145+
$iterations++;
146+
147+
$prevScopeEndIndex = $phpcsFile->findPrevious(T_WHITESPACE, $currentPtr - 1, null, true);
148+
if (!$prevScopeEndIndex || empty($tokens[$prevScopeEndIndex]['scope_opener'])) {
149+
return false;
150+
}
151+
152+
$scopeStartIndex = $tokens[$prevScopeEndIndex]['scope_opener'];
153+
$prevScopeLastTokenIndex = $phpcsFile->findPrevious(T_WHITESPACE, $prevScopeEndIndex - 1, null, true);
154+
155+
if ($tokens[$prevScopeLastTokenIndex]['type'] !== 'T_SEMICOLON') {
156+
return false;
157+
}
158+
159+
// Check if this branch has an early exit (return, throw, exit, continue, break)
160+
$returnEarlyIndex = $phpcsFile->findPrevious(
161+
[T_RETURN, T_THROW, T_EXIT, T_CONTINUE, T_BREAK],
162+
$prevScopeLastTokenIndex - 1,
163+
$scopeStartIndex + 1,
164+
);
165+
166+
if (!$returnEarlyIndex) {
167+
return false;
168+
}
169+
170+
// Make sure it's the last statement (no other semicolons after the early exit)
171+
for ($i = $returnEarlyIndex + 1; $i < $prevScopeLastTokenIndex; $i++) {
172+
if ($tokens[$i]['type'] === 'T_SEMICOLON') {
173+
return false;
174+
}
175+
}
176+
177+
// Find the condition token (if/elseif)
178+
$prevParenthesisEndIndex = $phpcsFile->findPrevious(T_WHITESPACE, $scopeStartIndex - 1, null, true);
179+
if (!$prevParenthesisEndIndex || !array_key_exists('parenthesis_opener', $tokens[$prevParenthesisEndIndex])) {
180+
return false;
181+
}
182+
183+
$parenthesisStartIndex = $tokens[$prevParenthesisEndIndex]['parenthesis_opener'];
184+
$prevConditionIndex = $phpcsFile->findPrevious(T_WHITESPACE, $parenthesisStartIndex - 1, null, true);
185+
186+
if (!in_array($tokens[$prevConditionIndex]['code'], [T_IF, T_ELSEIF], true)) {
187+
return false;
188+
}
189+
190+
// If we found an IF, we're done checking (this is the start of the chain)
191+
if ($tokens[$prevConditionIndex]['code'] === T_IF) {
192+
return true;
193+
}
194+
195+
// Ensure we're making progress (moving backwards)
196+
if ($prevConditionIndex >= $currentPtr) {
197+
return false;
198+
}
199+
200+
// Otherwise, it's an ELSEIF, continue checking the previous branch
201+
$currentPtr = $prevConditionIndex;
202+
}
203+
204+
return false;
205+
}
206+
207+
/**
208+
* Convert elseif to if by removing the else part
209+
*
210+
* @param \PHP_CodeSniffer\Files\File $phpcsFile
211+
* @param int $stackPtr
212+
*
213+
* @return void
214+
*/
215+
protected function fixElseIfToIf(File $phpcsFile, int $stackPtr): void
216+
{
217+
$tokens = $phpcsFile->getTokens();
218+
219+
$phpcsFile->fixer->beginChangeset();
220+
221+
$prevIndex = $phpcsFile->findPrevious(T_WHITESPACE, $stackPtr - 1, null, true);
222+
if ($prevIndex === false) {
223+
$phpcsFile->fixer->endChangeset();
224+
225+
return;
226+
}
227+
228+
$indentation = $this->getIndentationWhitespace($phpcsFile, $prevIndex);
229+
230+
for ($i = $prevIndex + 1; $i < $stackPtr; $i++) {
231+
$phpcsFile->fixer->replaceToken($i, '');
232+
}
233+
234+
$phpcsFile->fixer->addNewline($prevIndex);
235+
236+
$phpcsFile->fixer->replaceToken($stackPtr, $indentation . 'if');
237+
238+
$phpcsFile->fixer->endChangeset();
239+
}
240+
}

‎PhpCollective/Sniffs/Formatting/ArrayDeclarationSniff.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -676,7 +676,8 @@ protected function processMultiLineIndentation(File $phpcsFile, int $arrayStart,
676676
$usesTabs = true;
677677

678678
break;
679-
} elseif (strlen($whitespace) > 0) {
679+
}
680+
if (strlen($whitespace) > 0) {
680681
// Count spaces to determine indent size
681682
$spaceCount = strlen(str_replace(["\n", "\r"], '', $whitespace));
682683
if ($spaceCount > 0 && $spaceCount % 4 === 0) {

‎docs/sniffs.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
# PhpCollective Code Sniffer
22

33

4-
The PhpCollectiveStrict standard contains 235 sniffs
4+
The PhpCollectiveStrict standard contains 236 sniffs
55

66
Generic (27 sniffs)
77
-------------------
@@ -48,7 +48,7 @@ PEAR (4 sniffs)
4848
- PEAR.Functions.ValidDefaultValue
4949
- PEAR.NamingConventions.ValidClassName
5050

51-
PhpCollective (82 sniffs)
51+
PhpCollective (83 sniffs)
5252
-------------------------
5353
- PhpCollective.Arrays.ArrayBracketSpacing
5454
- PhpCollective.Arrays.DisallowImplicitArrayCreation
@@ -100,6 +100,7 @@ PhpCollective (82 sniffs)
100100
- PhpCollective.ControlStructures.DisallowCloakingCheck
101101
- PhpCollective.ControlStructures.ElseIfDeclaration
102102
- PhpCollective.ControlStructures.NoInlineAssignment
103+
- PhpCollective.ControlStructures.UnneededElse
103104
- PhpCollective.Formatting.ArrayDeclaration
104105
- PhpCollective.Formatting.MethodSignatureParametersLineBreakMethod
105106
- PhpCollective.Internal.DisallowFunctions

‎tests/PhpCollective/Sniffs/ControlStructures/UnneededElseSniffTest.php‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
<?php
2+
3+
/**
4+
* MIT License
5+
* For full license information, please view the LICENSE file that was distributed with this source code.
6+
*/
7+
8+
namespace PhpCollective\Test\PhpCollective\Sniffs\ControlStructures;
9+
10+
use PhpCollective\Sniffs\ControlStructures\UnneededElseSniff;
11+
use PhpCollective\Test\TestCase;
12+
13+
class UnneededElseSniffTest extends TestCase
14+
{
15+
/**
16+
* Tests that the sniff finds the expected number of errors
17+
*
18+
* Expected errors:
19+
* - simpleReturn: 1 error (line 15 - else)
20+
* - allReturns: 2 errors (line 28 - elseif→if, line 31 - else)
21+
* - noReturnInFirstBranch: 0 errors (should NOT fix - critical test case!)
22+
* - withThrow: 1 error (line 65 - else)
23+
* - withExit: 1 error (line 75 - else)
24+
* - withBreak: 1 error (line 86 - else)
25+
* - withContinue: 1 error (line 98 - else)
26+
* - multipleStatements: 1 error (line 111 - else)
27+
* - nestedStructure: 1 error (line 121 - else)
28+
*
29+
* Total: 9 errors (8 else removals + 1 elseif→if conversion)
30+
*
31+
* @return void
32+
*/
33+
public function testUnneededElseSniffer(): void
34+
{
35+
$this->assertSnifferFindsErrors(new UnneededElseSniff(), 9);
36+
}
37+
38+
/**
39+
* Tests that the fixer correctly removes unneeded else blocks
40+
*
41+
* @return void
42+
*/
43+
public function testUnneededElseFixer(): void
44+
{
45+
$this->assertSnifferCanFixErrors(new UnneededElseSniff());
46+
}
47+
}

0 commit comments

Comments
 (0)