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

Refactor Shuffle in Get-Random to save a full iteration of the objects. #8969

Merged
merged 7 commits into from
Mar 1, 2019
Merged

Refactor Shuffle in Get-Random to save a full iteration of the objects. #8969

merged 7 commits into from
Mar 1, 2019

Conversation

st0le
Copy link
Contributor

@st0le st0le commented Feb 24, 2019

PR Summary

Minor refactoring of the Fisher Yates shuffle to save an additional iteration.

PR Context

Micro-optimization to the shuffle function.

PR Checklist

@st0le st0le changed the title Refactor Shuffle in Get-Random to save an iteration. Refactor Shuffle in Get-Random to save a full iteration of the objects. Feb 24, 2019
@st0le st0le closed this Feb 24, 2019
@st0le st0le reopened this Feb 24, 2019
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Feb 26, 2019
object tmp = _chosenListItems[i];
_chosenListItems[i] = _chosenListItems[j];
_chosenListItems[j] = tmp;
_chosenListItems[j] = _chosenListItems[i];
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this cause the value in i to be duplicated? Shouldn't we swap like we did before?

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand now, the assignment effectively removes the output value from the list for the next iteration. The comment should be updated to make this more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do it soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

Please update code comment to make assignment rather than swap change more clear.

object tmp = _chosenListItems[i];
_chosenListItems[i] = _chosenListItems[j];
_chosenListItems[j] = tmp;
_chosenListItems[j] = _chosenListItems[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand now, the assignment effectively removes the output value from the list for the next iteration. The comment should be updated to make this more clear.

@iSazonov iSazonov self-assigned this Feb 27, 2019
@st0le
Copy link
Contributor Author

st0le commented Feb 28, 2019

How do I trigger the CI Build without a dummy commit? The build broke due to an unrelated change

@TravisEz13
Copy link
Member

Just ask and a maintainer can do it for you. Otherwise it requires a dummy commit. In this scenario, asking was better.

@TravisEz13
Copy link
Member

But looking at the history, it looks like GitHub is not remerging your PR. You might need to rebase your PR to reliably get a clean Windows CI.

@TravisEz13
Copy link
Member

I can do that for you if you don't know how, but the failures I saw are intermittent.

Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

@st0le Please fix the style issues and we'll merge.

@iSazonov iSazonov merged commit f31b338 into PowerShell:master Mar 1, 2019
@iSazonov
Copy link
Collaborator

iSazonov commented Mar 1, 2019

@st0le Thanks for your contribution!

Perhaps you might be interested in #8986.

@st0le st0le deleted the users/gakama/RefactorGetRandom branch March 19, 2019 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants