Skip to content

Conversation

@khanrn
Copy link
Contributor

@khanrn khanrn commented Apr 26, 2018

get and has method's redundant else condition removed.

@browner12
Copy link
Contributor

browner12 commented Apr 26, 2018

this is not redundant.

this changes execution since the if condition is not returning anything.

Copy link
Member

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

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

I believe the code is more readable with your proposal. I have requested some changed tho. 👍

$array = $array[$segment];
} else {
return value($default);
if (!static::accessible($array) && !static::exists($array, $segment)) {
Copy link
Member

Choose a reason for hiding this comment

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

I believe you made a mistake here. The code you are looking for is:

if (! static::accessible($array) ||  ! static::exists($array, $segment)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh... I missed to turn the and to an or!

$subKeyArray = $subKeyArray[$segment];
} else {
return false;
if (!static::accessible($subKeyArray) && !static::exists($subKeyArray, $segment)) {
Copy link
Member

Choose a reason for hiding this comment

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

I believe you made a mistake here. The code you are looking for is:

if (! static::accessible($subKeyArray) || ! static::exists($subKeyArray, $segment)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh... I missed to turn the and to an or!

@tillkruss
Copy link
Contributor

Tests are failing.

@Miguel-Serejo
Copy link

Miguel-Serejo commented Apr 26, 2018

I am continuously unable to understand how this type of change contributes anything to anyone.

The PR description is misleading. This isn't not removing a redundant else, it is attempting to inverse the logic of a condition in order to make the else redundant, and then removing it.

Keyword being "attempting" because this simply negated both sides of an and gate instead of negating the actual condition.
Not A and Not B is not the opposite of A and B. The correct way to negate an and gate is to turn it into a nor gate, i.e. Not A or Not B. This also changes the way the expression is evaluated.
For A and B, B is only evaluated if A is true. For Not A or Not B, B is only evaluated if A is false.
In this particular case there shouldn't be a difference in behavior, and at most negligible performance differences, but it's still a change in behavior that should be acknowledged.

In short, this doesn't fix anything, the PR description is misleading, and I fail to see how
"if array is accessible and the segment exists in the array, set array to segment, else return the default value"
is any less readable than
"if array is not accessible or the segment does not exist in the array, return the default value, then set array to segment if we didn't return"

@nunomaduro
Copy link
Member

@tillkruss Yeah. There is a mistake on the new if condition 😕

@tillkruss
Copy link
Contributor

tillkruss commented Apr 26, 2018

Please resubmit with CI passing.

@tillkruss tillkruss closed this Apr 26, 2018
@khanrn
Copy link
Contributor Author

khanrn commented Apr 27, 2018

Uh... A lot has happened here while I was sleeping! 😮 Anyway, I'm fixing the issue and will resubmit the PR. Thanks to all of you. 😄

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.

5 participants