From d45ed0f53629c2c25154cbae12451a94628f9c90 Mon Sep 17 00:00:00 2001 From: Laurent Garnier Date: Sat, 18 Nov 2023 16:09:30 +0100 Subject: [PATCH] Reivew comment: use TemporalAmount Signed-off-by: Laurent Garnier --- .../core/ui/internal/chart/ChartServlet.java | 39 ++--- .../chart/ChartServletPeriodParamTest.java | 165 +++++++----------- 2 files changed, 76 insertions(+), 128 deletions(-) diff --git a/bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/chart/ChartServlet.java b/bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/chart/ChartServlet.java index e71e0901df0..82091f3f92f 100644 --- a/bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/chart/ChartServlet.java +++ b/bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/chart/ChartServlet.java @@ -20,6 +20,7 @@ import java.time.ZonedDateTime; import java.time.format.DateTimeFormatter; import java.time.format.DateTimeParseException; +import java.time.temporal.TemporalAmount; import java.util.Map; import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; @@ -99,7 +100,7 @@ public class ChartServlet extends HttpServlet { // The URI of this servlet public static final String SERVLET_PATH = "/chart"; - private static final Duration DEFAULT_DURATION = Duration.ofDays(1); + private static final Duration DEFAULT_PERIOD = Duration.ofDays(1); protected static final Map CHART_PROVIDERS = new ConcurrentHashMap<>(); @@ -223,8 +224,7 @@ protected void doGet(HttpServletRequest req, HttpServletResponse res) throws Ser } // Read out the parameter period, begin and end and save them. - Period period = convertToPeriod(periodParam, Period.ZERO); - Duration duration = convertToDuration(periodParam, DEFAULT_DURATION); + TemporalAmount period = convertToTemporalAmount(periodParam, DEFAULT_PERIOD); ZonedDateTime timeBegin = null; ZonedDateTime timeEnd = null; @@ -251,13 +251,13 @@ protected void doGet(HttpServletRequest req, HttpServletResponse res) throws Ser // Set begin and end time and check legality. if (timeBegin == null && timeEnd == null) { timeEnd = ZonedDateTime.now(timeZoneProvider.getTimeZone()); - timeBegin = timeEnd.minus(!period.isZero() ? period : duration); + timeBegin = timeEnd.minus(period); logger.debug("No begin or end is specified, use now as end and now-period as begin."); } else if (timeEnd == null) { - timeEnd = timeBegin.plus(!period.isZero() ? period : duration); + timeEnd = timeBegin.plus(period); logger.debug("No end is specified, use begin + period as end."); } else if (timeBegin == null) { - timeBegin = timeEnd.minus(!period.isZero() ? period : duration); + timeBegin = timeEnd.minus(period); logger.debug("No begin is specified, use end-period as begin"); } else if (timeEnd.isBefore(timeBegin)) { res.sendError(HttpServletResponse.SC_BAD_REQUEST, "The end is before the begin."); @@ -351,30 +351,25 @@ public void init(@Nullable ServletConfig config) throws ServletException { public void destroy() { } - public static Period convertToPeriod(@Nullable String periodParam, Period defaultPeriod) { - Period period = defaultPeriod; + public static TemporalAmount convertToTemporalAmount(@Nullable String periodParam, TemporalAmount defaultPeriod) { + TemporalAmount period = defaultPeriod; String convertedPeriod = convertPeriodToISO8601(periodParam); if (convertedPeriod != null) { + boolean failed = false; try { period = Period.parse(convertedPeriod); } catch (DateTimeParseException e) { - // Ignored + failed = true; } - } - return period; - } - - public static Duration convertToDuration(@Nullable String periodParam, Duration defaultDuration) { - Duration duration = defaultDuration; - String convertedPeriod = convertPeriodToISO8601(periodParam); - if (convertedPeriod != null) { - try { - duration = Duration.parse(convertedPeriod); - } catch (DateTimeParseException e) { - // Ignored + if (failed) { + try { + period = Duration.parse(convertedPeriod); + } catch (DateTimeParseException e) { + // Ignored + } } } - return duration; + return period; } private static @Nullable String convertPeriodToISO8601(@Nullable String period) { diff --git a/bundles/org.openhab.core.ui/src/test/java/org/openhab/core/ui/internal/chart/ChartServletPeriodParamTest.java b/bundles/org.openhab.core.ui/src/test/java/org/openhab/core/ui/internal/chart/ChartServletPeriodParamTest.java index 2f1cde665b5..31e0d642d2c 100644 --- a/bundles/org.openhab.core.ui/src/test/java/org/openhab/core/ui/internal/chart/ChartServletPeriodParamTest.java +++ b/bundles/org.openhab.core.ui/src/test/java/org/openhab/core/ui/internal/chart/ChartServletPeriodParamTest.java @@ -12,10 +12,11 @@ */ package org.openhab.core.ui.internal.chart; -import static org.junit.jupiter.api.Assertions.*; +import static org.junit.jupiter.api.Assertions.assertEquals; import java.time.Duration; -import java.time.Period; +import java.time.temporal.ChronoUnit; +import java.time.temporal.TemporalAmount; import org.eclipse.jdt.annotation.NonNullByDefault; import org.junit.jupiter.api.Test; @@ -27,128 +28,80 @@ public class ChartServletPeriodParamTest { @Test - public void convertToPeriodFromNull() { - Period period = ChartServlet.convertToPeriod(null, Period.ZERO); - assertTrue(period.isZero()); + public void convertToTemporalAmountFromNull() { + TemporalAmount period = ChartServlet.convertToTemporalAmount(null, Duration.ZERO); + assertEquals(0, period.get(ChronoUnit.SECONDS)); } @Test - public void convertToPeriodFromHours() { - Period period = ChartServlet.convertToPeriod("2h", Period.ZERO); - assertTrue(period.isZero()); - } - - @Test - public void convertToPeriodFromDays() { - Period period = ChartServlet.convertToPeriod("D", Period.ZERO); - assertEquals(1, period.getDays()); - assertEquals(0, period.getMonths()); - assertEquals(0, period.getYears()); - - period = ChartServlet.convertToPeriod("4D", Period.ZERO); - assertEquals(4, period.getDays()); - assertEquals(0, period.getMonths()); - assertEquals(0, period.getYears()); - } - - @Test - public void convertToPeriodFromWeeks() { - Period period = ChartServlet.convertToPeriod("W", Period.ZERO); - assertEquals(7, period.getDays()); - assertEquals(0, period.getMonths()); - assertEquals(0, period.getYears()); - - period = ChartServlet.convertToPeriod("2W", Period.ZERO); - assertEquals(14, period.getDays()); - assertEquals(0, period.getMonths()); - assertEquals(0, period.getYears()); - } + public void convertToTemporalAmountFromHours() { + TemporalAmount period = ChartServlet.convertToTemporalAmount("h", Duration.ZERO); + assertEquals(1 * 60 * 60, period.get(ChronoUnit.SECONDS)); - @Test - public void convertToPeriodFromMonths() { - Period period = ChartServlet.convertToPeriod("M", Period.ZERO); - assertEquals(0, period.getDays()); - assertEquals(1, period.getMonths()); - assertEquals(0, period.getYears()); - - period = ChartServlet.convertToPeriod("3M", Period.ZERO); - assertEquals(0, period.getDays()); - assertEquals(3, period.getMonths()); - assertEquals(0, period.getYears()); + period = ChartServlet.convertToTemporalAmount("12h", Duration.ZERO); + assertEquals(12 * 60 * 60, period.get(ChronoUnit.SECONDS)); } @Test - public void convertToPeriodFromYears() { - Period period = ChartServlet.convertToPeriod("Y", Period.ZERO); - assertEquals(0, period.getDays()); - assertEquals(0, period.getMonths()); - assertEquals(1, period.getYears()); - - period = ChartServlet.convertToPeriod("2Y", Period.ZERO); - assertEquals(0, period.getDays()); - assertEquals(0, period.getMonths()); - assertEquals(2, period.getYears()); + public void convertToTemporalAmountFromDays() { + TemporalAmount period = ChartServlet.convertToTemporalAmount("D", Duration.ZERO); + assertEquals(1, period.get(ChronoUnit.DAYS)); + assertEquals(0, period.get(ChronoUnit.MONTHS)); + assertEquals(0, period.get(ChronoUnit.YEARS)); + + period = ChartServlet.convertToTemporalAmount("4D", Duration.ZERO); + assertEquals(4, period.get(ChronoUnit.DAYS)); + assertEquals(0, period.get(ChronoUnit.MONTHS)); + assertEquals(0, period.get(ChronoUnit.YEARS)); } @Test - public void convertToPeriodFromISO8601() { - Period period = ChartServlet.convertToPeriod("P2Y3M4D", Period.ZERO); - assertEquals(4, period.getDays()); - assertEquals(3, period.getMonths()); - assertEquals(2, period.getYears()); - - period = ChartServlet.convertToPeriod("P1DT12H30M15S", Period.ZERO); - assertTrue(period.isZero()); + public void convertToTemporalAmountFromWeeks() { + TemporalAmount period = ChartServlet.convertToTemporalAmount("W", Duration.ZERO); + assertEquals(7, period.get(ChronoUnit.DAYS)); + assertEquals(0, period.get(ChronoUnit.MONTHS)); + assertEquals(0, period.get(ChronoUnit.YEARS)); + + period = ChartServlet.convertToTemporalAmount("2W", Duration.ZERO); + assertEquals(14, period.get(ChronoUnit.DAYS)); + assertEquals(0, period.get(ChronoUnit.MONTHS)); + assertEquals(0, period.get(ChronoUnit.YEARS)); } @Test - public void convertToDurationFromNull() { - Duration duration = ChartServlet.convertToDuration(null, Duration.ZERO); - assertTrue(duration.isZero()); + public void convertToTemporalAmountFromMonths() { + TemporalAmount period = ChartServlet.convertToTemporalAmount("M", Duration.ZERO); + assertEquals(0, period.get(ChronoUnit.DAYS)); + assertEquals(1, period.get(ChronoUnit.MONTHS)); + assertEquals(0, period.get(ChronoUnit.YEARS)); + + period = ChartServlet.convertToTemporalAmount("3M", Duration.ZERO); + assertEquals(0, period.get(ChronoUnit.DAYS)); + assertEquals(3, period.get(ChronoUnit.MONTHS)); + assertEquals(0, period.get(ChronoUnit.YEARS)); } @Test - public void convertToDurationFromHours() { - Duration duration = ChartServlet.convertToDuration("h", Duration.ZERO); - assertEquals(1 * 60 * 60, duration.getSeconds()); - - duration = ChartServlet.convertToDuration("12h", Duration.ZERO); - assertEquals(12 * 60 * 60, duration.getSeconds()); + public void convertToTemporalAmountFromYears() { + TemporalAmount period = ChartServlet.convertToTemporalAmount("Y", Duration.ZERO); + assertEquals(0, period.get(ChronoUnit.DAYS)); + assertEquals(0, period.get(ChronoUnit.MONTHS)); + assertEquals(1, period.get(ChronoUnit.YEARS)); + + period = ChartServlet.convertToTemporalAmount("2Y", Duration.ZERO); + assertEquals(0, period.get(ChronoUnit.DAYS)); + assertEquals(0, period.get(ChronoUnit.MONTHS)); + assertEquals(2, period.get(ChronoUnit.YEARS)); } @Test - public void convertToDurationFromDays() { - Duration duration = ChartServlet.convertToDuration("D", Duration.ZERO); - assertEquals(24 * 60 * 60, duration.getSeconds()); - - duration = ChartServlet.convertToDuration("2D", Duration.ZERO); - assertEquals(48 * 60 * 60, duration.getSeconds()); - } - - @Test - public void convertToDurationFromWeeks() { - Duration duration = ChartServlet.convertToDuration("2W", Duration.ZERO); - assertTrue(duration.isZero()); - } - - @Test - public void convertToDurationFromMonths() { - Duration duration = ChartServlet.convertToDuration("3M", Duration.ZERO); - assertTrue(duration.isZero()); - } - - @Test - public void convertToDurationFromYears() { - Duration duration = ChartServlet.convertToDuration("2Y", Duration.ZERO); - assertTrue(duration.isZero()); - } - - @Test - public void convertToDurationFromISO8601() { - Duration duration = ChartServlet.convertToDuration("P1DT12H30M15S", Duration.ZERO); - assertEquals(36 * 60 * 60 + 30 * 60 + 15, duration.getSeconds()); - - duration = ChartServlet.convertToDuration("P2Y3M4D", Duration.ZERO); - assertTrue(duration.isZero()); + public void convertToTemporalAmountFromISO8601() { + TemporalAmount period = ChartServlet.convertToTemporalAmount("P2Y3M4D", Duration.ZERO); + assertEquals(4, period.get(ChronoUnit.DAYS)); + assertEquals(3, period.get(ChronoUnit.MONTHS)); + assertEquals(2, period.get(ChronoUnit.YEARS)); + + period = ChartServlet.convertToTemporalAmount("P1DT12H30M15S", Duration.ZERO); + assertEquals(36 * 60 * 60 + 30 * 60 + 15, period.get(ChronoUnit.SECONDS)); } }