array_get_random/array_pop_random

[Alphish]

An offshoot from the issue #13, which related to picking items from an array with both uniform and weighed distribution. The aforementioned issue will stay up for the purposes of the weighed array randomisation, as a more advanced scenario. I decided to separate these, because some people might want to just stick with uniform distribution and would find weighed randomisation superfluous.

This suggestion focuses on the simplest array-picking scenarios with uniform distribution. I propose the following two functions:

• array_get_random(array, [offset], [length]) - gets a random item from an array (or a range within it)
• array_pop_random(array, [offset], [length]) - removes a random item from the array (or a range within it) and returns it

From the random array items selection, I've found myself using functions like the two above the most.

I'm open to other name suggestions. ^^

[biyectivo]

So I just implemented this on my fork. I know it has to be accepted before I actually submit the PR.

One thing, I read the discussion on the array max/min/... regarding zero-element arrays. However, I don't think it's a good idea to return 0 for array_get_random (definitely NOT a good idea for array_pop_random), I think both functions should throw an error when given empty arrays. Do you agree?

[Alphish]

@biyectivo I'd opt for returning undefined instead, since array_pop, array_first and array_last all return undefined as well.
array_get does actually crash, but struct_get returns undefined once more, so it seems like "array_get" is the odd one here (that, and also you are dealing with a concrete index here, unlike array_get/pop_random).

[Alphish]

Just to confirm everyone agrees on the names:

• react with 🎉 if you are fine with array_get_random(...) and array_pop_random(...)
• react with 😕 if you want some other names

[biyectivo]

@Alphish agreed with the undefined return for empty arrays. My implementation does throw an exception for invalid (negative offset or length arguments), is this ok?

[Alphish]

@biyectivo Actually, that's how offset and length are mapped onto the starting index and range:

• positive offset N maps onto the starting index of N
• negative offset -N maps onto the Nth last array starting index (so offset -1 returns last element, -2 second last etc.); if N exceeds array length, starting index of 0 is used
• positive length L takes L elements forwards from the starting index (i.e. [N, N+1, N+2, ..., N+(L-1)])
• negative length L takes L elements backwards from the starting index (i.e. [N, N-1, N-2, ..., N-(L-1)])

The negative L index results in a reversed order, but in practice the order doesn't matter for array_get_random and array_pop_random, so we may transform the indices like so:

if (_offset < 0)
    _offset = max(0, array_length(_array) + _offset);

if (_length < 0) {
    var _new_offset = max(0, _offset + _length + 1);
    _length = _offset - _new_offset + 1;
    _offset = _new_offset;
}

// process the rest as usual

If I'm not mistaken, this should properly handle our negative offset/length needs.

[Alphish]

Looks like no one has objections against array_get_random and array_pop_random names, so we can go ahead with the implementation!

The exact signatures would be:

• array_get_random(array,[offset],[length])
• array_pop_random(array,[offset],[length])

If the array is empty, the function returns undefined.

The negative offsets and lengths should be handled as described above. When in doubt on what values range would be produced by the given offset and length, you can use array_filter(_array, function() { return true; }, _offset, _length) for reference. Actually allocating a new array with array_filter(...) in functions themselves would be an overkill, though, so it should be avoided in the final implementation.

The tests should check that:

• the result of array_get_random(...) is from the source array
• the result of array_pop_random(...) was originally in the source array, but isn't anymore (the array items must be unique for this to apply)
• the results of array_get_random(...) and array_pop_random(...) given a single-item array return that item (and also array_pop_random(...) leaves the array empty afterwards)
• the results of array_get_random(...) and array_pop_random(...) are undefined for empty array
• the array_get_random(...) and array_pop_random(...) functions respect the offset and length, positive and negative alike; to make tests consistent, the length can be set to 1/-1

The randomness might make the tests non-deterministic with the randomness involved, but it's acceptable, as long as the assertions don't exclude any valid possibility (e.g. asserting that random item would be 3 given [1, 2, 3, 4, 5] array would exclude valid possibilities, but asserting that it can be either of those five is not). Worst case scenario, the implementation will be actually incorrect, but the tests won't stumble upon the invalid cases and mark everything as correct. I don't want to use random_set_seed(...) to ensure consistent results, because there might be other randomness-related tests that completely break the execution and also it may be inconsistent between platforms (in short: it doesn't ensure consistent results).

As for the demo, I suggest expanding upon the array utilities demo (min/max/mean/mediam/sum), maybe adding some "Get random" and "Pop random" buttons, which will set some "Last random value" variable.

Usually I'd say to let others know about your implementation efforts, but it seems like @biyectivo is already onto it, so I recommend contacting them if you want to help. ^^

[biyectivo]

Hi, just checking in to see if there's something wrong or missing on my part with the PR or you haven't had time to review it :) No rush though, just making sure I didn't screw something up.

[Alphish]

@biyectivo Ah, right, sorry. I've been kinda sick the previous week, and then had lots of other stuff involved (including fixing the existing array functions and figuring out the offset/length handling).

I'd put this PR on hold for now. Verrific is a package I develop independently, so I'd like to make a version with additional "is value in array" check you needed here, and then update the Community Toolbox repo with the new Verrific version, to avoid inconsistencies between main library and local version. I should actually made a minor release of Verrific with new functions today or tomorrow.

Then, once the updated Verrific library is merged, you could tweak your tests to match the library. Also, there's PR #36 where I implemented offset/length to match the handling of array_filter, array_map and the like. I suggest to copy the offset/length boilerplate from there, but that's probably once both changes (array function fixes and Verrific update) are merged first, so you can tweak the functions and tests in one go. ^^

[biyectivo]

I created the new pull request considering what was asked. Kindly review.

[Alphish]

I made another pull request (#41) based on the previous one, since I figured it would be easier to make a PR from my branch directly rather than making a PR to the fork branch, and only then accepting the PR from the fork branch. ^^'
(the PR also describes the changes made)

[Alphish]

The new functions were merged! Closing the issue now...