Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(88)

Issue 11367087: Added nextIntRange to dartvm and dart2js. (Closed)

Created:
8 years, 1 month ago by Adam
Modified:
7 years, 1 month ago
Reviewers:
floitsch
CC:
reviews_dartlang.org, Lasse Reichstein Nielsen
Visibility:
Public.

Description

Added nextIntRange to dartvm and dart2js. Adding implementation of nextIntRange to get the next int from a range. BUG=

Patch Set 1 #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+37 lines, -0 lines) Patch
M runtime/lib/math_patch.dart View 1 chunk +9 lines, -0 lines 3 comments Download
M sdk/lib/_internal/compiler/implementation/lib/math_patch.dart View 1 chunk +9 lines, -0 lines 2 comments Download
M sdk/lib/math/random.dart View 1 chunk +10 lines, -0 lines 0 comments Download
M tests/compiler/dart2js_extra/math_lib_prefix_test.dart View 1 chunk +1 line, -0 lines 0 comments Download
M tests/compiler/dart2js_extra/math_lib_test.dart View 1 chunk +1 line, -0 lines 0 comments Download
M tests/lib/math/math2_test.dart View 2 chunks +7 lines, -0 lines 1 comment Download

Messages

Total messages: 1 (0 generated)
floitsch
7 years, 1 month ago (2013-11-18 13:36:03 UTC) #1
Message was sent while issue was closed.
Sorry this took so long.
If you are still up for it, LGTM after changing from nextIntRange to
nextIntInRange, and after the comments have been addressed.

https://codereview.chromium.org/11367087/diff/1/runtime/lib/math_patch.dart
File runtime/lib/math_patch.dart (right):

https://codereview.chromium.org/11367087/diff/1/runtime/lib/math_patch.dart#n...
runtime/lib/math_patch.dart:73: if (range <= 0) {
Doesn't test if max or min is negative.

https://codereview.chromium.org/11367087/diff/1/runtime/lib/math_patch.dart#n...
runtime/lib/math_patch.dart:74: throw new ArgumentError("min is greater then or
equal to max");
Would be nice to have the values in the error message.

https://codereview.chromium.org/11367087/diff/1/runtime/lib/math_patch.dart#n...
runtime/lib/math_patch.dart:76: range = nextInt(range);
I wouldn't have called the result of the random-call "range".

Either use a different variable, or just "return min + nextInt(range);".

https://codereview.chromium.org/11367087/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/math_patch.dart (right):

https://codereview.chromium.org/11367087/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/math_patch.dart:59: if (min >=
max) throw new ArgumentError("min is greater then or equal to max");
80 chars. And the error message should contain the values.

https://codereview.chromium.org/11367087/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/math_patch.dart:62: range =
nextInt(range);
Don't reuse the variable (which is not a range).
I would just write it in one line:
return min + nextInt(max - min);

https://codereview.chromium.org/11367087/diff/1/tests/lib/math/math2_test.dart
File tests/lib/math/math2_test.dart (right):

https://codereview.chromium.org/11367087/diff/1/tests/lib/math/math2_test.dar...
tests/lib/math/math2_test.dart:246: checkClose(5, r.nextIntRange(1, 10), 10);
checkClose is typed for doubles. This will throw in checked mode.

Check by hand that it is in the range.
Also: Add 3 more tests:
- one that has min negative: Expect.throws(...)
- one that has min == max.
- one that has min == max - 1. (just one value). and make sure the returned
value equals min.

Powered by Google App Engine
This is Rietveld 408576698