Skip to content

Conversation

@rottencowz
Copy link
Collaborator

Extract functionality for sorting an array into a helper method.

Make functions that are named randomize to return the expected randomized result. (Caller should not need to randomize the resulting array. Otherwise functions should be renamed like selectFactions or something.

Previous implementation used .sort(() => Math.random() - 0.5) which is a biased sorting and will favor the first element being first a higher amount of time. This is because the sort implementation remembers previous results and will order based on them.

I decided not to leverage modifying Array.prototype. Ultimately we can easily add an extension method to a customized array subtype but I don't want to do that until we have better typing tooling which would support type coercion.

Copy link
Owner

@haswellr haswellr left a comment

Choose a reason for hiding this comment

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

Not sure if this is better or not. The "randomize" functions previously chose a random list, but did not guarantee a random order. With your change, you're now assuming that each chooses a random order for you, but there's no way to really guarantee that with the function signature. You could rename to "randomizeFactionsAndRandomizeOrder" or something, but I think it's kind of a red flag - you're doing two things in one function. Random selection and random ordering are different.

Maybe renaming previous functions to "selectRandomFactions" etc would be the most clear?

The helper function is great though.

@haswellr
Copy link
Owner

Oh and good catch on the sort! I didn't even notice that at first read.

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.

3 participants