1
\$\begingroup\$

im using the following code which is working. The code provide range of random values but unique one i.e. it doesn't return the same values until the full range was provided ,what do you think and doest in ES6 there is shorter way to achieve this?

var range = [10, 20];
var not = [];

function randomRange(range, n) {
  if (not.length >= range[1] - range[0]) {
    not.length = 0;
  }
  var curr = [];
  var res = [];
  for (let i = range[0]; i < range[1]; i++) {
    if (!not.some(function(num) {
      return i == num
    }) && not.length < range[1] - range[0]) {
      curr.push(i)
    }
  }
  for (let i = 0; i < n; i++) {
    var j = curr.splice(Math.floor(Math.random() * curr.length), 1)[0];
    res[i] = not[not.length] = j;
  }

  return res.filter(Boolean)
}

function log(result) {
  document.body.innerHTML += JSON.stringify(result) + "<br>"
}

window.onload = function() {

log(randomRange(range, 3));
log(randomRange(range, 3));
log(randomRange(range, 3));
log(randomRange(range, 3));
log(randomRange(range, 3));
log(randomRange(range, 3));
log(randomRange(range, 3));
log(randomRange(range, 3));
log(randomRange(range, 3));
log(randomRange(range, 3));
}
\$\endgroup\$
0
\$\begingroup\$

Confusing API

The current API is a bit confusing. range is a parameter of randomRange, but in the usage example you gave you always call the function with the same range value. It's unclear if you ever intend to call it with a different value. And if you do, it's unclear what realistic purpose it could serve. The behavior would seem rather chaotic.

Also note that the current API and the global not variable prevents you from using multiple different random ranges at the same time.

Instead of making range a parameter of randomRange, it would be better to use it once for initialization, and store it in an internal data structure not accessible from outside.

An array seems a strange choice for storing two values. Instead of range as an array, why not use simply start and end variables?

Fragile implementation

The implementation is very fragile. not is quite crucial for the inner workings of the randomRange function, but it's easily accessible in the global namespace, making it vulnerable to accidental or malicious modifications.

Performance

The current implementation is inefficient. On each call, it scans the range and populates curr with values that are not in not, and then selects random values from curr. It would be better to shuffle the range of values, splice from the shuffled range, and reshuffle when it's emptied.

Alternative implementation

Consider this alternative implementation that solves all the above issues:

var shuffle = function(arr) {
  for (let i = arr.length - 1; i > 0; i--) {
    let target = nextInt(i + 1);
    swap(arr, i, target);
  }
};

var swap = function(arr, i, j) {
  var tmp = arr[i];
  arr[i] = arr[j];
  arr[j] = tmp;
};

var nextInt = function(bound) {
  if (bound === undefined) {
    bound = Number.MAX_SAFE_INTEGER;
  }
  return Math.floor(bound * Math.random());
};

var recyclingRandomRange = function(start, end) {
  var range = [];
  var resetRange = function() {
    for (let i = start; i < end; i++) {
      range.push(i);
    }
    shuffle(range);
  };

  return function(n) {
    if (range.length === 0) {
      resetRange();
    }
    return range.splice(0, n);
  };
};

This usage would be equivalent to your example:

var ranger = recyclingRandomRange(10, 20);
log(ranger(3));
log(ranger(3));
log(ranger(3));
log(ranger(3));
log(ranger(3));
log(ranger(3));
log(ranger(3));
\$\endgroup\$
  • \$\begingroup\$ Thank you very much, the only problem I see it that I need to get an array and this return diff structure, how it should be adapt? \$\endgroup\$ – John Jerrby Dec 7 '16 at 7:07
  • \$\begingroup\$ I don't understand what you mean. The ranger(n) function returns an array. \$\endgroup\$ – Stop ongoing harm to Monica Dec 7 '16 at 7:25
  • \$\begingroup\$ sorry :-) my mistake , 2 point 1. does with lodash/es6 the code can be more readable/clean (we use it in our framework, my fault that I didn't mention it ...2. I use the function like the following recyclingRandomRange(10,20)(3) to provide data , I didnt use function like this before is it ok? \$\endgroup\$ – John Jerrby Dec 7 '16 at 7:37
  • \$\begingroup\$ I don't know if lodash has some utilities to shuffle or get random integers. If yes then you could use those instead of these home-made shuffle and nextInt. On the other hand, these are simple enough, and give you independence and flexibility. As for recyclingRandomRange(10,20)(3), that's legal, but I don't think you want to do that. Repeated calls to recyclingRandomRange(10,20)(n) don't share the tracking of already used numbers. If you want that tracking, you need to use like in my example at the end. \$\endgroup\$ – Stop ongoing harm to Monica Dec 7 '16 at 10:57
0
\$\begingroup\$

If what you want is to call the same function with the same array and obtain random numbers from that array every time, but without them repeating, well, that's almost literally the definition of "shuffle".

Have a look at this answer on stackoverflow. The idea in your case is that you shuffle the array and pick the first (or last) n elements.

That means something like (almost entirely copied from linked answer):

function shuffle(array) {
  var currentIndex = array.length, temporaryValue, randomIndex;

  while (0 !== currentIndex) {
    randomIndex = Math.floor(Math.random() * currentIndex);
    currentIndex -= 1;
    temporaryValue = array[currentIndex];
    array[currentIndex] = array[randomIndex];
    array[randomIndex] = temporaryValue;
  }

  return array;
}

var arr = [];
for (var i = 10; i <= 20; i++) {
   arr.push(i);
}
shuffle(arr);
console.log(arr.slice(0,3));
console.log(arr.slice(3,6));

The advantage is that you need to randomize only once, no need to loop through the array and generate random numbers every time.

The disadvantage is that if you want different numbers, you'll have to keep track of the index by yourself, because requesting arr.slice(0,3) will always return the same numbers. But considering that you'd have to keep track of which numbers have already been used anyway, this should not be a problem.

Also, this may actually not be a disadvantage depending on what you use the function for. (Of course you could shuffle every time, but that would lose any speed benefit of shuffling).

Another approach would be to shuffle once and then having a function that returns the first n numbers but also removes them from the list. The obvious disadvantage in that case would be that you have to keep a copy of your original array, and when the shuffled array is empty, copy it and shuffle it again.

If you still want to use your approach (which works anyway), at least make sure to use meaningful variable names. In my opinion not is probably the worst possible name for a variable.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.