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

remove Yoda condition #5381

Merged
merged 1 commit into from Jun 27, 2015

Conversation

Projects
None yet
5 participants
@greg0ire
Contributor

greg0ire commented Jun 11, 2015

Yoda conditions : a questionable best practice that makes code less error-prone, but also harder to read for people that don't share the same grammar rules as people from Dagobah. Anyway, they are useful when you want to avoid confusing assignment and equality operators, but I doubt people often write = while meaning to write <=.

remove Yoda condition
Yoda conditions : a questionable best practice that makes code less error-prone, but also harder to read for people that don't share the same grammar rules as on Dagobah. Anyway, they are useful when you want to avoid confusing assignment and equality operators, but I doubt people often write `=` while meaning to write `<=`.
@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Jun 11, 2015

Member

@greg0ire as you may know, the source code of Symfony uses this style of coding. However, in this particular case, I totally agree with you. The non-Yoda conditional is much easier to understand. Let's see what others think about this.

Member

javiereguiluz commented Jun 11, 2015

@greg0ire as you may know, the source code of Symfony uses this style of coding. However, in this particular case, I totally agree with you. The non-Yoda conditional is much easier to understand. Let's see what others think about this.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Jun 11, 2015

Member

This is perfectly fine here. This is comparing a constant and the return value of a method call, so the Yoda condition does not add benefit here.
The benefit applies only when one side of the comparison is a variable (which should then be on the right). We don't apply it in the source code in other cases than that (we don't enforce it at least. There might still be occurences)

Member

stof commented Jun 11, 2015

This is perfectly fine here. This is comparing a constant and the return value of a method call, so the Yoda condition does not add benefit here.
The benefit applies only when one side of the comparison is a variable (which should then be on the right). We don't apply it in the source code in other cases than that (we don't enforce it at least. There might still be occurences)

@greg0ire

This comment has been minimized.

Show comment
Hide comment
@greg0ire

greg0ire Jun 11, 2015

Contributor

@stof : this PR removes the yodaism, not the other way around.

This is perfectly fine here

Does "here" mean before or after the P.R. ? It feels like you mean before…

Contributor

greg0ire commented Jun 11, 2015

@stof : this PR removes the yodaism, not the other way around.

This is perfectly fine here

Does "here" mean before or after the P.R. ? It feels like you mean before…

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Jun 11, 2015

Member

I mean the change is fine here (i.e. in this kind of code snippet). Sorry for being unclear. My vote is a 👍

Member

stof commented Jun 11, 2015

I mean the change is fine here (i.e. in this kind of code snippet). Sorry for being unclear. My vote is a 👍

@greg0ire

This comment has been minimized.

Show comment
Hide comment
@greg0ire

greg0ire Jun 11, 2015

Contributor

No problem, thanks for your support!

Contributor

greg0ire commented Jun 11, 2015

No problem, thanks for your support!

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Jun 11, 2015

Member

👍 Fine for me.

Member

xabbuh commented Jun 11, 2015

👍 Fine for me.

@wouterj

This comment has been minimized.

Show comment
Hide comment
@wouterj

wouterj Jun 27, 2015

Member

I love this PR. It was created, reviewed and accepted within one day. It's a shame it took us 16 days to merge... Thanks Grégoire!

Member

wouterj commented Jun 27, 2015

I love this PR. It was created, reviewed and accepted within one day. It's a shame it took us 16 days to merge... Thanks Grégoire!

@wouterj wouterj merged commit 9dcac5a into symfony:2.3 Jun 27, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

wouterj added a commit that referenced this pull request Jun 27, 2015

minor #5381 remove Yoda condition (greg0ire)
This PR was merged into the 2.3 branch.

Discussion
----------

remove Yoda condition

Yoda conditions : a questionable best practice that makes code less error-prone, but also harder to read for people that don't share the same grammar rules as people from Dagobah. Anyway, they are useful when you want to avoid confusing assignment and equality operators, but I doubt people often write `=` while meaning to write `<=`.

Commits
-------

9dcac5a remove Yoda condition

@greg0ire greg0ire deleted the greg0ire:patch-8 branch Jun 27, 2015

@greg0ire

This comment has been minimized.

Show comment
Hide comment
@greg0ire

greg0ire Jun 27, 2015

Contributor

Haha thanks for merging, there was no hurry though :)

Contributor

greg0ire commented Jun 27, 2015

Haha thanks for merging, there was no hurry though :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment