Skip to content

Conversation

passerbyloo
Copy link
Contributor

@passerbyloo passerbyloo commented Mar 20, 2019

according to https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle

checks if j ≠ i may be omitted, and also remove checking logic will optimise performance(tested by 100000 times random).

Checklist

Description

according to https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle

`checks if j ≠ i may be omitted`, and also remove checking logic will optimise performance(tested by 100000 times random).
Copy link
Contributor

@richard-ash richard-ash left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Left some comments inline. Also, do you mind updating the README to say why the check for if i != j is unnecessary?

@@ -96,9 +96,6 @@ public func shuffledArray(_ n: Int) -> [Int] {
var a = [Int](repeating: 0, count: n)
for i in 0..<n {
let j = Int.random(in: 0...i)
if i != j {
a[i] = a[j]
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this line breaks the algorithm. You're right the check if i != j is unnecessary however you still need the line a[i] = a[j]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@@ -30,9 +30,6 @@ public func shuffledArray(_ n: Int) -> [Int] {
var a = Array(repeating: 0, count: n)
for i in 0..<n {
let j = random(i + 1)
if i != j {
a[i] = a[j]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@richard-ash
Copy link
Contributor

Cool, thanks for the update 🙂

@richard-ash richard-ash merged commit fc7869c into kodecocodes:master Apr 24, 2019
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.

2 participants