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

ISSUE-150: Validate RANDOM range input has the smaller number first #151

Merged
merged 2 commits into from
Jan 25, 2023

Conversation

dmalec
Copy link
Collaborator

@dmalec dmalec commented Jan 22, 2023

Resolves #150

Summary

The RANDOM procedure does not check if the first parameter is less than the second parameter on the two input form. This change adds an explicit check and errors if the range is out of order based on the description of the procedure in the manual. It also permits RANDOM to accept a range including negative numbers.

Testing

Without this change, passing in a higher number first leads to unexpected results:

? PRINT (RANDOM 5 3)
825322468
?

After making this change, passing in a higher number will produce an error:

? PRINT (RANDOM 5 6)
6

? PRINT (RANDOM 6 5)
RANDOM doesn't like [6 5] as input

Which is equivalent to passing a negative number to the single input form:

? PRINT RANDOM 10
9

? PRINT RANDOM -10
RANDOM doesn't like -10 as input

This change also allows negative numbers in the range:

? PRINT (RANDOM -10 -5)
-7

? PRINT (RANDOM -10 10)
5

While also enforcing the ordering constraint on the range even when negative:

? PRINT (RANDOM -5 -10)
RANDOM doesn't like [-5 -10] as input

Test Environments

  • OSX Catalina (10.15.7) w/ wxWidgets 3.0.5
  • Ubuntu Bionic (18.04.5) w/ wxWidgets 3.0.5

  • Windows 10 Pro (19045.2364) w/ wxWidgets 3.0.5

@jrincayc
Copy link
Owner

@dmalec Would you like this merged, or did you want to work on it more to fix the negative numbers problem as well?

@dmalec
Copy link
Collaborator Author

dmalec commented Jan 24, 2023

@dmalec Would you like this merged, or did you want to work on it more to fix the negative numbers problem as well?

@jrincayc - I'd like to work on it a bit more to fix the negative number problem; I'll mark it as in progress.

@dmalec dmalec added the unfinished This merge still needs work before merging. label Jan 24, 2023
@dmalec dmalec changed the title ISSUE-150: Validate RANDOM range input has the smaller number first Draft: ISSUE-150: Validate RANDOM range input has the smaller number first Jan 24, 2023
@dmalec dmalec changed the title Draft: ISSUE-150: Validate RANDOM range input has the smaller number first ISSUE-150: Validate RANDOM range input has the smaller number first Jan 24, 2023
@dmalec dmalec marked this pull request as draft January 24, 2023 02:01
* Allow negative ranges as parameters to RANDOM
* Manually check the single parameter form to prevent a negative number
* Refactored code around random number generation to clarify how the
  random number is produced.
@dmalec dmalec removed the unfinished This merge still needs work before merging. label Jan 25, 2023
@dmalec dmalec marked this pull request as ready for review January 25, 2023 02:03
@dmalec
Copy link
Collaborator Author

dmalec commented Jan 25, 2023

@jrincayc - this PR now contains a proposed change for allowing negative numbers in ranges and is ready for re-review.

Copy link
Owner

@jrincayc jrincayc left a comment

Choose a reason for hiding this comment

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

Reviewed and tested and I approve.

@jrincayc jrincayc merged commit f8ee837 into jrincayc:master Jan 25, 2023
@dmalec dmalec deleted the ISSUE-150-RANDOM-RANGE branch January 26, 2023 23:14
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.

Two input form of RANDOM procedure has odd behavior if first input is larger than second input.
2 participants