-
Notifications
You must be signed in to change notification settings - Fork 1
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
Zoned date time #3
base: option-0-no-api
Are you sure you want to change the base?
Changes from 1 commit
23e33a7
89e4c02
4c379c1
d2eb50d
19e10a6
c9c2268
54a628c
20ce194
6baa03c
50af408
aa0836c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
…ate time
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,13 @@ | ||
package tw.joi.energy.domain; | ||
|
||
import static java.util.Collections.emptyList; | ||
import static java.util.Collections.emptySet; | ||
import static org.assertj.core.api.AssertionsForInterfaceTypes.assertThat; | ||
|
||
import java.math.BigDecimal; | ||
import java.time.LocalDateTime; | ||
import java.time.Month; | ||
import java.time.ZoneId; | ||
import java.time.ZoneOffset; | ||
import java.time.ZonedDateTime; | ||
import java.util.TimeZone; | ||
import org.assertj.core.data.Percentage; | ||
import org.junit.jupiter.api.DisplayName; | ||
import org.junit.jupiter.api.Test; | ||
|
||
|
@@ -19,7 +16,7 @@ public class PricePlanTest { | |
@Test | ||
@DisplayName("Get energy supplier should return supplier if not null") | ||
public void get_energy_supplier_should_return_the_energy_supplier_given_supplier_is_existent() { | ||
PricePlan pricePlan = new PricePlan("Test Plan Name", "Energy Supplier Name", BigDecimal.ONE, emptyList()); | ||
PricePlan pricePlan = new PricePlan("Test Plan Name", "Energy Supplier Name", BigDecimal.ONE, emptySet()); | ||
|
||
assertThat(pricePlan.getEnergySupplier()).isEqualTo("Energy Supplier Name"); | ||
} | ||
|
@@ -30,7 +27,7 @@ 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't thought this through completely, but what's the value in using a time zone? I presume:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider the following. In the model, there is no time zone associated with a meter, rather the readings are captured as 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
// the price plan has no peak days.... | ||
PricePlan pricePlan = new PricePlan("test plan", "test supplier", BigDecimal.ONE, emptyList()); | ||
PricePlan pricePlan = new PricePlan("test plan", "test supplier", BigDecimal.ONE, emptySet()); | ||
|
||
BigDecimal price = pricePlan.getPrice(nonPeakDateTime); | ||
|
||
|
@@ -40,7 +37,7 @@ public void get_price_should_return_the_base_price_given_an_ordinary_date_time() | |
@Test | ||
@DisplayName("Get unit rate should return unit rate if no null") | ||
public void get_unit_rate_should_return_unit_rate_given_unit_rate_is_present() { | ||
PricePlan pricePlan = new PricePlan("test-price-plan", "test-energy-supplier", BigDecimal.TWO, emptyList()); | ||
PricePlan pricePlan = new PricePlan("test-price-plan", "test-energy-supplier", BigDecimal.TWO, emptySet()); | ||
|
||
BigDecimal rate = pricePlan.getUnitRate(); | ||
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is still being used by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't actually see that test in the branch There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My bad, somehow that one was sitting around from another branch 🤷 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,22 @@ | ||
package tw.joi.energy.fixture; | ||
|
||
import java.math.BigDecimal; | ||
import java.util.Collections; | ||
import tw.joi.energy.domain.PricePlan; | ||
|
||
import static java.util.Collections.emptySet; | ||
|
||
public class PricePlanFixture { | ||
|
||
public static final String WORST_PLAN_ID = "worst-supplier"; | ||
public static final String BEST_PLAN_ID = "best-supplier"; | ||
public static final String SECOND_BEST_PLAN_ID = "second-best-supplier"; | ||
|
||
public static final PricePlan DEFAULT_PRICE_PLAN = | ||
new PricePlan(SECOND_BEST_PLAN_ID, "energy-supplier", BigDecimal.TWO, Collections.emptyList()); | ||
new PricePlan(SECOND_BEST_PLAN_ID, "energy-supplier", BigDecimal.TWO, emptySet()); | ||
|
||
public static final PricePlan WORST_PRICE_PLAN = | ||
new PricePlan(WORST_PLAN_ID, null, BigDecimal.TEN, Collections.emptyList()); | ||
new PricePlan(WORST_PLAN_ID, null, BigDecimal.TEN, emptySet()); | ||
|
||
public static final PricePlan BEST_PRICE_PLAN = | ||
new PricePlan(BEST_PLAN_ID, null, BigDecimal.ONE, Collections.emptyList()); | ||
new PricePlan(BEST_PLAN_ID, null, BigDecimal.ONE, emptySet()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 aStream
.The name also adds a lot of stutter (unless every caller switches to a static import).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...