Zoned date time#3
Conversation
Largely simplifies things down a little more and reduces reliance on global settings such as timezones or clocks. Some other notable points: - simplifes ElecriticityReadingsGenerator with a stream. Supplies tests for this stream. - factors out PeakTimeMultiplier into a simple record - provides ElectricityReading with a secondary constructor to avoid the fixture factory - PricePLan is now constructed with simple Map<DayOfWeek, BigDecimal> rather than a list for more determinism - Tidies up imports and formatting using the code conventions defined in the build
| readings.add(electricityReading); | ||
| previousReading = currentReading; | ||
| } | ||
| public static Stream<ElectricityReading> generateElectricityReadingStream(int days) { |
There was a problem hiding this comment.
I'm not sure I buy returning a stream here. The implementation doesn't look much simpler, and every single caller wants a List instead of a Stream.
The name also adds a lot of stutter (unless every caller switches to a static import).
There was a problem hiding this comment.
I'd recommend the static import 😄 which was sort of my assumption anyway.
Lets think a bit more about streams - they're trivially convertible to lists anyway. To be honest, the functions also need some more possible parameters - eg the interval between the readings to generate and the function to generate the values. I hadn't quite finished removing all dependencies on hidden state such as Random...
| @DisplayName("Get price should return price given non-peak date and time") | ||
| public void get_price_should_return_the_base_price_given_an_ordinary_date_time() { | ||
| ZonedDateTime nonPeakDateTime = ZonedDateTime.of(LocalDateTime.of(2017, Month.AUGUST, 31, 12, 0, 0), | ||
| ZoneId.of("GMT")); |
There was a problem hiding this comment.
I haven't thought this through completely, but what's the value in using a time zone? I presume:
- the local date time is in the time zone of the meter that produced the reading
- only the local date/time matters when determining the day and therefore the zone isn't necessary
- we'll never compare dates from two different meters in meaningful ways
There was a problem hiding this comment.
Consider the following. In the model, there is no time zone associated with a meter, rather the readings are captured as Instant - as in 'absolute' moments in time and not associated with a time zone.
So, such a reading could be in different days depending on where we are in the world. As the Javadoc says, there's no way to convert to an instant with an offset or timezone. Likewise, there's no way to map an Instant to a day of the week without an offset or timezone.
To make that conversion, you'd need to supply a Zone whichever way, and in the worst case you'd arbitrarily choose whatever the system property gave you, making yourself dependent on hidden global variables.
There was a problem hiding this comment.
Oh, huh, yeah. Then it seems most correct to associate a zone with the measurements (or with the smart meter). But that'd be a bigger change.
There was a problem hiding this comment.
This is still being used by CostComparisonTest, which no longer compiles.
There was a problem hiding this comment.
It works on my machine ... That's possibly because it seems to have disappeared. I'll double check what's happening there.
There was a problem hiding this comment.
I don't actually see that test in the branch option-0-no-api, so I'm a bit puzzled. Could you take another look?
There was a problem hiding this comment.
My bad, somehow that one was sitting around from another branch 🤷
|
|
||
| private static final ZoneId GMT = ZoneId.of("GMT"); | ||
| private static final String SMART_METER_ID = "10101010"; | ||
| private static final Clock FIXED_CLOCK = Clock.fixed(Instant.ofEpochSecond(1721124813L), GMT); |
There was a problem hiding this comment.
Curious what the value of using a fixed clock is here. I get the general idea of fixing a clock to test time-based code, but it seems like we're not actually leveraging it here, and the resulting code is harder to understand (to my eyes anyway).
And the hardcoded epoch seconds are a bit distracting. Makes me curious what the significance is, and I have to use a converter just to find out that it's when you made this change 😅
There was a problem hiding this comment.
Ok, that was maybe a bit cute. I'll make that easier to understand.
What I generally wanted to avoid was the dependence on the system clock or other hidden global variables.
You're right that it's not used in these specific tests, but it's sort of a matter of principle too 😄
…r.java Co-authored-by: Jeremy Huiskamp <jeremyhuiskamp@users.noreply.github.com>
…rTest.java Co-authored-by: Jeremy Huiskamp <jeremyhuiskamp@users.noreply.github.com>
…immutable empty map of peak time modifiers. Modifies the existing constructor to remove unnecessary (and broken) conversion from set to map.
| this.energySupplier = energySupplier; | ||
| this.unitRate = unitRate; | ||
| this.peakTimeMultipliers = peakTimeMultipliers; | ||
| this.peakTimeMultipliers = Collections.unmodifiableMap(new HashMap<>(peakTimeMultipliers)); |
There was a problem hiding this comment.
| this.peakTimeMultipliers = Collections.unmodifiableMap(new HashMap<>(peakTimeMultipliers)); | |
| this.peakTimeMultipliers = Map.copyOf(peakTimeMultipliers); |
Credit goes to IntelliJ for that suggestion...
| public record ElectricityReading(Instant time, BigDecimal reading) {} | ||
| public record ElectricityReading(Instant time, BigDecimal readingInKwH) { | ||
|
|
||
| public ElectricityReading(Clock clock, double readingInKwH) { |
There was a problem hiding this comment.
Is this constructor meant to be for testing purposes (faking the current time, lighter syntax for the reading) or also for production usage?
2 participants
What is being fixed - and why?
Excess use of global configuration (including time zones). Started with introducing
ClockandZonedDateTime.Followed up with some further simplification and refactorings which I hope make sense.
What has changed?
Some other notable points: