LouisDotCom / NextItem

what to do!
0 stars 1 forks source link

Remove unneeded shuffle #1

Closed cristoper closed 7 years ago

cristoper commented 7 years ago

Congrats on your first app!

As a beginner you have no choice but to depend on example code you find, and the array shuffling function you found works fine for your application, but it might be instructive to see why it is not very good:

extension Array
{
    /** Randomizes the order of an array's elements. */
    mutating func shuffle()
    {
        for _ in 0..<10
        {
            sort { (_,_) in arc4random() < arc4random() }
         }
     }
}

The function shuffles the array by sorting it ten times with a predicate that compares 2 random numbers (so that each element is randomly moved to the first or last half of the array). But why 10 times? And why 2 random numbers? Any time random variables interact, it becomes that much harder to keep track of statistics (for me anyway): in this case, what is the probability that the first random number is smaller than the second? It turns out (I think) to be 0.5, so it would have been simpler to randomly choose one of two numbers:

// Randomly generate a 0 or a 1, and test if it is 0 (50% probability)
sort { _ in arc4random_uniform(2) == 0 }

But the real problem is that sorting an array with a random predicate like that does not result in a uniform distribution of permutations: some orderings of the array will occur more often than others. I think that is why the author of the code shuffles it 10 times... they probably found that was enough to approximate a uniform distribution.

But that is an inefficient way to end up at something that is not even really uniform. Much better is to use the Fisher-Yates algorithm, which is a simple way to quickly and uniformly shuffle an array:

extension Array
{
    mutating func fisher_yates()
    {
        for i in (0..<count).reversed()
        {
            let j32 = arc4random_uniform(UInt32(i+1))
            let j = Int(j32) // because arc4random_uniform returns a UInt32
            (self[i], self[j]) = (self[j], self[i]) // swap current item with random item at larger index
        }
    }
}

But in your case, since you want a random element of the array every time the button is touched, you don't need to shuffle the array at all. You just need to choose a random element to display. That's what the change attached to this pull request does.

LouisDotCom commented 7 years ago

cristoper, thanks for the tutorial. I appreciate how you show me a better way to shuffle and then drop shuffling altogether in order to move to (what I knew was) the requirement, which I didn't know how to implement, a random selection. I merged your code but had to update my local project manually. "git fetch NextItem" didn't throw an error but it didn't appear to bring down the new ViewController.swift file either. Anecdotally, I think the new version gives a better variety or distribution of items (but I didn't measure my clicks and results on either version).

cristoper commented 7 years ago

git fetch will download the commit history from the remote server, but it won't actually update your current branch. After doing a git fetch you could then merge any new changes with git merge origin/NextItem

But most of the time you want to use the git pull command (I think git pull origin NextItem should work, depending on how your local repository is setup) which automatically does a fetch followed by a merge.

Judging by the number of upvotes and replies this StackOverflow question has, I'd say it's a very common trap for new git users: https://stackoverflow.com/questions/292357/what-is-the-difference-between-git-pull-and-git-fetch

LouisDotCom commented 7 years ago

All recommendations for cristoper above are completed (and I just noticed his avatar, from way back...).

On Thu, Jul 20, 2017 at 10:30 PM, chris burkhardt notifications@github.com wrote:

git fetch will download the commit history from the remote server, but it won't actually update your current branch. After doing a git fetch you could then merge any new changes with git merge origin/NextItem

But most of the time you want to use the git pull command (I think git pull origin NextItem should work, depending on how your local repository is setup) which automatically does a fetch followed by a merge.

Judging by the number of upvotes and replies this StackOverflow question has, I'd say it's a very common trap for new git users: https://stackoverflow.com/questions/292357/what-is-the- difference-between-git-pull-and-git-fetch

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/LouisDotCom/NextItem/pull/1#issuecomment-316899759, or mute the thread https://github.com/notifications/unsubscribe-auth/AHFJsaabToK5Y4MTmcTrDsau-Qxvdprcks5sQCldgaJpZM4OeYSp .