Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cs] remove unnecesary variables and if conditions #9902

Merged
merged 2 commits into from Mar 15, 2019

Conversation

Projects
None yet
5 participants
@TomasVotruba
Copy link
Contributor

commented Nov 6, 2018

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets -
License MIT

Code cleanup, brought by @Rector with Code Quality level

@TomasVotruba TomasVotruba requested a review from Sylius/core-team as a code owner Nov 6, 2018

Show resolved Hide resolved src/Sylius/Component/Core/Model/Order.php Outdated
Show resolved Hide resolved ...ndle/ChannelBundle/spec/Context/FakeChannel/FakeChannelPersisterSpec.php Outdated
Show resolved Hide resolved src/Sylius/Component/Core/Payment/Provider/OrderPaymentProvider.php
}
return null;
return $this->orderItemUnit;

This comment has been minimized.

Copy link
@pamil

pamil Nov 7, 2018

Member

We could go with return $this->order ?? $this->orderItem ?? $this->orderItemUnit; here

This comment has been minimized.

Copy link
@TomasVotruba

TomasVotruba Nov 8, 2018

Author Contributor

That would suffer the same cognitive overload as https://github.com/Sylius/Sylius/pull/9902/files#r231438056

Code should be as short and as readable as possible.

This comment has been minimized.

Copy link
@pamil

pamil Nov 9, 2018

Member

This is my thought process when reading this code:

  1. Return order if it exists
  2. Return order item if it exists
  3. Return order item unit

When it previously was:

  1. Return order if it exists
  2. Return order item if it exists
  3. Return order item unit if it exists
  4. Return null

This change makes me think whether orderItemUnit is always set or not (so that I need to go up and look it up) and also makes the diff with possible changes bigger (since if you want to add some logic after order item unit is also null, you have to remove than return, switch it into an if and add your logic).

This comment has been minimized.

Copy link
@pamil

pamil Nov 9, 2018

Member

As for ?? usage, I think both $a ?? $b and $a ?: $b are quite idiomatic in PHP.

This comment has been minimized.

Copy link
@TomasVotruba

TomasVotruba Dec 13, 2018

Author Contributor

We could go with return $this->order ?? $this->orderItem ?? $this->orderItemUnit; here

I get it now 👍 Genius 👏

I'll try to implement this to Rector.

Show resolved Hide resolved .../Promotion/Checker/Eligibility/PromotionUsageLimitEligibilityChecker.php
@mamazu

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

Looks good to me.

@TomasVotruba TomasVotruba force-pushed the TomasVotruba:rector-cleanup branch 3 times, most recently from 7626cb2 to 9997539 Dec 13, 2018

@TomasVotruba

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2018

@pamil Thank you for your patience. I had pretty busy months working on Rector fulltime lately, finally found some time to address your comments.

I find these 2 points - rectorphp/rector#424 (comment) - genious. I'll definitely integrate them to Rector.

Let me know if there is something more you need me to do 👍

@stale

This comment has been minimized.

Copy link

commented Mar 13, 2019

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale label Mar 13, 2019

@TomasVotruba

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Closing for no response

@Zales0123

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Hello @TomasVotruba, sorry for such a long response time, we realize that sometime we can loose some valuable PRs in the workload we have. I would say it's still a good change and I would vote to merge. Let me only ping @pamil and @lchrusciel for the quick opinion... It only needs a quick rebase, as far as I see. I'm also reopening it, if you don't mind :)

@Zales0123 Zales0123 reopened this Mar 14, 2019

@stale stale bot removed the Stale label Mar 14, 2019

@Zales0123 Zales0123 added the DX label Mar 14, 2019

@TomasVotruba

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

Hi, thanks for reaching out.
I don't have time for this PR, but sure, go for it! 👍

@lchrusciel
Copy link
Member

left a comment

If we can remove some code, I'm 💯% for it. We should pick it up and merge it :)

@lchrusciel lchrusciel force-pushed the TomasVotruba:rector-cleanup branch from e268455 to 1e1e616 Mar 15, 2019

@lchrusciel

This comment was marked as outdated.

Copy link
Member

commented Mar 15, 2019

The base of this pull-request was changed, you need fetch and reset your local branch
if you want to add new commits to this pull request. Reset before you pull, else commits
may become messed-up.

Unless you added new commits (to this branch) locally that you did not push yet,
execute git fetch origin && git reset "authorize-payment" to update your local branch.

Feel free to ask for assistance when you get stuck 👍

TomasVotruba and others added some commits Nov 6, 2018

Update src/Sylius/Bundle/ChannelBundle/spec/Context/FakeChannel/FakeC…
…hannelPersisterSpec.php

Co-Authored-By: TomasVotruba <tomas.vot@gmail.com>

@lchrusciel lchrusciel changed the base branch from master to 1.3 Mar 15, 2019

@lchrusciel lchrusciel force-pushed the TomasVotruba:rector-cleanup branch from 1e1e616 to 5ac384c Mar 15, 2019

@lchrusciel

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

The base of this pull-request was changed, you need fetch and reset your local branch
if you want to add new commits to this pull request. Reset before you pull, else commits
may become messed-up.

Unless you added new commits (to this branch) locally that you did not push yet,
execute git fetch origin && git reset "rector-cleanup" to update your local branch.

Feel free to ask for assistance when you get stuck 👍

@pamil

pamil approved these changes Mar 15, 2019

@pamil

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

Thank you, Tomáš! 🎉

@pamil pamil merged commit 43a9d10 into Sylius:1.3 Mar 15, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.