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

Any.DecimalInRange fixed #8657

Merged
merged 1 commit into from Sep 17, 2020
Merged

Any.DecimalInRange fixed #8657

merged 1 commit into from Sep 17, 2020

Conversation

NKarolak
Copy link

@NKarolak NKarolak commented Sep 15, 2020

Sometimes Any.DecimalInRange returned a decimal number, whose No. of decimal places did not equal the parameter.
This now has been fixed in accordance with @nikolakukrika. It should be ported afterwards to library random.

The variable name pow has been chosen in accordance to procedure DecimalInRange(MinValue: Decimal; MaxValue: Decimal; DecimalPlaces: Integer): Decimal.

I am unsure, but it looks like procedure DecimalInRange(MinValue: Decimal; MaxValue: Decimal; DecimalPlaces: Integer): Decimal might need a fix as well.

Please refer to https://www.yammer.com/dynamicsnavdev/threads/857565538000896 for further information.

@JesperSchulz JesperSchulz added the processing-PR The PR is currently being reviewed label Sep 15, 2020
@@ -44,8 +44,17 @@ codeunit 130500 "Any"
/// <param name="DecimalPlaces">Number of decimal places</param>
/// <returns>Pseudo-random decimal value</returns>
procedure DecimalInRange(MaxValue: Integer; DecimalPlaces: Integer): Decimal
var
x: Integer;
Pow: Integer;

Choose a reason for hiding this comment

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

I would rename Pow to NumberWithLengthEqualsDecimalPlaces. This is much more understandable for future code readers

Copy link
Author

Choose a reason for hiding this comment

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

If you decide to rename Pow here, you'll need to update it also in

  • procedure DecimalInRange(MinValue: Decimal; MaxValue: Decimal; DecimalPlaces: Integer): Decimal
  • procedure DecimalInRange(MaxValue: Integer; DecimalPlaces: Integer): Decimal

Copy link
Contributor

Choose a reason for hiding this comment

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

It is too long, Pow or Power is intuitive enough. Let's keep the change small.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would use the very descriptive name if the method longer, for very a small method like this one, Pow or Power will do. This code looks nice IMO:

procedure DecimalInRange(MaxValue: Integer; DecimalPlaces: Integer): Decimal
var
    PseudoRandomInteger: Integer;
    Pow: Integer;
begin
    Pow := Power(10, DecimalPlaces);

    PseudoRandomInteger := IntegerInRange(MaxValue * Pow);
    if PseudoRandomInteger mod 10 = 0 then
        PseudoRandomInteger += IntegerInRange(1, 9);

    exit(PseudoRandomInteger / Pow);
end;

@@ -44,8 +44,17 @@ codeunit 130500 "Any"
/// <param name="DecimalPlaces">Number of decimal places</param>
/// <returns>Pseudo-random decimal value</returns>
procedure DecimalInRange(MaxValue: Integer; DecimalPlaces: Integer): Decimal
var
x: Integer;

Choose a reason for hiding this comment

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

I would rename x to RandomIntegerInRange. This is much more understandable for future code readers

Copy link
Contributor

Choose a reason for hiding this comment

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

PseudoRandomInteger? It is not random it is PseudoRandom. InRange is just implementation detail, thus I would not reflect it in the name. It is shorter as well.

Copy link
Author

Choose a reason for hiding this comment

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

@nikolakukrika

It is not random it is PseudoRandom.

Just for my information; could you please explain the difference?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you call Library Any or Library Random and ask for randIntInRange 5 times and get 11, 25, 1, 2, 4
When this transaction ends and you call the same function again, you will get the same numbers - 11, 25, 1, 2, 4.

With true random, you would get a completely different set of numbers.

This behavior is essential for the tests, otherwise we would not be able to reproduce failures on a local box. Pseudorandom is achieved by always setting a seed for the Any library and in a weird way in Library Random : ). I will implement clearing the seed after every test method for library any, so the tests in a codeunit and the tests individually will always get the same numbers.

The cool thing about it is that you can simply change a seed and all tests will use different sets of numbers. We are lacking an event for this, you can create a new PR for this if you are interested :), if not I will add it in a month or two when I take some time on the side to asses the stability of the tests.

I believe that this definition from the wiki defines it the best:
Pseudorandomness measures the extent to which a sequence of numbers, "though produced by a completely deterministic and repeatable process, appear to be patternless."

@bc-ghost
Copy link

Thanks for reporting this. We agree, and we’ll publish a fix asap, either in an update for the current version or in the next major release. We will update this issue with information about availability.

@bc-ghost bc-ghost added the ships-in-future-update Fix ships in a future update label Sep 17, 2020
@bc-ghost bc-ghost closed this Sep 17, 2020
@JesperSchulz
Copy link
Contributor

Our ghost doesn't seem to merge the PR before it closes it. Reopening :-)

@JesperSchulz JesperSchulz reopened this Sep 17, 2020
@JesperSchulz JesperSchulz merged commit 2f2034d into microsoft:master Sep 17, 2020
@bc-ghost
Copy link

bc-ghost commented Oct 5, 2020

Thanks for submitting this issue. We will publish a fix for it in the next minor update for release 17.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
processing-PR The PR is currently being reviewed ships-in-future-update Fix ships in a future update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants