Configurable temporal unit in DurationRandomizer

[lutovich]

This change makes it possible to create a DurationRandomizer that generates durations of the specified TemporalUnit. When not configured, the default unit is hours.

[fmbenhassine]

Hi @lutovich,

Thank you for opening this PR! I like the idea, but I would implement it differently. What about making the ChronoUnit configurable instead of picking a random unit?

The changes to EnumRandomizer that allow to exclude values from the enum will make things inconsistent. The reason is that if we do it for enums, we need to support it for other randomizers as well, like with IntegerRangeRandomizer, asking for a random number in a given range but also exclude some values from within the range..

Do you agree?

Kr,
Mahmoud

[lutovich]

Hi @benas,

Thanks for your response!

The change in EnumRandomizer exposes the following constructor:
EnumRandomizer(Class<E> enumeration, long seed, E... excludedValues)
so that DurationRandomizer can pass in its seed.
There already exists a constructor that accepts excluded values:
EnumRandomizer(Class<E> enumeration, E... excludedValues).

Thus I feel like this PR does not add much new functionality to the EnumRandomizer but just exposes another way to instantiate it.

What do you think?

Making ChronoUnit configurable sounds good too.

[fmbenhassine]

There already exists a constructor that accepts excluded values:

Oh my bad, sorry for that.

Making ChronoUnit configurable sounds good too.

Great, in that case, please update the PR accordingly and it will be good to merge. We would need a new constructor public DurationRandomizer(final long seed, final ChronoUnit unit) (with its corresponding static factory method) and make the unit defaults to HOURS (for backward compatibility).

Do not hesitate to add your name to the contributors list as well, you deserve the credit!

[lutovich]

@benas PR is now updated. Thank you!

[fmbenhassine]

Thanks for the updates. However, I thought we agreed on passing ChronoUnit as parameter and not TemporalUnit. I do not understand the purpose of requireValid method, does this mean we can't generate a duration in DAYS?

By making the ChronoUnit configurable I meant something like:

Index: easy-random-core/src/main/java/org/jeasy/random/randomizers/time/DurationRandomizer.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- easy-random-core/src/main/java/org/jeasy/random/randomizers/time/DurationRandomizer.java	(revision a7ea389b71671760d63d7a9568082881d16aa9be)
+++ easy-random-core/src/main/java/org/jeasy/random/randomizers/time/DurationRandomizer.java	(date 1572644643611)
@@ -40,6 +40,7 @@
     private static final int MAX_AMOUNT = 100;
 
     private final IntegerRangeRandomizer amountRandomizer;
+    private ChronoUnit chronoUnit = ChronoUnit.HOURS;
 
     /**
      * Create a new {@link DurationRandomizer}.
@@ -56,6 +57,17 @@
     public DurationRandomizer(final long seed) {
         amountRandomizer = new IntegerRangeRandomizer(MIN_AMOUNT, MAX_AMOUNT, seed);
     }
+
+    /**
+     * Create a new {@link DurationRandomizer}.
+     *
+     * @param seed initial seed
+     * @param chronoUnit unit of the random duration
+     */
+    public DurationRandomizer(final long seed, final ChronoUnit chronoUnit) {
+        amountRandomizer = new IntegerRangeRandomizer(MIN_AMOUNT, MAX_AMOUNT, seed);
+        this.chronoUnit = chronoUnit;
+    }
 
     /**
      * Create a new {@link DurationRandomizer}.
@@ -76,10 +88,21 @@
         return new DurationRandomizer(seed);
     }
 
+    /**
+     * Create a new {@link DurationRandomizer}.
+     *
+     * @param seed initial seed
+     * @param chronoUnit unit of the random duration
+     * @return a new {@link DurationRandomizer}.
+     */
+    public static DurationRandomizer aNewDurationRandomizer(final long seed, final ChronoUnit chronoUnit) {
+        return new DurationRandomizer(seed, chronoUnit);
+    }
+
     @Override
     public Duration getRandomValue() {
         int randomAmount = amountRandomizer.getRandomValue();
-        return Duration.of(randomAmount, ChronoUnit.HOURS);
+        return Duration.of(randomAmount, this.chronoUnit);
     }
 
 }

[lutovich]

@benas ChronoUnit implements TemporalUnit interface. The factory method used to create durations has this signature Duration#of(long amount, TemporalUnit unit). That's why I think it is reasonable to also accept a TemporalUnit in constructors of the DurationRandomizer.

Method requireValid prevents users from creating a DurationRandomizer that will throw on every attempt to generate a duration. Not every TemporalUnit is valid for Duration#of(). For example:

jshell> Duration.of(1, ChronoUnit.SECONDS)
$4 ==> PT1S

jshell> Duration.of(1, ChronoUnit.DAYS)
$5 ==> PT24H

jshell> Duration.of(1, ChronoUnit.MONTHS)
|  Exception java.time.temporal.UnsupportedTemporalTypeException: Unit must not have an estimated duration
|        at Duration.plus (Duration.java:715)
|        at Duration.of (Duration.java:310)
|        at (#6:1)

Condition unit.isDurationEstimated() && unit != ChronoUnit.DAYS allows units from NANOS to DAYS and disallows units from WEEKS to FOREVER. Please see the added test.

[fmbenhassine]

The factory method used to create durations has this signature Duration#of(long amount, TemporalUnit unit).

I did not notice that. In that case, we should definitely use TemporalUnit and not ChronoUnit. And did not imagine Duration.of(1, ChronoUnit.MONTHS) would throw an exception! I learned something interesting about durations here, so thank you!

The PR LGTM, I will merge it.

Thank you for your contribution!