Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add nextIntRange to Random #6524

Open
DartBot opened this issue Nov 5, 2012 · 8 comments
Open

Add nextIntRange to Random #6524

DartBot opened this issue Nov 5, 2012 · 8 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. core-2 P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@DartBot
Copy link

DartBot commented Nov 5, 2012

This issue was originally filed by @financecoding


Adding function to Random to help with getting a random number between a range (min, max). Depending on when Random gets refactored this could be a solution for now.

https://codereview.chromium.org/11367087

@iposva-google
Copy link
Contributor

This was briefly discussed when I created the current Random class but since it is literally "return min + rnd.nextInt(max - min);" it was deemed unnecessary. Over to the libraries team to reevaluate.


Removed Type-Defect label.
Added Type-Enhancement, Area-Library, Triaged labels.
Changed the title to: "Add nextIntRange to Random".

@sethladd
Copy link
Contributor

sethladd commented Nov 5, 2012

For context, the author of this patch is one of our earliest adopters and supporters. What may be trivial or unnecessary to us is very helpful to others.

@DartBot
Copy link
Author

DartBot commented Nov 6, 2012

This comment was originally written by @bp74


As a C# developer i tried to use this method too and i was surprised that it's not there. Sure it's easy to work around but the code looks better with the a method like this.

@DartBot
Copy link
Author

DartBot commented Nov 10, 2013

This comment was originally written by jacobly@google.com


Related feature request: add a 'choice(Iterable it)' method that would be a terser way to express it.elementAt(randInt(it.length)).

Both of these methods could be implemented on Random itself as concrete methods that delegate to randInt(), instead of adding new abstract methods that would have to be implemented by everyone.

@DartBot DartBot added Type-Enhancement area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. labels Nov 10, 2013
@kevmoo kevmoo added P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug and removed triaged labels Feb 29, 2016
@lrhn lrhn added the core-m label Aug 11, 2017
@floitschG floitschG added core-2 and removed core-m labels Aug 29, 2017
@natebosch
Copy link
Member

I think all of Random.nextIntRange(min, max), Random.choice(Iterable), and Random.asyncChoice(Stream) could be useful. The latter 2 especially so since the reservoir sample isn't the obvious implementation but it's what we'd want to avoid iterating twice.

@lrhn - would you accept a patch adding these? Or are they breaking because of implements Random and we'd have to wait until Dart 3?

@lrhn
Copy link
Member

lrhn commented Sep 6, 2018

It's a breaking change because it's an interface that is already being implemented.

I don't think even Dart 3 will allow us to break interfaces without very good reason. Backwards compatibility is going to be more important in the future than it has ever been before.

All the operations can be based on Random.nextInt, so they seem like something that can be written as static helper functions (and extension methods or default methods if we get those). I see no urgency in adding them to the Random interface.

I recommend just putting them in a package, say package:math (I think we reserved that name, not sure if we have used it yet).

Random _defaultRandom;
int randomIntInRange(int start, int end, [Random random]) {
  if (start <= end) {
    return (random ?? _defaultRandom = Random()).nextInt(end - start) + start;
  }
}
T randomElement<T>(Iterable<T> elements, [Random random]) {
   random ??= _defaultRandom = Random();
   if (elements is List<Object>) {
     return elements.elementAt(random.nextInt(elements.length));
   }
   T result = null;
   int count = 0;
   for (var element in elements) {
     if (random.nextInt(++count) == 0) result = element;
   }
   return element;
}

Future<T> randomAsyncElement<T>(Stream<T> elements, [Random random]) async {
   random ??= _defaultRandom = Random();
   T result = null;
   int count = 0;
   await for (var element in elements) {
     if (random.nextInt(++count) == 0) result = element;
   }
   return element;
}

(Or, if you don't want to return null on empty lists, do something else if count is zero at the end).

@matanlurey
Copy link
Contributor

@lrhn Your preference to add static or top-level methods here (but not Future.catchError - see your comment here: #34144 (comment)) is really confusing to me. This seems like one of the easier changes - Random is not used nearly as much as Future.

Internally I can can count 31 implements Random {, of which they virtually all either:

import 'package:mockito/mockito.dart';

// OK: Since Mock implements noSuchMethod we can add new methods to Random.
class MockRandom extends Mock implements Random {}

... or:

class FakeRandom implements Random {
  double value;

  FakeRandom(this.value);

  double nextDouble() => value;
  int nextInt(int max) => (value * max).toInt();
  bool nextBool() => value != 0;
}

If we had default methods, we could do this in a non-breaking way. Personally, I still think we should consider some way of doing class-level evolution for Dart 3.0 changes, otherwise we might as well just start dart:core2 (or Guava for Dart).

@natebosch
Copy link
Member

natebosch commented Sep 6, 2018

These seem like ideal candidates for extension methods which could go in package:math... hopefully one day we can do that.

In the mean time top level methods seems like a good compromise.

I've started an internal thread about getting that package going - assuming all goes well I should be able to close this issue by publishing them there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. core-2 P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

8 participants