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

Remove unnecessary condition #16412

Closed
wants to merge 6 commits into from
Closed

Remove unnecessary condition #16412

wants to merge 6 commits into from

Conversation

nvyush
Copy link
Contributor

@nvyush nvyush commented Jun 1, 2017

The check !empty($this->item) is unneeded at this point.
It is possible to join the statements of if conditions at the lines 72 and 77.

Pull Request for Issue #16409.

Summary of Changes

Unnecessary condition is removed.
The statements of equivalent conditions is joined.

Testing Instructions

Code review.

Expected result

Actual result

Documentation Changes Required

Not needed.

The check ```!empty($this->item)``` is unneeded at this point. It is possible to join the statements of if conditions at the lines 72 and 77.
@Bakual
Copy link
Contributor

Bakual commented Jun 1, 2017

Not sure if that is accurate.
Because the first check !empty($this->item->id)is only true when the item already has an id set and other than 0/false (means we edit an existing item).
The second check !empty($this->item) && isset($this->item->id) will be true if the item is valid, but the ID may also be 0 (means it can also be a new item).

@nvyush
Copy link
Contributor Author

nvyush commented Jun 1, 2017

Well, but check !empty($this->item) is not needed and can be removed. If $this->item is empty at this point, all checks $this->item->id above this line will be wrong. Are you agree?
I can update the commit.

@nvyush
Copy link
Contributor Author

nvyush commented Jun 1, 2017

I do not understand, why commit 5ed1d17 is valid, but 6b585ae is not. I simple removed white spaces.

@Bakual
Copy link
Contributor

Bakual commented Jun 1, 2017

Imho, the isset should indeed be sufficient, because it will also return false if the item doesn't exist. So we gain nothing by explictiely checking the existence of the item prior to it.

I tried to restart the drone test. looks like an unrelated error to me.

@nvyush
Copy link
Contributor Author

nvyush commented Jul 7, 2017

@Bakual, why continuous-integration/jenkins/pr-merge reported an error? What I must do?

@rdeutz
Copy link
Contributor

rdeutz commented Jul 7, 2017

@nvyush you can ignore the jenkins failure for now, we are testing a new setup. It is important that the Required checks pass and the drone test (code styles) should also pass.

@nvyush
Copy link
Contributor Author

nvyush commented Jul 7, 2017

@rdeutz, thanks

@Quy
Copy link
Contributor

Quy commented Nov 16, 2017

I have tested this item ✅ successfully on 60788fd


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16412.

@fancyFranci
Copy link
Contributor

For new items the ID is always NULL. Even by saying a_id=0 in the url. So why not joining the two if-conditions?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16412.

@Quy
Copy link
Contributor

Quy commented Dec 21, 2017

@FPerisa You're right. They can be combined and remove the second if statement.

@joomla-cms-bot
Copy link

Set to "closed" on behalf of @Quy by The JTracker Application at issues.joomla.org/joomla-cms/16412

@Quy
Copy link
Contributor

Quy commented Apr 29, 2018

See PR #20254


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16412.

@nvyush nvyush deleted the patch-1 branch February 11, 2019 09:08
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.

None yet

6 participants