Skip to content

Add ShortenElseIfRector#2094

Merged
TomasVotruba merged 3 commits intorectorphp:masterfrom
keulinho:add-shorten-else-if-rector
Oct 4, 2019
Merged

Add ShortenElseIfRector#2094
TomasVotruba merged 3 commits intorectorphp:masterfrom
keulinho:add-shorten-else-if-rector

Conversation

@keulinho
Copy link
Copy Markdown
Contributor

@keulinho keulinho commented Oct 4, 2019

Fixes #1945

@keulinho keulinho force-pushed the add-shorten-else-if-rector branch from c393833 to 0196d1e Compare October 4, 2019 12:18
Copy link
Copy Markdown
Member

@TomasVotruba TomasVotruba left a comment

Choose a reason for hiding this comment

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

Very nice job!

I've added few comments

return null;
}

$if = $this->refactor($if) ?? $if;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is pretty difficult to understand recursion.
Could you think of more readable syntax?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed the coalesc and made it more verbose,
hopefully that makes it clearer

Do you have any better ideas in mind how to clarify this?

Copy link
Copy Markdown
Member

@TomasVotruba TomasVotruba Oct 4, 2019

Choose a reason for hiding this comment

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

I'm not sure why have you chosen this approach, so I don't even understand the idea to describe it

I always try to avoid calling the self or another Rector in a Rector, but rather decopule a method with meaning captured in it's name

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok the reason behind the recursion is that we get rid of the original nested If_ node, but this rector runs only on If_ nodes.
This means after refactoring the top-level If_ we won't detect any more else/if condition that we could refactor inside that if statement, because now it is an Elseif_ Node.
Consider the example from the recursive fixture:

    public function run()
    {
        if ($this->cond1) {
            $this->doSomething();
        } else {
            if ($this->cond2) {
                $this->doSomething();
            } else {
                if ($this->cond3) {
                    $this->doSomething();
                }
            }
        }
    }

In that case we would move the if ($this->cond2) statement up and combine it to the elseif ($this->cond2). By this refactoring our rector doesn't run for the nested if ($this->cond3), it would get refactored if we run the rector a second time.
Therefore we have to run the refactoring on the inner if ($this->cond3) before applying the refactoring on the outer if ($this->cond3).

I hope this clarifies the need for recursion, i'll try to express this inside the code

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see

I hope this clarifies the need for recursion, i'll try to express this inside the code

That would be perfect, so this comment is not needed to understand the mechanism

Comment thread packages/CodeQuality/src/Rector/If_/ShortenElseIfRector.php
*/
public function refactor(Node $node): ?Node
{
return $this->shortenElseIf($node);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 Thank you

@TomasVotruba TomasVotruba merged commit 4d07eae into rectorphp:master Oct 4, 2019
TomasVotruba added a commit that referenced this pull request Apr 18, 2022
rectorphp/rector-src@8e51a6c [Php74] Skip array type on trait on RestoreDefaultNullToNullableTypePropertyRector (#2094)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CodeQuality] Shorten else/if

2 participants