Removed Else statement from getUserStateFromRequest #7975

Merged
merged 2 commits into from Jan 10, 2016

Conversation

Projects
None yet
5 participants
@Mathewlenning
Contributor

Mathewlenning commented Sep 29, 2015

Very simple change

@n9iels

This comment has been minimized.

Show comment
Hide comment
@n9iels

n9iels Oct 8, 2015

Contributor

I have tested this item successfully on 81750b1

Looks good to me


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

Contributor

n9iels commented Oct 8, 2015

I have tested this item successfully on 81750b1

Looks good to me


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

@sovainfo

This comment has been minimized.

Show comment
Hide comment
@sovainfo

sovainfo Oct 8, 2015

Contributor

Disagree with this change. Don't mind removing use of variable, do mind setting user state regardless whether it was changed in the request. Would prefer it not be set unneeded or change the comment to reflect what is done in code!

Contributor

sovainfo commented Oct 8, 2015

Disagree with this change. Don't mind removing use of variable, do mind setting user state regardless whether it was changed in the request. Would prefer it not be set unneeded or change the comment to reflect what is done in code!

@Mathewlenning

This comment has been minimized.

Show comment
Hide comment
@Mathewlenning

Mathewlenning Oct 8, 2015

Contributor

Actually NoNumber presented a better refactor that keep the signature and side effects the same as before. I'll update it when I get back to my desk.

Sincerely,
Mathew Lenning

P.S. This message was sent via iPhone, so please forgive any errors

On Oct 8, 2015, at 7:17 PM, sovainfo notifications@github.com wrote:

Disagree with this change. Don't mind removing use of variable, do mind setting user state regardless whether it was changed in the request. Would prefer it not be set unneeded or change the comment to reflect what is done in code!


Reply to this email directly or view it on GitHub.

Contributor

Mathewlenning commented Oct 8, 2015

Actually NoNumber presented a better refactor that keep the signature and side effects the same as before. I'll update it when I get back to my desk.

Sincerely,
Mathew Lenning

P.S. This message was sent via iPhone, so please forgive any errors

On Oct 8, 2015, at 7:17 PM, sovainfo notifications@github.com wrote:

Disagree with this change. Don't mind removing use of variable, do mind setting user state regardless whether it was changed in the request. Would prefer it not be set unneeded or change the comment to reflect what is done in code!


Reply to this email directly or view it on GitHub.

Implemented the refactor that NoNumbers suggested. Now the signature …
…and the side effect are exactly as they were before refactoring.
@joomla-cms-bot

This comment has been minimized.

Show comment
Hide comment
@joomla-cms-bot

joomla-cms-bot Oct 8, 2015

This PR has received new commits.

CC: @n9iels


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

This PR has received new commits.

CC: @n9iels


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

@Mathewlenning

This comment has been minimized.

Show comment
Hide comment
@Mathewlenning

Mathewlenning Oct 8, 2015

Contributor

@n9iels Thanks for testing. I appreciate it =^D

Contributor

Mathewlenning commented Oct 8, 2015

@n9iels Thanks for testing. I appreciate it =^D

@sovainfo

This comment has been minimized.

Show comment
Hide comment
@sovainfo

sovainfo Oct 8, 2015

Contributor

Not using the variable $cur_state would be an quality improvement and more efficient.

Contributor

sovainfo commented Oct 8, 2015

Not using the variable $cur_state would be an quality improvement and more efficient.

wilsonge added a commit that referenced this pull request Jan 10, 2016

Merge pull request #7975 from Mathewlenning/CLEANING-JApplicationCms
Removed Else statement from getUserStateFromRequest

@wilsonge wilsonge merged commit e9f5928 into joomla:staging Jan 10, 2016

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Jan 10, 2016

Contributor

Merged on review - thanks Mathew!

Contributor

wilsonge commented Jan 10, 2016

Merged on review - thanks Mathew!

@wilsonge wilsonge added this to the Joomla! 3.5.0 milestone Jan 10, 2016

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