inconsistent behaviour of `nextInt` and `nextLong`

[asolntsev]

Describe the bug Methods nextInt and nextLong have inconsistent behaviour. The "max" parameter is exclusive for most of them, but inclusive for one of them.

• nextInt(3): [0, 1, 2] - exclusive
• nextInt(2, 4): [2, 3, 4] - INCLUSIVE
• nextLong(3): [0, 1, 2] - exclusive
• nextLong(2, 4): [2, 3] - exclusive
• nextDouble(2, 3) - exclusive

To Reproduce

    @RepeatedTest(100)
    void exclusive() {
        assertThat(randomService.nextInt(3)).isLessThan(3); // ok
        assertThat(randomService.nextInt(1, 3)).isLessThan(3); // fails because 3==3
        assertThat(randomService.nextLong(3)).isLessThan(3); //ok
        assertThat(randomService.nextLong(1, 3)).isLessThan(3); // ok
        assertThat(randomService.nextDouble(2, 3)).isLessThan(3); // ok
    }

Expected behavior All these methods should have the same behaviour: either inclusive or exclusive. I assume exclusive is better because

1. most of them are already exclusive
2. Standard java methods java.lang.Random.nextInt(n), java.lang.Random.nextLong(n) are exclusive.

@bodiam @kingthorin What do you choose: inclusive or exclusive?

Versions:

• Faker Version: 1.0 ... 2.4.0

[kingthorin]

My vote is for exclusive.

[bodiam]

I'm not sure about this really. On one side it's nice to keep it all aligned, on the other hand, it's quite a breaking change, and would need to be communicated first. So, perhaps we need to make things deprecated first or so, maybe even without removing it, and changing the contract in version 3.0? I think it would lead to subtle and annoying bugs if we do it as part of the 2.x version, and it's a breaking change, so a 3.x would make more sense perhaps.

Another question: what's the expectation when we pick exclusive, and someone does this: nextInt(2, 2) ?

Right now, nextInt(2,2) and nextLong(2,2) both return 2. Is that in line with the contract (including the lower bound) or not in line, since it should be excluding the upper bound?

[asolntsev]

@bodiam I agree that it should be communicated first. And maybe it's not worth it. It's enough just to describe the current behaviour in javadoc (I already did it).

P.S. If we decide for "exclusive" behaviour, then nextInt(2,2) should throw IllgalArgumentException.

[bodiam]

@asolntsev I'm a big fan of consistency, and I also have to check the source code every time for what it's doing, since I never really know if the documentation, if any, is correct. So, I do agree with you, consistency would be better, and also agree on the IllegalArugmentException. If we'd make the offending methods deprecated now, with a small note on it, we could communicate it. The only downside is that a lot of code would be considered deprecated, even our own code, without a clear deadline when we're really going to pull the plug.

Thoughts?

[asolntsev]

Current state is quite ok, I would say. Method nextLong is documented, meaning of "min" and "max" is explained.

If we want to improve it, then I suggest to

1. Leave methods nextLong(min, max) and nextInt(min, max) as is, at least until the next major release.
2. Create new methods nextLong(range) and nextInt(range). Where Range is the class that explicitly defines "inclusive" or "exclusive" for both ends".

The usage would look like this:

randomService.nextLong(Range.inclusive(2, 4)) // 2, 3, 4
randomService.nextInt(Range.inclusiveExclusive(2, 5)) // 2, 3, 4
randomService.nextInt(Range.exclusiveInclusive(2, 5)) // 3, 4, 5
randomService.nextLong(Range.exclusive(2, 5))  // 3, 4

Class Range already exists in libraries like Guava and commons-lang, so we could create a similar class in DF as well.

[bodiam]

I like it. Seems pretty expressive. There's also Kotlin Range: https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.ranges/, which could be used as inspiration.

[kingthorin]

I like that idea too.

Do we want to remove the others or just decide on a default and have them use the new code internally for the next major?

[asolntsev]

have them use the new code internally for the next major?

I vote for "have them use the new code internally for the next major".