Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions config/set/code-quality/code-quality.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,4 @@ services:
Rector\CodeQuality\Rector\Ternary\ArrayKeyExistsTernaryThenValueToCoalescingRector: ~
Rector\CodeQuality\Rector\Include_\AbsolutizeRequireAndIncludePathRector: ~
Rector\CodeQuality\Rector\FuncCall\ChangeArrayPushToArrayAssignRector: ~
Rector\CodeQuality\Rector\For_\ForRepeatedCountToOwnVariableRector: ~
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
<?php

declare(strict_types=1);

namespace Rector\CodeQuality\Rector\For_;

use PhpParser\Node;
use PhpParser\Node\Expr\Assign;
use PhpParser\Node\Expr\FuncCall;
use PhpParser\Node\Expr\Variable;
use PhpParser\Node\Stmt\For_;
use Rector\Rector\AbstractRector;
use Rector\RectorDefinition\CodeSample;
use Rector\RectorDefinition\RectorDefinition;

/**
* @see \Rector\CodeQuality\Tests\Rector\For_\ForRepeatedCountToOwnVariableRector\ForRepeatedCountToOwnVariableRectorTest
*/
final class ForRepeatedCountToOwnVariableRector extends AbstractRector
{
/**
* @var string
*/
private const DEFAULT_VARIABLE_COUNT_NAME = 'itemsCount';

public function getDefinition(): RectorDefinition
{
return new RectorDefinition('Change count() in for function to own variable', [
new CodeSample(
<<<'PHP'
class SomeClass
{
public function run($items)
{
for ($i = 5; $i <= count($items); $i++) {
echo $items[$i];
}
}
}
PHP
,
<<<'PHP'
class SomeClass
{
public function run($items)
{
$itemsCount = count($items);
for ($i = 5; $i <= $itemsCount; $i++) {
echo $items[$i];
}
}
}
PHP

),
]);
}

/**
* @return string[]
*/
public function getNodeTypes(): array
{
return [For_::class];
}

/**
* @param For_ $node
*/
public function refactor(Node $node): ?Node
{
$countInCond = null;
$countedValueName = null;

$this->traverseNodesWithCallable($node->cond, function (Node $node) use (&$countInCond, &$countedValueName) {
if (! $node instanceof FuncCall) {
return null;
}

if (! $this->isName($node, 'count')) {
return null;
}

$countInCond = $node;
$valueName = $this->getName($node->args[0]->value);

if ($valueName === null) {
$countedValueName = self::DEFAULT_VARIABLE_COUNT_NAME;
} else {
$countedValueName = $valueName . 'Count';
}

Copy link
Copy Markdown
Contributor

@staabm staabm Jan 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the rector make sure that there is no other - already existing variable - in the current scope which might collide with the newly introduced one?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you provide code example?

How would you name new variable in case of conflict?

Copy link
Copy Markdown
Contributor

@staabm staabm Jan 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class SomeClass
 {
     public function run($items)
     {
         $itemsCount = 25;
         for ($i = 5; $i <= count($items); $i++) {
             echo $items[$i];
         }
         If ($itemsCount == 25 ) echo „whoo“;
     }
 }

I guess you would need just make sure the var name is unique, e.g. by appending a 1 or similar (after you checked the final var name is unique)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Covered in #2562

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thx

return new Variable($countedValueName);
});

if ($countInCond === null || $countedValueName === null) {
return null;
}

$countAssign = new Assign(new Variable($countedValueName), $countInCond);

$this->addNodeBeforeNode($countAssign, $node);

return $node;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

namespace Rector\CodeQuality\Tests\Rector\For_\ForRepeatedCountToOwnVariableRector\Fixture;

class FallbackForComplex
{
public function run($items, \stdClass $someObject)
{
for ($i = 5; $i <= count($someObject->getItems() + 10); $i++) {
echo $items[$i];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case the loop body changes the number of elements of the array the count() cannot be calculated beforehand .. could happen when the array get re-assigned, changed by reference or elements get added/deleted

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many theoretical edge cases. Not worth covering unless really exist

}
}
}

?>
-----
<?php

namespace Rector\CodeQuality\Tests\Rector\For_\ForRepeatedCountToOwnVariableRector\Fixture;

class FallbackForComplex
{
public function run($items, \stdClass $someObject)
{
$itemsCount = count($someObject->getItems() + 10);
for ($i = 5; $i <= $itemsCount; $i++) {
echo $items[$i];
}
}
}

?>
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

namespace Rector\CodeQuality\Tests\Rector\For_\ForRepeatedCountToOwnVariableRector\Fixture;

class SomeClass
{
public function run($items)
{
for ($i = 5; $i <= count($items); $i++) {
echo $items[$i];
}
}
}

?>
-----
<?php

namespace Rector\CodeQuality\Tests\Rector\For_\ForRepeatedCountToOwnVariableRector\Fixture;

class SomeClass
{
public function run($items)
{
$itemsCount = count($items);
for ($i = 5; $i <= $itemsCount; $i++) {
echo $items[$i];
}
}
}

?>
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

namespace Rector\CodeQuality\Tests\Rector\For_\ForRepeatedCountToOwnVariableRector\Fixture;

class MethodCallCount
{
public function run($items, \stdClass $someObject)
{
for ($i = 5; $i <= count($someObject->getItems()); $i++) {
echo $items[$i];
}
}
}

?>
-----
<?php

namespace Rector\CodeQuality\Tests\Rector\For_\ForRepeatedCountToOwnVariableRector\Fixture;

class MethodCallCount
{
public function run($items, \stdClass $someObject)
{
$getItemsCount = count($someObject->getItems());
for ($i = 5; $i <= $getItemsCount; $i++) {
echo $items[$i];
}
}
}

?>
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php

declare(strict_types=1);

namespace Rector\CodeQuality\Tests\Rector\For_\ForRepeatedCountToOwnVariableRector;

use Iterator;
use Rector\CodeQuality\Rector\For_\ForRepeatedCountToOwnVariableRector;
use Rector\Testing\PHPUnit\AbstractRectorTestCase;

final class ForRepeatedCountToOwnVariableRectorTest extends AbstractRectorTestCase
{
/**
* @dataProvider provideDataForTest()
*/
public function test(string $file): void
{
$this->doTestFile($file);
}

public function provideDataForTest(): Iterator
{
return $this->yieldFilesFromDirectory(__DIR__ . '/Fixture');
}

protected function getRectorClass(): string
{
return ForRepeatedCountToOwnVariableRector::class;
}
}