From f8ae51b0e7110e58528e01c0a32252a5222ea23a Mon Sep 17 00:00:00 2001 From: Roshni Joshi Date: Tue, 21 Nov 2023 18:55:47 -0400 Subject: [PATCH 1/8] refactor: Resolved Feature Envy design smell --- .../field/constraint/FieldConstraints.java | 36 +++++++++++++++ .../ValidationFieldExpressionVisitor.java | 45 +++---------------- .../constraints/FieldConstraintsTest.java | 21 +++++++++ .../ValidationFieldExpressionVisitorTest.java | 18 -------- 4 files changed, 62 insertions(+), 58 deletions(-) diff --git a/src/main/java/com/cronutils/model/field/constraint/FieldConstraints.java b/src/main/java/com/cronutils/model/field/constraint/FieldConstraints.java index b2b3b37a..a9667073 100755 --- a/src/main/java/com/cronutils/model/field/constraint/FieldConstraints.java +++ b/src/main/java/com/cronutils/model/field/constraint/FieldConstraints.java @@ -13,8 +13,11 @@ package com.cronutils.model.field.constraint; +import com.cronutils.model.field.value.FieldValue; +import com.cronutils.model.field.value.IntegerFieldValue; import com.cronutils.model.field.value.SpecialChar; import com.cronutils.utils.Preconditions; +import com.cronutils.utils.VisibleForTesting; import java.io.Serializable; import java.util.Collections; @@ -66,6 +69,22 @@ public int getEndRange() { public Set getSpecialChars() { return specialChars; } + + /** + * Check if given number is greater or equal to start range and minor or equal to end range. + * + * @param fieldValue - to be validated + * @throws IllegalArgumentException - if not in range + */ + @VisibleForTesting + public void isInRange(final FieldValue fieldValue) { + if (fieldValue instanceof IntegerFieldValue) { + final int value = ((IntegerFieldValue) fieldValue).getValue(); + if (!isInRange(value)) { + throw new IllegalArgumentException(String.format("Value %s not in range [%s, %s]", value, getStartRange(), getEndRange())); + } + } + } /** * Check if given number is greater or equal to start range and minor or equal to end range. @@ -76,6 +95,23 @@ public boolean isInRange(final int value) { return value >= getStartRange() && value <= getEndRange(); } + /** + * Check if given period is compatible with range. + * + * @param fieldValue - to be validated + * @throws IllegalArgumentException - if not in range + */ + @VisibleForTesting + public void isPeriodInRange(final FieldValue fieldValue) { + if (fieldValue instanceof IntegerFieldValue) { + final int value = ((IntegerFieldValue) fieldValue).getValue(); + if (!isPeriodInRange(value)) { + throw new IllegalArgumentException( + String.format("Period %s not in range [%s, %s]", value, getStartRange(), getEndRange())); + } + } + } + /** * Check if given period is compatible with the given range. * diff --git a/src/main/java/com/cronutils/model/field/expression/visitor/ValidationFieldExpressionVisitor.java b/src/main/java/com/cronutils/model/field/expression/visitor/ValidationFieldExpressionVisitor.java index 0aa2bf97..679f3265 100755 --- a/src/main/java/com/cronutils/model/field/expression/visitor/ValidationFieldExpressionVisitor.java +++ b/src/main/java/com/cronutils/model/field/expression/visitor/ValidationFieldExpressionVisitor.java @@ -23,8 +23,6 @@ import com.cronutils.utils.VisibleForTesting; public class ValidationFieldExpressionVisitor implements FieldExpressionVisitor { - private static final String OORANGE = "Value %s not in range [%s, %s]"; - private final FieldConstraints constraints; private final StringValidations stringValidations; @@ -84,7 +82,7 @@ public Every visit(final Every every) { this.checkUnsupportedChars(every); if (every.getExpression() != null) every.getExpression().accept(this); - isPeriodInRange(every.getPeriod()); + constraints.isPeriodInRange(every.getPeriod()); return every; } @@ -92,10 +90,10 @@ public Every visit(final Every every) { public On visit(final On on) { this.checkUnsupportedChars(on); if (!isDefault(on.getTime())) { - isInRange(on.getTime()); + constraints.isInRange(on.getTime()); } if (!isDefault(on.getNth())) { - isInRange(on.getNth()); + constraints.isInRange(on.getNth()); } return on; } @@ -107,46 +105,13 @@ public QuestionMark visit(final QuestionMark questionMark) { } private void preConditions(final Between between) { - isInRange(between.getFrom()); - isInRange(between.getTo()); + constraints.isInRange(between.getFrom()); + constraints.isInRange(between.getTo()); if (isSpecialCharNotL(between.getFrom()) || isSpecialCharNotL(between.getTo())) { throw new IllegalArgumentException("No special characters allowed in range, except for 'L'"); } } - /** - * Check if given number is greater or equal to start range and minor or equal to end range. - * - * @param fieldValue - to be validated - * @throws IllegalArgumentException - if not in range - */ - @VisibleForTesting - protected void isInRange(final FieldValue fieldValue) { - if (fieldValue instanceof IntegerFieldValue) { - final int value = ((IntegerFieldValue) fieldValue).getValue(); - if (!constraints.isInRange(value)) { - throw new IllegalArgumentException(String.format(OORANGE, value, constraints.getStartRange(), constraints.getEndRange())); - } - } - } - - /** - * Check if given period is compatible with range. - * - * @param fieldValue - to be validated - * @throws IllegalArgumentException - if not in range - */ - @VisibleForTesting - protected void isPeriodInRange(final FieldValue fieldValue) { - if (fieldValue instanceof IntegerFieldValue) { - final int value = ((IntegerFieldValue) fieldValue).getValue(); - if (!constraints.isPeriodInRange(value)) { - throw new IllegalArgumentException( - String.format("Period %s not in range [%s, %s]", value, constraints.getStartRange(), constraints.getEndRange())); - } - } - } - @VisibleForTesting protected boolean isDefault(final FieldValue fieldValue) { return fieldValue instanceof IntegerFieldValue && ((IntegerFieldValue) fieldValue).getValue() == -1; diff --git a/src/test/java/com/cronutils/model/field/constraints/FieldConstraintsTest.java b/src/test/java/com/cronutils/model/field/constraints/FieldConstraintsTest.java index 13ffeedd..7280d8b5 100755 --- a/src/test/java/com/cronutils/model/field/constraints/FieldConstraintsTest.java +++ b/src/test/java/com/cronutils/model/field/constraints/FieldConstraintsTest.java @@ -14,7 +14,10 @@ package com.cronutils.model.field.constraints; import com.cronutils.model.field.constraint.FieldConstraints; +import com.cronutils.model.field.value.IntegerFieldValue; import com.cronutils.model.field.value.SpecialChar; +import com.cronutils.model.field.value.SpecialCharFieldValue; + import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -32,6 +35,7 @@ public class FieldConstraintsTest { private int startRange; private int endRange; private boolean strictRange; + private FieldConstraints fieldConstraints; @BeforeEach public void setUp() { @@ -41,6 +45,7 @@ public void setUp() { startRange = 0; endRange = 59; strictRange = true; + fieldConstraints = new FieldConstraints(Collections.emptyMap(), Collections.emptyMap(), Collections.emptySet(), startRange, endRange, true); } @Test @@ -57,4 +62,20 @@ public void testConstructorIntMappingNull() { public void testSpecialCharsSetNull() { assertThrows(NullPointerException.class, () -> new FieldConstraints(stringMapping, intMapping, null, startRange, endRange, strictRange)); } + + @Test + public void testIsInRangeForFieldValue() { + final SpecialCharFieldValue nonIntegerFieldValue = new SpecialCharFieldValue(SpecialChar.LW); + fieldConstraints.isInRange(nonIntegerFieldValue); + + final IntegerFieldValue integerValue = new IntegerFieldValue(5); + fieldConstraints.isInRange(integerValue); + } + + @Test + public void testIsInRangeForFieldValueOORangeStrict() { + final IntegerFieldValue integerValue = new IntegerFieldValue(999); + assertThrows(IllegalArgumentException.class, () -> fieldConstraints.isInRange(integerValue)); + } + } diff --git a/src/test/java/com/cronutils/model/field/expression/visitor/ValidationFieldExpressionVisitorTest.java b/src/test/java/com/cronutils/model/field/expression/visitor/ValidationFieldExpressionVisitorTest.java index b6eb80e3..ba1dcd6f 100755 --- a/src/test/java/com/cronutils/model/field/expression/visitor/ValidationFieldExpressionVisitorTest.java +++ b/src/test/java/com/cronutils/model/field/expression/visitor/ValidationFieldExpressionVisitorTest.java @@ -464,22 +464,4 @@ public void testIsSpecialCharNotLWithIntegerFieldValue() { assertFalse(strictVisitor.isSpecialCharNotL(integerValue)); assertFalse(visitor.isSpecialCharNotL(integerValue)); } - - @Test - public void testIsInRange() { - final SpecialCharFieldValue nonIntegerFieldValue = new SpecialCharFieldValue(SpecialChar.LW); - strictVisitor.isInRange(nonIntegerFieldValue); - visitor.isInRange(nonIntegerFieldValue); - - final IntegerFieldValue integerValue = new IntegerFieldValue(5); - strictVisitor.isInRange(integerValue); - visitor.isInRange(integerValue); - } - - @Test - public void testIsInRangeOORangeStrict() { - final IntegerFieldValue integerValue = new IntegerFieldValue(HIGHOOR); - assertThrows(IllegalArgumentException.class, () -> strictVisitor.isInRange(integerValue)); - } - } From 00f457d381dcd1047ad238ec391bdc3e16be504a Mon Sep 17 00:00:00 2001 From: Roshni Joshi Date: Sat, 25 Nov 2023 19:08:51 -0400 Subject: [PATCH 2/8] refactor: Resolved cyclic hierarchy smell, applied push down method refactoring --- .../com/cronutils/builder/CronBuilder.java | 25 ++++++++++--------- .../NominalDescriptionStrategy.java | 2 +- .../descriptor/TimeDescriptionStrategy.java | 2 +- .../java/com/cronutils/mapper/CronMapper.java | 4 +-- .../model/field/expression/Always.java | 4 +++ .../model/field/expression/Every.java | 2 +- .../field/expression/FieldExpression.java | 8 ------ .../model/field/expression/QuestionMark.java | 4 +++ .../ValueMappingFieldExpressionVisitor.java | 2 +- .../model/time/ExecutionTimeBuilder.java | 5 ++-- .../com/cronutils/parser/FieldParser.java | 4 +-- src/test/java/com/cronutils/Issue421Test.java | 2 +- src/test/java/com/cronutils/Issue446Test.java | 4 +-- src/test/java/com/cronutils/Issue459Test.java | 2 +- src/test/java/com/cronutils/Issue480Test.java | 2 +- .../model/field/expression/AlwaysTest.java | 2 +- .../ValidationFieldExpressionVisitorTest.java | 10 ++++---- ...alueMappingFieldExpressionVisitorTest.java | 2 +- .../AlwaysFieldValueGeneratorTest.java | 2 +- .../parser/CronParserNicknameTest.java | 2 +- .../utils/descriptor/CronDescriptorTest.java | 15 +++++------ 21 files changed, 54 insertions(+), 51 deletions(-) diff --git a/src/main/java/com/cronutils/builder/CronBuilder.java b/src/main/java/com/cronutils/builder/CronBuilder.java index 3054c61a..44c6082f 100644 --- a/src/main/java/com/cronutils/builder/CronBuilder.java +++ b/src/main/java/com/cronutils/builder/CronBuilder.java @@ -21,6 +21,7 @@ import com.cronutils.model.field.CronFieldName; import com.cronutils.model.field.constraint.FieldConstraints; import com.cronutils.model.field.definition.FieldDefinition; +import com.cronutils.model.field.expression.Always; import com.cronutils.model.field.expression.FieldExpression; import com.cronutils.model.field.expression.On; import com.cronutils.model.field.expression.visitor.ValidationFieldExpressionVisitor; @@ -106,7 +107,7 @@ public static Cron yearly(final CronDefinition definition){ builder = builder.withMonth(new On(new IntegerFieldValue(1))); } if(definition.containsFieldDefinition(DAY_OF_WEEK)){ - builder = builder.withDoW(FieldExpression.always()); + builder = builder.withDoW(Always.always()); } return builder.instance(); } @@ -130,10 +131,10 @@ public static Cron monthly(final CronDefinition definition){ builder = builder.withDoM(new On(new IntegerFieldValue(1))); } if(definition.containsFieldDefinition(MONTH)){ - builder = builder.withMonth(FieldExpression.always()); + builder = builder.withMonth(Always.always()); } if(definition.containsFieldDefinition(DAY_OF_WEEK)){ - builder = builder.withDoW(FieldExpression.always()); + builder = builder.withDoW(Always.always()); } return builder.instance(); } @@ -150,10 +151,10 @@ public static Cron weekly(final CronDefinition definition){ builder = builder.withHour(new On(new IntegerFieldValue(0))); } if(definition.containsFieldDefinition(DAY_OF_MONTH)){ - builder = builder.withDoM(FieldExpression.always()); + builder = builder.withDoM(Always.always()); } if(definition.containsFieldDefinition(MONTH)){ - builder = builder.withMonth(FieldExpression.always()); + builder = builder.withMonth(Always.always()); } if(definition.containsFieldDefinition(DAY_OF_WEEK)){ builder = builder.withDoW(new On(new IntegerFieldValue(0))); @@ -173,13 +174,13 @@ public static Cron daily(final CronDefinition definition){ builder = builder.withHour(new On(new IntegerFieldValue(0))); } if(definition.containsFieldDefinition(DAY_OF_MONTH)){ - builder = builder.withDoM(FieldExpression.always()); + builder = builder.withDoM(Always.always()); } if(definition.containsFieldDefinition(MONTH)){ - builder = builder.withMonth(FieldExpression.always()); + builder = builder.withMonth(Always.always()); } if(definition.containsFieldDefinition(DAY_OF_WEEK)){ - builder = builder.withDoW(FieldExpression.always()); + builder = builder.withDoW(Always.always()); } return builder.instance(); } @@ -197,16 +198,16 @@ public static Cron hourly(final CronDefinition definition){ builder = builder.withMinute(new On(new IntegerFieldValue(0))); } if(definition.containsFieldDefinition(HOUR)){ - builder = builder.withHour(FieldExpression.always()); + builder = builder.withHour(Always.always()); } if(definition.containsFieldDefinition(DAY_OF_MONTH)){ - builder = builder.withDoM(FieldExpression.always()); + builder = builder.withDoM(Always.always()); } if(definition.containsFieldDefinition(MONTH)){ - builder = builder.withMonth(FieldExpression.always()); + builder = builder.withMonth(Always.always()); } if(definition.containsFieldDefinition(DAY_OF_WEEK)){ - builder = builder.withDoW(FieldExpression.always()); + builder = builder.withDoW(Always.always()); } return builder.instance(); } diff --git a/src/main/java/com/cronutils/descriptor/NominalDescriptionStrategy.java b/src/main/java/com/cronutils/descriptor/NominalDescriptionStrategy.java index 9739f0e1..cd10e500 100644 --- a/src/main/java/com/cronutils/descriptor/NominalDescriptionStrategy.java +++ b/src/main/java/com/cronutils/descriptor/NominalDescriptionStrategy.java @@ -20,7 +20,7 @@ import java.util.ResourceBundle; import java.util.Set; -import static com.cronutils.model.field.expression.FieldExpression.always; +import static com.cronutils.model.field.expression.Always.always; /** * Description strategy where a cron field number can be mapped to a name. diff --git a/src/main/java/com/cronutils/descriptor/TimeDescriptionStrategy.java b/src/main/java/com/cronutils/descriptor/TimeDescriptionStrategy.java index bd397840..13e2691a 100644 --- a/src/main/java/com/cronutils/descriptor/TimeDescriptionStrategy.java +++ b/src/main/java/com/cronutils/descriptor/TimeDescriptionStrategy.java @@ -23,7 +23,7 @@ import java.util.ResourceBundle; import java.util.Set; -import static com.cronutils.model.field.expression.FieldExpression.always; +import static com.cronutils.model.field.expression.Always.always; /** * Strategy to provide a human readable description to hh:mm:ss variations. diff --git a/src/main/java/com/cronutils/mapper/CronMapper.java b/src/main/java/com/cronutils/mapper/CronMapper.java index 8446fac4..101e1b95 100755 --- a/src/main/java/com/cronutils/mapper/CronMapper.java +++ b/src/main/java/com/cronutils/mapper/CronMapper.java @@ -41,8 +41,8 @@ import java.util.List; import java.util.Map; -import static com.cronutils.model.field.expression.FieldExpression.always; -import static com.cronutils.model.field.expression.FieldExpression.questionMark; +import static com.cronutils.model.field.expression.Always.always; +import static com.cronutils.model.field.expression.QuestionMark.questionMark; public class CronMapper { private final Map> mappings; diff --git a/src/main/java/com/cronutils/model/field/expression/Always.java b/src/main/java/com/cronutils/model/field/expression/Always.java index 17de3a01..626bc04f 100644 --- a/src/main/java/com/cronutils/model/field/expression/Always.java +++ b/src/main/java/com/cronutils/model/field/expression/Always.java @@ -45,4 +45,8 @@ public String asString() { public String toString() { return "Always{}"; } + + public static FieldExpression always() { + return INSTANCE; + } } diff --git a/src/main/java/com/cronutils/model/field/expression/Every.java b/src/main/java/com/cronutils/model/field/expression/Every.java index df457661..fe8aab7b 100644 --- a/src/main/java/com/cronutils/model/field/expression/Every.java +++ b/src/main/java/com/cronutils/model/field/expression/Every.java @@ -29,7 +29,7 @@ public class Every extends FieldExpression { private final IntegerFieldValue period; public Every(final IntegerFieldValue time) { - this(always(), time); + this(Always.always(), time); } public Every(final FieldExpression expression, final IntegerFieldValue period) { diff --git a/src/main/java/com/cronutils/model/field/expression/FieldExpression.java b/src/main/java/com/cronutils/model/field/expression/FieldExpression.java index f88c48cd..cf5b7d18 100644 --- a/src/main/java/com/cronutils/model/field/expression/FieldExpression.java +++ b/src/main/java/com/cronutils/model/field/expression/FieldExpression.java @@ -42,12 +42,4 @@ public And and(final FieldExpression exp) { * @return FieldExpression copied instance with visitor action performed. */ public abstract FieldExpression accept(final FieldExpressionVisitor visitor); - - public static FieldExpression always() { - return Always.INSTANCE; - } - - public static FieldExpression questionMark() { - return QuestionMark.INSTANCE; - } } diff --git a/src/main/java/com/cronutils/model/field/expression/QuestionMark.java b/src/main/java/com/cronutils/model/field/expression/QuestionMark.java index 151dc3a6..6df303ed 100644 --- a/src/main/java/com/cronutils/model/field/expression/QuestionMark.java +++ b/src/main/java/com/cronutils/model/field/expression/QuestionMark.java @@ -43,4 +43,8 @@ public String asString() { public String toString() { return "QuestionMark{}"; } + + public static FieldExpression questionMark() { + return INSTANCE; + } } diff --git a/src/main/java/com/cronutils/model/field/expression/visitor/ValueMappingFieldExpressionVisitor.java b/src/main/java/com/cronutils/model/field/expression/visitor/ValueMappingFieldExpressionVisitor.java index bee8ae04..9f35c994 100644 --- a/src/main/java/com/cronutils/model/field/expression/visitor/ValueMappingFieldExpressionVisitor.java +++ b/src/main/java/com/cronutils/model/field/expression/visitor/ValueMappingFieldExpressionVisitor.java @@ -18,7 +18,7 @@ import com.cronutils.model.field.value.FieldValue; import com.cronutils.model.field.value.IntegerFieldValue; -import static com.cronutils.model.field.expression.FieldExpression.questionMark; +import static com.cronutils.model.field.expression.QuestionMark.questionMark; /** * Performs a transformation on FieldExpression values. diff --git a/src/main/java/com/cronutils/model/time/ExecutionTimeBuilder.java b/src/main/java/com/cronutils/model/time/ExecutionTimeBuilder.java index 580237c4..70dccac8 100755 --- a/src/main/java/com/cronutils/model/time/ExecutionTimeBuilder.java +++ b/src/main/java/com/cronutils/model/time/ExecutionTimeBuilder.java @@ -20,12 +20,13 @@ import com.cronutils.model.field.constraint.FieldConstraintsBuilder; import com.cronutils.model.field.expression.FieldExpression; import com.cronutils.model.field.expression.On; +import com.cronutils.model.field.expression.QuestionMark; import com.cronutils.model.field.value.IntegerFieldValue; import com.cronutils.model.time.generator.FieldValueGenerator; import com.cronutils.model.time.generator.FieldValueGeneratorFactory; import com.cronutils.utils.Preconditions; -import static com.cronutils.model.field.expression.FieldExpression.always; +import static com.cronutils.model.field.expression.Always.always; /** * Builds required components to get previous/next execution to certain reference date. @@ -138,7 +139,7 @@ protected ExecutionTime build() { } if (daysOfYearCronField == null) { final FieldConstraints constraints = getConstraint(CronFieldName.DAY_OF_YEAR); - daysOfYearCronField = new CronField(CronFieldName.DAY_OF_YEAR, lowestAssigned ? FieldExpression.questionMark() : always(), + daysOfYearCronField = new CronField(CronFieldName.DAY_OF_YEAR, lowestAssigned ? QuestionMark.questionMark() : always(), constraints); } diff --git a/src/main/java/com/cronutils/parser/FieldParser.java b/src/main/java/com/cronutils/parser/FieldParser.java index cd8f703a..6591fa77 100755 --- a/src/main/java/com/cronutils/parser/FieldParser.java +++ b/src/main/java/com/cronutils/parser/FieldParser.java @@ -27,8 +27,8 @@ import java.util.regex.Pattern; -import static com.cronutils.model.field.expression.FieldExpression.always; -import static com.cronutils.model.field.expression.FieldExpression.questionMark; +import static com.cronutils.model.field.expression.Always.always; +import static com.cronutils.model.field.expression.QuestionMark.questionMark; import static com.cronutils.model.field.value.SpecialChar.*; /** diff --git a/src/test/java/com/cronutils/Issue421Test.java b/src/test/java/com/cronutils/Issue421Test.java index 75f26ce9..5cc5476b 100644 --- a/src/test/java/com/cronutils/Issue421Test.java +++ b/src/test/java/com/cronutils/Issue421Test.java @@ -13,7 +13,7 @@ import java.util.Locale; import java.util.Optional; -import static com.cronutils.model.field.expression.FieldExpression.questionMark; +import static com.cronutils.model.field.expression.QuestionMark.questionMark; import static com.cronutils.model.field.expression.FieldExpressionFactory.every; import static com.cronutils.model.field.expression.FieldExpressionFactory.on; import static org.junit.jupiter.api.Assertions.*; diff --git a/src/test/java/com/cronutils/Issue446Test.java b/src/test/java/com/cronutils/Issue446Test.java index 50f60b21..f639583d 100644 --- a/src/test/java/com/cronutils/Issue446Test.java +++ b/src/test/java/com/cronutils/Issue446Test.java @@ -13,8 +13,8 @@ import java.time.*; import java.util.Optional; -import static com.cronutils.model.field.expression.FieldExpression.always; -import static com.cronutils.model.field.expression.FieldExpression.questionMark; +import static com.cronutils.model.field.expression.Always.always; +import static com.cronutils.model.field.expression.QuestionMark.questionMark; import static com.cronutils.model.field.expression.FieldExpressionFactory.every; import static com.cronutils.model.field.expression.FieldExpressionFactory.on; import static org.junit.jupiter.api.Assertions.*; diff --git a/src/test/java/com/cronutils/Issue459Test.java b/src/test/java/com/cronutils/Issue459Test.java index 8a195123..870b7d82 100644 --- a/src/test/java/com/cronutils/Issue459Test.java +++ b/src/test/java/com/cronutils/Issue459Test.java @@ -5,7 +5,7 @@ import org.junit.jupiter.api.Test; import static com.cronutils.model.CronType.UNIX; -import static com.cronutils.model.field.expression.FieldExpression.always; +import static com.cronutils.model.field.expression.Always.always; import static com.cronutils.model.field.expression.FieldExpressionFactory.on; import static org.junit.jupiter.api.Assertions.assertThrows; diff --git a/src/test/java/com/cronutils/Issue480Test.java b/src/test/java/com/cronutils/Issue480Test.java index 6f90039a..80f7b076 100644 --- a/src/test/java/com/cronutils/Issue480Test.java +++ b/src/test/java/com/cronutils/Issue480Test.java @@ -14,7 +14,7 @@ import java.time.temporal.ChronoUnit; import java.util.Optional; -import static com.cronutils.model.field.expression.FieldExpression.questionMark; +import static com.cronutils.model.field.expression.QuestionMark.questionMark; import static com.cronutils.model.field.expression.FieldExpressionFactory.always; import static com.cronutils.model.field.expression.FieldExpressionFactory.on; import static org.junit.jupiter.api.Assertions.*; diff --git a/src/test/java/com/cronutils/model/field/expression/AlwaysTest.java b/src/test/java/com/cronutils/model/field/expression/AlwaysTest.java index 2604765f..74169fba 100755 --- a/src/test/java/com/cronutils/model/field/expression/AlwaysTest.java +++ b/src/test/java/com/cronutils/model/field/expression/AlwaysTest.java @@ -21,6 +21,6 @@ public class AlwaysTest { @Test public void testAsString() { - assertEquals("*", FieldExpression.always().asString()); + assertEquals("*", Always.always().asString()); } } diff --git a/src/test/java/com/cronutils/model/field/expression/visitor/ValidationFieldExpressionVisitorTest.java b/src/test/java/com/cronutils/model/field/expression/visitor/ValidationFieldExpressionVisitorTest.java index ba1dcd6f..59b2c3bc 100755 --- a/src/test/java/com/cronutils/model/field/expression/visitor/ValidationFieldExpressionVisitorTest.java +++ b/src/test/java/com/cronutils/model/field/expression/visitor/ValidationFieldExpressionVisitorTest.java @@ -57,7 +57,7 @@ public void setUp() { @Test public void testVisitWithInvalidChars() { final ValidationFieldExpressionVisitor tempVisitor = new ValidationFieldExpressionVisitor(fieldConstraints, invalidStringValidations); - final FieldExpression exp = FieldExpression.always(); + final FieldExpression exp = Always.always(); assertThrows(IllegalArgumentException.class, () -> exp.accept(tempVisitor)); } @@ -66,7 +66,7 @@ public void testVisit() { ValidationFieldExpressionVisitor spy = Mockito.spy(visitor); ValidationFieldExpressionVisitor strictSpy = Mockito.spy(strictVisitor); - FieldExpression exp = FieldExpression.always(); + FieldExpression exp = Always.always(); final Always always = (Always) exp; exp.accept(spy); exp.accept(strictSpy); @@ -123,7 +123,7 @@ public void testVisit() { spy = Mockito.spy(visitor); strictSpy = Mockito.spy(strictVisitor); - exp = FieldExpression.questionMark(); + exp = QuestionMark.questionMark(); final QuestionMark qm = (QuestionMark) exp; exp.accept(spy); exp.accept(strictSpy); @@ -146,14 +146,14 @@ public void testVisitBadExp() { @Test public void testVisitAlwaysField() { - final FieldExpression always = FieldExpression.always(); + final FieldExpression always = Always.always(); assertEquals(always, always.accept(strictVisitor)); assertEquals(always, always.accept(visitor)); } @Test public void testVisitQuestionMarkField() { - final FieldExpression qm = FieldExpression.questionMark(); + final FieldExpression qm = QuestionMark.questionMark(); assertEquals(qm, qm.accept(strictVisitor)); assertEquals(qm, qm.accept(visitor)); } diff --git a/src/test/java/com/cronutils/model/field/expression/visitor/ValueMappingFieldExpressionVisitorTest.java b/src/test/java/com/cronutils/model/field/expression/visitor/ValueMappingFieldExpressionVisitorTest.java index 73d05f34..5cfb1381 100755 --- a/src/test/java/com/cronutils/model/field/expression/visitor/ValueMappingFieldExpressionVisitorTest.java +++ b/src/test/java/com/cronutils/model/field/expression/visitor/ValueMappingFieldExpressionVisitorTest.java @@ -33,7 +33,7 @@ public void setUp() { @Test public void testVisitQuestionMark() { - final FieldExpression param = FieldExpression.questionMark(); + final FieldExpression param = QuestionMark.questionMark(); final QuestionMark questionMark = (QuestionMark) param.accept(valueMappingFieldExpressionVisitor); assertTrue(param == questionMark);//always the same cause of singleton pattern } diff --git a/src/test/java/com/cronutils/model/time/generator/AlwaysFieldValueGeneratorTest.java b/src/test/java/com/cronutils/model/time/generator/AlwaysFieldValueGeneratorTest.java index 811dc47e..1b947148 100755 --- a/src/test/java/com/cronutils/model/time/generator/AlwaysFieldValueGeneratorTest.java +++ b/src/test/java/com/cronutils/model/time/generator/AlwaysFieldValueGeneratorTest.java @@ -33,7 +33,7 @@ public class AlwaysFieldValueGeneratorTest { @BeforeEach public void setUp() { fieldValueGenerator = new AlwaysFieldValueGenerator( - new CronField(CronFieldName.HOUR, FieldExpression.always(), FieldConstraintsBuilder.instance().createConstraintsInstance())); + new CronField(CronFieldName.HOUR, Always.always(), FieldConstraintsBuilder.instance().createConstraintsInstance())); } @Test diff --git a/src/test/java/com/cronutils/parser/CronParserNicknameTest.java b/src/test/java/com/cronutils/parser/CronParserNicknameTest.java index 4d0f64da..af407fe0 100644 --- a/src/test/java/com/cronutils/parser/CronParserNicknameTest.java +++ b/src/test/java/com/cronutils/parser/CronParserNicknameTest.java @@ -7,7 +7,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import static com.cronutils.model.field.expression.FieldExpression.always; +import static com.cronutils.model.field.expression.Always.always; import static com.cronutils.model.field.expression.FieldExpressionFactory.on; import static org.junit.jupiter.api.Assertions.assertTrue; diff --git a/src/test/java/com/cronutils/utils/descriptor/CronDescriptorTest.java b/src/test/java/com/cronutils/utils/descriptor/CronDescriptorTest.java index b7c08dac..80beba32 100755 --- a/src/test/java/com/cronutils/utils/descriptor/CronDescriptorTest.java +++ b/src/test/java/com/cronutils/utils/descriptor/CronDescriptorTest.java @@ -20,6 +20,7 @@ import com.cronutils.model.field.CronFieldName; import com.cronutils.model.field.constraint.FieldConstraints; import com.cronutils.model.field.constraint.FieldConstraintsBuilder; +import com.cronutils.model.field.expression.Always; import com.cronutils.model.field.expression.Between; import com.cronutils.model.field.expression.Every; import com.cronutils.model.field.expression.FieldExpression; @@ -107,8 +108,8 @@ public void testDescribeAtXHours() { final int hour = 11; final List results = new ArrayList<>(); results.add(new CronField(CronFieldName.HOUR, new On(new IntegerFieldValue(hour)), nullFieldConstraints)); - results.add(new CronField(CronFieldName.MINUTE, FieldExpression.always(), nullFieldConstraints)); - results.add(new CronField(CronFieldName.SECOND, FieldExpression.always(), nullFieldConstraints)); + results.add(new CronField(CronFieldName.MINUTE, Always.always(), nullFieldConstraints)); + results.add(new CronField(CronFieldName.SECOND, Always.always(), nullFieldConstraints)); assertEquals(String.format("at %s:00", hour), descriptor.describe(new SingleCron(mockDefinition, results))); } @@ -116,9 +117,9 @@ public void testDescribeAtXHours() { public void testEverySecondInMonth() { final int month = 2; final List results = new ArrayList<>(); - results.add(new CronField(CronFieldName.HOUR, FieldExpression.always(), nullFieldConstraints)); - results.add(new CronField(CronFieldName.MINUTE, FieldExpression.always(), nullFieldConstraints)); - results.add(new CronField(CronFieldName.SECOND, FieldExpression.always(), nullFieldConstraints)); + results.add(new CronField(CronFieldName.HOUR, Always.always(), nullFieldConstraints)); + results.add(new CronField(CronFieldName.MINUTE, Always.always(), nullFieldConstraints)); + results.add(new CronField(CronFieldName.SECOND, Always.always(), nullFieldConstraints)); results.add(new CronField(CronFieldName.MONTH, new On(new IntegerFieldValue(month)), nullFieldConstraints)); assertEquals("every second at February month", descriptor.describe(new SingleCron(mockDefinition, results))); } @@ -128,8 +129,8 @@ public void testEveryMinuteBetweenMonths() { final int monthStart = 2; final int monthEnd = 3; final List results = new ArrayList<>(); - results.add(new CronField(CronFieldName.HOUR, FieldExpression.always(), nullFieldConstraints)); - results.add(new CronField(CronFieldName.MINUTE, FieldExpression.always(), nullFieldConstraints)); + results.add(new CronField(CronFieldName.HOUR, Always.always(), nullFieldConstraints)); + results.add(new CronField(CronFieldName.MINUTE, Always.always(), nullFieldConstraints)); results.add(new CronField(CronFieldName.MONTH, new Between(new IntegerFieldValue(monthStart), new IntegerFieldValue(monthEnd)), nullFieldConstraints)); assertEquals("every minute every month between February and March", descriptor.describe(new SingleCron(mockDefinition, results))); } From 5b05dfe1f556849651f7f1258f119e61bf44fa33 Mon Sep 17 00:00:00 2001 From: Roshni Joshi Date: Sat, 25 Nov 2023 19:56:03 -0400 Subject: [PATCH 3/8] refactor: Introduced explaining variable --- src/main/java/com/cronutils/model/CompositeCron.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/cronutils/model/CompositeCron.java b/src/main/java/com/cronutils/model/CompositeCron.java index d6c32fb1..1dacd2ff 100644 --- a/src/main/java/com/cronutils/model/CompositeCron.java +++ b/src/main/java/com/cronutils/model/CompositeCron.java @@ -21,7 +21,8 @@ public CompositeCron(List crons){ this.crons = Collections.unmodifiableList(crons); Preconditions.checkNotNullNorEmpty(crons, "List of Cron cannot be null or empty"); this.definition = crons.get(0).getCronDefinition(); - Preconditions.checkArgument(crons.size()==crons.stream().filter(c->c.getCronDefinition().equals(definition)).count(), "All Cron objects must have same definition for CompositeCron"); + long sameDefCronsCount = crons.stream().filter(c->c.getCronDefinition().equals(definition)).count(); + Preconditions.checkArgument(crons.size()==sameDefCronsCount, "All Cron objects must have same definition for CompositeCron"); } public List getCrons() { From 0d6ba30343546848e602493260e281eff1abc6e1 Mon Sep 17 00:00:00 2001 From: Roshni Joshi Date: Sat, 25 Nov 2023 22:01:08 -0400 Subject: [PATCH 4/8] refactor: applied decompose conditional refactoring --- .../visitor/ValidationFieldExpressionVisitor.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/cronutils/model/field/expression/visitor/ValidationFieldExpressionVisitor.java b/src/main/java/com/cronutils/model/field/expression/visitor/ValidationFieldExpressionVisitor.java index 679f3265..541c1f10 100755 --- a/src/main/java/com/cronutils/model/field/expression/visitor/ValidationFieldExpressionVisitor.java +++ b/src/main/java/com/cronutils/model/field/expression/visitor/ValidationFieldExpressionVisitor.java @@ -66,7 +66,7 @@ public Between visit(final Between between) { this.checkUnsupportedChars(between); preConditions(between); - if ((constraints.isStrictRange()) && between.getFrom() instanceof IntegerFieldValue && between.getTo() instanceof IntegerFieldValue) { + if (isIntegerStrictRange(between)) { final int from = ((IntegerFieldValue) between.getFrom()).getValue(); final int to = ((IntegerFieldValue) between.getTo()).getValue(); if (from > to) { @@ -120,4 +120,10 @@ protected boolean isDefault(final FieldValue fieldValue) { protected boolean isSpecialCharNotL(final FieldValue fieldValue) { return fieldValue instanceof SpecialCharFieldValue && !SpecialChar.L.equals(fieldValue.getValue()); } + + private boolean isIntegerStrictRange(final Between between) { + return constraints.isStrictRange() + && between.getFrom() instanceof IntegerFieldValue + && between.getTo() instanceof IntegerFieldValue; + } } From c15c1bcec01cf08a36bd1029bba41121560ada10 Mon Sep 17 00:00:00 2001 From: Roshni Joshi Date: Sun, 26 Nov 2023 00:40:12 -0400 Subject: [PATCH 5/8] refactor: Renamed variable to more meaningful name --- src/main/java/com/cronutils/parser/CronParser.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/cronutils/parser/CronParser.java b/src/main/java/com/cronutils/parser/CronParser.java index e6052876..e07144f6 100755 --- a/src/main/java/com/cronutils/parser/CronParser.java +++ b/src/main/java/com/cronutils/parser/CronParser.java @@ -93,8 +93,8 @@ private Cron validateAndReturnSupportedCronNickname(String nickname, Set x.endsWith(",")).findAny().orElse(null); if(fieldWithTrailingCommas!=null){ From 22d63f0b87c4abb5ba50b6fc7452f28249baf2b3 Mon Sep 17 00:00:00 2001 From: Roshni Joshi Date: Sun, 26 Nov 2023 09:54:35 -0400 Subject: [PATCH 6/8] refactor: Applied extract method refactoring --- .../java/com/cronutils/parser/CronParser.java | 135 ++++++++++-------- 1 file changed, 78 insertions(+), 57 deletions(-) diff --git a/src/main/java/com/cronutils/parser/CronParser.java b/src/main/java/com/cronutils/parser/CronParser.java index e07144f6..314e3188 100755 --- a/src/main/java/com/cronutils/parser/CronParser.java +++ b/src/main/java/com/cronutils/parser/CronParser.java @@ -98,72 +98,93 @@ public Cron parse(final String expression) { throw new IllegalArgumentException("Empty expression!"); } - Set cronNicknames = cronDefinition.getCronNicknames(); if(expression.startsWith("@")){ - if(cronNicknames.isEmpty()){ - throw new IllegalArgumentException("Nicknames not supported!"); - } - switch (expression){ - case "@yearly": - return validateAndReturnSupportedCronNickname(expression, cronNicknames, CronNicknames.YEARLY, CronBuilder.yearly(cronDefinition)); - case "@annually": - return validateAndReturnSupportedCronNickname(expression, cronNicknames, CronNicknames.ANNUALLY, CronBuilder.annually(cronDefinition)); - case "@monthly": - return validateAndReturnSupportedCronNickname(expression, cronNicknames, CronNicknames.MONTHLY, CronBuilder.monthly(cronDefinition)); - case "@weekly": - return validateAndReturnSupportedCronNickname(expression, cronNicknames, CronNicknames.WEEKLY, CronBuilder.weekly(cronDefinition)); - case "@daily": - return validateAndReturnSupportedCronNickname(expression, cronNicknames, CronNicknames.DAILY, CronBuilder.daily(cronDefinition)); - case "@midnight": - return validateAndReturnSupportedCronNickname(expression, cronNicknames, CronNicknames.MIDNIGHT, CronBuilder.midnight(cronDefinition)); - case "@hourly": - return validateAndReturnSupportedCronNickname(expression, cronNicknames, CronNicknames.HOURLY, CronBuilder.hourly(cronDefinition)); - case "@reboot": - return validateAndReturnSupportedCronNickname(expression, cronNicknames, CronNicknames.REBOOT, CronBuilder.reboot(cronDefinition)); - } + Cron cron = parseNicknameExpression(expression); + if(cron != null) { + return cron; + } } if(expression.contains("||")) { - List crons = Arrays.stream(expression.split("\\|\\|")).map(this::parse).collect(Collectors.toList()); - return new CompositeCron(crons); + return parseCompositeExpression(expression); } if(expression.contains("|")){ - List crons = new ArrayList<>(); - int cronscount = Arrays.stream(expression.split("\\s+")).mapToInt(s->s.split("\\|").length).max().orElse(0); - for(int j=0; j x.endsWith(",")).findAny().orElse(null); - if(fieldWithTrailingCommas!=null){ - throw new IllegalArgumentException(String.format("Invalid field value! Trailing commas not permitted! '%s'", fieldWithTrailingCommas)); - } - final List fields = expressions.get(expressionLength); - if (fields == null) { - throw new IllegalArgumentException( - String.format("Cron expression contains %s parts but we expect one of %s", expressionLength, expressions.keySet())); - } - try { - final int size = expressionParts.length; - final List results = new ArrayList<>(size + 1); - for (int j = 0; j < size; j++) { - results.add(fields.get(j).parse(expressionParts[j])); + return parseSingleExpression(noExtraSpaceExpression); + } + } + + private Cron parseNicknameExpression(final String expression) { + Set cronNicknames = cronDefinition.getCronNicknames(); + if(cronNicknames.isEmpty()){ + throw new IllegalArgumentException("Nicknames not supported!"); + } + switch (expression){ + case "@yearly": + return validateAndReturnSupportedCronNickname(expression, cronNicknames, CronNicknames.YEARLY, CronBuilder.yearly(cronDefinition)); + case "@annually": + return validateAndReturnSupportedCronNickname(expression, cronNicknames, CronNicknames.ANNUALLY, CronBuilder.annually(cronDefinition)); + case "@monthly": + return validateAndReturnSupportedCronNickname(expression, cronNicknames, CronNicknames.MONTHLY, CronBuilder.monthly(cronDefinition)); + case "@weekly": + return validateAndReturnSupportedCronNickname(expression, cronNicknames, CronNicknames.WEEKLY, CronBuilder.weekly(cronDefinition)); + case "@daily": + return validateAndReturnSupportedCronNickname(expression, cronNicknames, CronNicknames.DAILY, CronBuilder.daily(cronDefinition)); + case "@midnight": + return validateAndReturnSupportedCronNickname(expression, cronNicknames, CronNicknames.MIDNIGHT, CronBuilder.midnight(cronDefinition)); + case "@hourly": + return validateAndReturnSupportedCronNickname(expression, cronNicknames, CronNicknames.HOURLY, CronBuilder.hourly(cronDefinition)); + case "@reboot": + return validateAndReturnSupportedCronNickname(expression, cronNicknames, CronNicknames.REBOOT, CronBuilder.reboot(cronDefinition)); + default: + return null; + } + } + + private Cron parseCompositeExpression(final String expression) { + List crons = Arrays.stream(expression.split("\\|\\|")).map(this::parse).collect(Collectors.toList()); + return new CompositeCron(crons); + } + + private Cron parseMultipleExpression(final String expression) { + List crons = new ArrayList<>(); + int cronscount = Arrays.stream(expression.split("\\s+")).mapToInt(s->s.split("\\|").length).max().orElse(0); + for(int j=0; j x.endsWith(",")).findAny().orElse(null); + if(fieldWithTrailingCommas!=null){ + throw new IllegalArgumentException(String.format("Invalid field value! Trailing commas not permitted! '%s'", fieldWithTrailingCommas)); + } + final List fields = expressions.get(expressionLength); + if (fields == null) { + throw new IllegalArgumentException( + String.format("Cron expression contains %s parts but we expect one of %s", expressionLength, expressions.keySet())); + } + try { + final int size = expressionParts.length; + final List results = new ArrayList<>(size + 1); + for (int j = 0; j < size; j++) { + results.add(fields.get(j).parse(expressionParts[j])); + } + return new SingleCron(cronDefinition, results).validate(); + } catch (final IllegalArgumentException e) { + throw new IllegalArgumentException(String.format("Failed to parse cron expression. %s", e.getMessage()), e); } } } From 7aac7e082464815d828e5821794d703208e91785 Mon Sep 17 00:00:00 2001 From: Roshni Joshi Date: Mon, 27 Nov 2023 19:58:54 -0400 Subject: [PATCH 7/8] refactor: Applied extract class refactoring to move all generate days methods to separate class --- .../cronutils/model/time/DaysGenerator.java | 197 ++++++++++++++++++ .../model/time/SingleExecutionTime.java | 180 +--------------- 2 files changed, 207 insertions(+), 170 deletions(-) create mode 100644 src/main/java/com/cronutils/model/time/DaysGenerator.java diff --git a/src/main/java/com/cronutils/model/time/DaysGenerator.java b/src/main/java/com/cronutils/model/time/DaysGenerator.java new file mode 100644 index 00000000..9b85ec6e --- /dev/null +++ b/src/main/java/com/cronutils/model/time/DaysGenerator.java @@ -0,0 +1,197 @@ +package com.cronutils.model.time; + +import static com.cronutils.model.field.CronFieldName.DAY_OF_MONTH; +import static com.cronutils.model.field.CronFieldName.DAY_OF_WEEK; +import static com.cronutils.model.field.CronFieldName.DAY_OF_YEAR; +import static com.cronutils.model.field.value.SpecialChar.QUESTION_MARK; +import static com.cronutils.model.time.generator.FieldValueGeneratorFactory.createDayOfMonthValueGeneratorInstance; +import static com.cronutils.model.time.generator.FieldValueGeneratorFactory.createDayOfWeekValueGeneratorInstance; +import static com.cronutils.model.time.generator.FieldValueGeneratorFactory.createDayOfYearValueGeneratorInstance; +import static com.cronutils.utils.Predicates.not; + +import java.time.LocalDate; +import java.time.ZonedDateTime; +import java.util.HashSet; +import java.util.List; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import com.cronutils.mapper.WeekDay; +import com.cronutils.model.definition.CronDefinition; +import com.cronutils.model.field.CronField; +import com.cronutils.model.field.definition.DayOfWeekFieldDefinition; +import com.cronutils.model.field.expression.Always; +import com.cronutils.model.field.expression.QuestionMark; + +/* + * Generate days for execution time + */ +class DaysGenerator { + + private final CronField daysOfWeekCronField; + private final CronField daysOfMonthCronField; + private final CronField daysOfYearCronField; + private final CronDefinition cronDefinition; + + DaysGenerator(CronField daysOfWeekCronField, CronField daysOfMonthCronField, CronField daysOfYearCronField, + CronDefinition cronDefinition) { + super(); + this.daysOfWeekCronField = daysOfWeekCronField; + this.daysOfMonthCronField = daysOfMonthCronField; + this.daysOfYearCronField = daysOfYearCronField; + this.cronDefinition = cronDefinition; + } + + Optional generateDays(final CronDefinition cronDefinition, final ZonedDateTime date) { + if (isGenerateDaysAsDoY(cronDefinition)) { + return generateDayCandidatesUsingDoY(date); + } + //If DoW is not supported in custom definition, we just return an empty list. + if (cronDefinition.getFieldDefinition(DAY_OF_WEEK) != null && cronDefinition.getFieldDefinition(DAY_OF_MONTH) != null) { + return generateDaysDoWAndDoMSupported(cronDefinition, date); + } + if (cronDefinition.getFieldDefinition(DAY_OF_WEEK) == null) { + return generateDayCandidatesUsingDoM(date); + } + return Optional + .of(generateDayCandidatesUsingDoW(date, ((DayOfWeekFieldDefinition) cronDefinition.getFieldDefinition(DAY_OF_WEEK)).getMondayDoWValue())); + } + + private boolean isGenerateDaysAsDoY(final CronDefinition cronDefinition) { + if (!cronDefinition.containsFieldDefinition(DAY_OF_YEAR)) { + return false; + } + + if (!cronDefinition.getFieldDefinition(DAY_OF_YEAR).getConstraints().getSpecialChars().contains(QUESTION_MARK)) { + return true; + } + + return !(daysOfYearCronField.getExpression() instanceof QuestionMark); + } + + private Optional generateDayCandidatesUsingDoY(final ZonedDateTime reference) { + final int year = reference.getYear(); + final int month = reference.getMonthValue(); + final LocalDate date = LocalDate.of(year, 1, 1); + final int lengthOfYear = date.lengthOfYear(); + + final List candidates = createDayOfYearValueGeneratorInstance(daysOfYearCronField, year).generateCandidates(1, lengthOfYear); + + final int low = LocalDate.of(year, month, 1).getDayOfYear(); + final int high = month == 12 + ? LocalDate.of(year, 12, 31).getDayOfYear() + 1 + : LocalDate.of(year, month + 1, 1).getDayOfYear(); + + final List collectedCandidates = candidates.stream().filter(dayOfYear -> dayOfYear >= low && dayOfYear < high) + .map(dayOfYear -> LocalDate.ofYearDay(reference.getYear(), dayOfYear).getDayOfMonth()) + .collect(Collectors.toList()); + + return Optional.of(collectedCandidates).filter(not(List::isEmpty)).map(TimeNode::new); + } + + private Optional generateDaysDoWAndDoMSupported(final CronDefinition cronDefinition, final ZonedDateTime date) { + final boolean questionMarkSupported = cronDefinition.getFieldDefinition(DAY_OF_WEEK).getConstraints().getSpecialChars().contains(QUESTION_MARK); + if (questionMarkSupported) { + final List candidates = generateDayCandidatesQuestionMarkSupportedUsingDoWAndDoM( + date.getYear(), + date.getMonthValue(), + ((DayOfWeekFieldDefinition) cronDefinition.getFieldDefinition(DAY_OF_WEEK)).getMondayDoWValue() + ); + return Optional.of(candidates).filter(not(List::isEmpty)).map(TimeNode::new); + } else { + final List candidates = generateDayCandidatesQuestionMarkNotSupportedUsingDoWAndDoM( + date.getYear(), date.getMonthValue(), + ((DayOfWeekFieldDefinition) + cronDefinition.getFieldDefinition(DAY_OF_WEEK) + ).getMondayDoWValue() + ); + return Optional.of(candidates).filter(not(List::isEmpty)).map(TimeNode::new); + } + } + + private List generateDayCandidatesQuestionMarkNotSupportedUsingDoWAndDoM(final int year, final int month, final WeekDay mondayDoWValue) { + final LocalDate date = LocalDate.of(year, month, 1); + final int lengthOfMonth = date.lengthOfMonth(); + if (daysOfMonthCronField.getExpression() instanceof Always && daysOfWeekCronField.getExpression() instanceof Always) { + return createDayOfMonthValueGeneratorInstance(daysOfMonthCronField, year, month) + .generateCandidates(1, lengthOfMonth) + .stream().distinct().sorted() + .collect(Collectors.toList()); + } else if (daysOfMonthCronField.getExpression() instanceof Always) { + return createDayOfWeekValueGeneratorInstance(daysOfWeekCronField, year, month, mondayDoWValue) + .generateCandidates(1, lengthOfMonth) + .stream().distinct().sorted() + .collect(Collectors.toList()); + } else if (daysOfWeekCronField.getExpression() instanceof Always) { + return createDayOfMonthValueGeneratorInstance(daysOfMonthCronField, year, month) + .generateCandidates(1, lengthOfMonth) + .stream().distinct().sorted() + .collect(Collectors.toList()); + } else { + final List dayOfWeekCandidates = createDayOfWeekValueGeneratorInstance(daysOfWeekCronField, + year, month, mondayDoWValue).generateCandidates(1, lengthOfMonth); + final List dayOfMonthCandidates = createDayOfMonthValueGeneratorInstance(daysOfMonthCronField, year, month) + .generateCandidates(1, lengthOfMonth); + if (cronDefinition.isMatchDayOfWeekAndDayOfMonth()) { + final Set intersection = new HashSet<>(dayOfWeekCandidates); + return dayOfMonthCandidates + .stream().filter(intersection::contains) + .distinct().sorted() + .collect(Collectors.toList()); + } else { + return Stream.concat(dayOfWeekCandidates.stream(), dayOfMonthCandidates.stream()) + .distinct().sorted() + .collect(Collectors.toList()); + } + } + } + + private List generateDayCandidatesQuestionMarkSupportedUsingDoWAndDoM(final int year, final int month, final WeekDay mondayDoWValue) { + final LocalDate date = LocalDate.of(year, month, 1); + final int lengthOfMonth = date.lengthOfMonth(); + if (daysOfMonthCronField.getExpression() instanceof Always && daysOfWeekCronField.getExpression() instanceof Always) { + return createDayOfMonthValueGeneratorInstance(daysOfMonthCronField, year, month) + .generateCandidates(1, lengthOfMonth) + .stream().distinct().sorted() + .collect(Collectors.toList()); + } else if (daysOfMonthCronField.getExpression() instanceof QuestionMark) { + return createDayOfWeekValueGeneratorInstance(daysOfWeekCronField, year, month, mondayDoWValue) + .generateCandidates(1, lengthOfMonth) + .stream().distinct().sorted() + .collect(Collectors.toList()); + } else if (daysOfWeekCronField.getExpression() instanceof QuestionMark) { + return createDayOfMonthValueGeneratorInstance(daysOfMonthCronField, year, month) + .generateCandidates(1, lengthOfMonth) + .stream().distinct().sorted() + .collect(Collectors.toList()); + } else { + Set candidates = new HashSet<>(createDayOfMonthValueGeneratorInstance(daysOfMonthCronField, year, month).generateCandidates(1, lengthOfMonth)); + Set daysOfWeek = new HashSet<>(createDayOfWeekValueGeneratorInstance(daysOfWeekCronField, year, month, mondayDoWValue).generateCandidates(1, lengthOfMonth)); + // Only the intersection of valid days from the days of week and valid days of month should be returned. + candidates.retainAll(daysOfWeek); + return candidates.stream().sorted().collect(Collectors.toList()); + } + } + + private Optional generateDayCandidatesUsingDoM(final ZonedDateTime reference) { + final LocalDate date = LocalDate.of(reference.getYear(), reference.getMonthValue(), 1); + final int lengthOfMonth = date.lengthOfMonth(); + final List candidates = createDayOfMonthValueGeneratorInstance(daysOfMonthCronField, reference.getYear(), reference.getMonthValue()) + .generateCandidates(1, lengthOfMonth) + .stream().distinct().sorted() + .collect(Collectors.toList()); + return candidates.isEmpty() ? Optional.empty() : Optional.of(new TimeNode(candidates)); + } + + private TimeNode generateDayCandidatesUsingDoW(final ZonedDateTime reference, final WeekDay mondayDoWValue) { + final LocalDate date = LocalDate.of(reference.getYear(), reference.getMonthValue(), 1); + final int lengthOfMonth = date.lengthOfMonth(); + final List candidates = createDayOfWeekValueGeneratorInstance(daysOfWeekCronField, reference.getYear(), reference.getMonthValue(), mondayDoWValue) + .generateCandidates(1, lengthOfMonth) + .stream().distinct().sorted() + .collect(Collectors.toList()); + return new TimeNode(candidates); + } +} diff --git a/src/main/java/com/cronutils/model/time/SingleExecutionTime.java b/src/main/java/com/cronutils/model/time/SingleExecutionTime.java index c10c7312..12446231 100755 --- a/src/main/java/com/cronutils/model/time/SingleExecutionTime.java +++ b/src/main/java/com/cronutils/model/time/SingleExecutionTime.java @@ -13,14 +13,11 @@ package com.cronutils.model.time; -import com.cronutils.mapper.WeekDay; import com.cronutils.model.definition.CronDefinition; import com.cronutils.model.field.CronField; import com.cronutils.model.field.CronFieldName; import com.cronutils.model.field.constraint.FieldConstraintsBuilder; -import com.cronutils.model.field.definition.DayOfWeekFieldDefinition; import com.cronutils.model.field.expression.Always; -import com.cronutils.model.field.expression.QuestionMark; import com.cronutils.model.time.generator.FieldValueGenerator; import com.cronutils.model.time.generator.FieldValueGeneratorFactory; import com.cronutils.model.time.generator.NoSuchValueException; @@ -37,12 +34,9 @@ import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; -import java.util.stream.Stream; import static com.cronutils.model.field.CronFieldName.*; -import static com.cronutils.model.field.value.SpecialChar.QUESTION_MARK; import static com.cronutils.model.time.generator.FieldValueGeneratorFactory.*; -import static com.cronutils.utils.Predicates.not; import static java.time.temporal.ChronoUnit.DAYS; import static java.time.temporal.ChronoUnit.SECONDS; import static java.time.temporal.TemporalAdjusters.lastDayOfMonth; @@ -57,10 +51,7 @@ public class SingleExecutionTime implements ExecutionTime { private final CronDefinition cronDefinition; private final FieldValueGenerator yearsValueGenerator; - private final CronField daysOfWeekCronField; - private final CronField daysOfMonthCronField; - private final CronField daysOfYearCronField; - + private final DaysGenerator daysGenerator; private final TimeNode months; private final TimeNode hours; private final TimeNode minutes; @@ -83,9 +74,9 @@ public class SingleExecutionTime implements ExecutionTime { this.yearsValueGenerator = FieldValueGeneratorFactory.forCronField(new CronField(CronFieldName.YEAR, Always.always(), FieldConstraintsBuilder.instance().createConstraintsInstance())); } - this.daysOfWeekCronField = Preconditions.checkNotNull(daysOfWeekCronField); - this.daysOfMonthCronField = Preconditions.checkNotNull(daysOfMonthCronField); - this.daysOfYearCronField = daysOfYearCronField; + Preconditions.checkNotNull(daysOfWeekCronField); + Preconditions.checkNotNull(daysOfMonthCronField); + this.daysGenerator = new DaysGenerator(daysOfWeekCronField, daysOfMonthCronField, daysOfYearCronField, cronDefinition); this.months = Preconditions.checkNotNull(months); this.hours = Preconditions.checkNotNull(hours); this.minutes = Preconditions.checkNotNull(minutes); @@ -167,7 +158,7 @@ private ExecutionTimeResult potentialNextClosestMatch(final ZonedDateTime date) return getNextPotentialMonth(date, lowestHour, lowestMinute, lowestSecond); } - final Optional optionalDays = generateDays(cronDefinition, date); + final Optional optionalDays = daysGenerator.generateDays(cronDefinition, date); if (!optionalDays.isPresent()) { return new ExecutionTimeResult(toBeginOfNextMonth(date), false); } @@ -196,7 +187,7 @@ private ExecutionTimeResult getNextPotentialYear(final ZonedDateTime date, final int lowestSecond) throws NoSuchValueException { final int newYear = yearsValueGenerator.generateNextValue(date.getYear()); - final Optional optionalDays = generateDays(cronDefinition, ZonedDateTime.of( + final Optional optionalDays = daysGenerator.generateDays(cronDefinition, ZonedDateTime.of( LocalDate.of(newYear, lowestMonth, 1), LocalTime.MIN, date.getZone()) @@ -218,7 +209,7 @@ private ExecutionTimeResult getNextPotentialMonth(final ZonedDateTime date, fina if (nearestValue.getShifts() > 0) { return new ExecutionTimeResult(date.truncatedTo(DAYS).withMonth(1).withDayOfMonth(1).plusYears(nearestValue.getShifts()), false); } - final Optional optionalDays = generateDays(cronDefinition, + final Optional optionalDays = daysGenerator.generateDays(cronDefinition, ZonedDateTime.of(LocalDateTime.of(date.getYear(), nextMonths, 1, 0, 0), date.getZone())); if (optionalDays.isPresent()) { final List days = optionalDays.get().getValues(); @@ -309,7 +300,7 @@ private ExecutionTimeResult potentialPreviousClosestMatch(final ZonedDateTime da //int startyear = cronDefinition.getFieldDefinition(CronFieldName.YEAR).getConstraints().getStartRange(); //final List year = yearsValueGenerator.generateCandidates(startyear, date.getYear()); final List year = yearsValueGenerator.generateCandidates(date.getYear(), date.getYear()); - final Optional optionalDays = generateDays(cronDefinition, date); + final Optional optionalDays = daysGenerator.generateDays(cronDefinition, date); TimeNode days; if (optionalDays.isPresent() && optionalDays.get().getValues().stream().anyMatch(i -> i <= date.getDayOfMonth())) { days = optionalDays.get(); @@ -467,74 +458,7 @@ private ZonedDateTime toEndOfPreviousMonth(final ZonedDateTime datetime) { .of(previousMonth.getYear(), previousMonth.getMonth().getValue(), previousMonth.getDayOfMonth(), highestHour, highestMinute, highestSecond, 0, previousMonth.getZone()); } - - private Optional generateDays(final CronDefinition cronDefinition, final ZonedDateTime date) { - if (isGenerateDaysAsDoY(cronDefinition)) { - return generateDayCandidatesUsingDoY(date); - } - //If DoW is not supported in custom definition, we just return an empty list. - if (cronDefinition.getFieldDefinition(DAY_OF_WEEK) != null && cronDefinition.getFieldDefinition(DAY_OF_MONTH) != null) { - return generateDaysDoWAndDoMSupported(cronDefinition, date); - } - if (cronDefinition.getFieldDefinition(DAY_OF_WEEK) == null) { - return generateDayCandidatesUsingDoM(date); - } - return Optional - .of(generateDayCandidatesUsingDoW(date, ((DayOfWeekFieldDefinition) cronDefinition.getFieldDefinition(DAY_OF_WEEK)).getMondayDoWValue())); - } - - private boolean isGenerateDaysAsDoY(final CronDefinition cronDefinition) { - if (!cronDefinition.containsFieldDefinition(DAY_OF_YEAR)) { - return false; - } - - if (!cronDefinition.getFieldDefinition(DAY_OF_YEAR).getConstraints().getSpecialChars().contains(QUESTION_MARK)) { - return true; - } - - return !(daysOfYearCronField.getExpression() instanceof QuestionMark); - } - - private Optional generateDayCandidatesUsingDoY(final ZonedDateTime reference) { - final int year = reference.getYear(); - final int month = reference.getMonthValue(); - final LocalDate date = LocalDate.of(year, 1, 1); - final int lengthOfYear = date.lengthOfYear(); - - final List candidates = createDayOfYearValueGeneratorInstance(daysOfYearCronField, year).generateCandidates(1, lengthOfYear); - - final int low = LocalDate.of(year, month, 1).getDayOfYear(); - final int high = month == 12 - ? LocalDate.of(year, 12, 31).getDayOfYear() + 1 - : LocalDate.of(year, month + 1, 1).getDayOfYear(); - - final List collectedCandidates = candidates.stream().filter(dayOfYear -> dayOfYear >= low && dayOfYear < high) - .map(dayOfYear -> LocalDate.ofYearDay(reference.getYear(), dayOfYear).getDayOfMonth()) - .collect(Collectors.toList()); - - return Optional.of(collectedCandidates).filter(not(List::isEmpty)).map(TimeNode::new); - } - - private Optional generateDaysDoWAndDoMSupported(final CronDefinition cronDefinition, final ZonedDateTime date) { - final boolean questionMarkSupported = cronDefinition.getFieldDefinition(DAY_OF_WEEK).getConstraints().getSpecialChars().contains(QUESTION_MARK); - if (questionMarkSupported) { - final List candidates = generateDayCandidatesQuestionMarkSupportedUsingDoWAndDoM( - date.getYear(), - date.getMonthValue(), - ((DayOfWeekFieldDefinition) cronDefinition.getFieldDefinition(DAY_OF_WEEK)).getMondayDoWValue() - ); - return Optional.of(candidates).filter(not(List::isEmpty)).map(TimeNode::new); - } else { - final List candidates = generateDayCandidatesQuestionMarkNotSupportedUsingDoWAndDoM( - date.getYear(), date.getMonthValue(), - ((DayOfWeekFieldDefinition) - cronDefinition.getFieldDefinition(DAY_OF_WEEK) - ).getMondayDoWValue() - ); - return Optional.of(candidates).filter(not(List::isEmpty)).map(TimeNode::new); - } - } - + /** * Provide nearest time for next execution. * @@ -645,91 +569,7 @@ private boolean dateValuesInExpectedRanges(final ZonedDateTime validCronDate, fi } return everythingInRange; } - - private List generateDayCandidatesQuestionMarkNotSupportedUsingDoWAndDoM(final int year, final int month, final WeekDay mondayDoWValue) { - final LocalDate date = LocalDate.of(year, month, 1); - final int lengthOfMonth = date.lengthOfMonth(); - if (daysOfMonthCronField.getExpression() instanceof Always && daysOfWeekCronField.getExpression() instanceof Always) { - return createDayOfMonthValueGeneratorInstance(daysOfMonthCronField, year, month) - .generateCandidates(1, lengthOfMonth) - .stream().distinct().sorted() - .collect(Collectors.toList()); - } else if (daysOfMonthCronField.getExpression() instanceof Always) { - return createDayOfWeekValueGeneratorInstance(daysOfWeekCronField, year, month, mondayDoWValue) - .generateCandidates(1, lengthOfMonth) - .stream().distinct().sorted() - .collect(Collectors.toList()); - } else if (daysOfWeekCronField.getExpression() instanceof Always) { - return createDayOfMonthValueGeneratorInstance(daysOfMonthCronField, year, month) - .generateCandidates(1, lengthOfMonth) - .stream().distinct().sorted() - .collect(Collectors.toList()); - } else { - final List dayOfWeekCandidates = createDayOfWeekValueGeneratorInstance(daysOfWeekCronField, - year, month, mondayDoWValue).generateCandidates(1, lengthOfMonth); - final List dayOfMonthCandidates = createDayOfMonthValueGeneratorInstance(daysOfMonthCronField, year, month) - .generateCandidates(1, lengthOfMonth); - if (cronDefinition.isMatchDayOfWeekAndDayOfMonth()) { - final Set intersection = new HashSet<>(dayOfWeekCandidates); - return dayOfMonthCandidates - .stream().filter(intersection::contains) - .distinct().sorted() - .collect(Collectors.toList()); - } else { - return Stream.concat(dayOfWeekCandidates.stream(), dayOfMonthCandidates.stream()) - .distinct().sorted() - .collect(Collectors.toList()); - } - } - } - - private List generateDayCandidatesQuestionMarkSupportedUsingDoWAndDoM(final int year, final int month, final WeekDay mondayDoWValue) { - final LocalDate date = LocalDate.of(year, month, 1); - final int lengthOfMonth = date.lengthOfMonth(); - if (daysOfMonthCronField.getExpression() instanceof Always && daysOfWeekCronField.getExpression() instanceof Always) { - return createDayOfMonthValueGeneratorInstance(daysOfMonthCronField, year, month) - .generateCandidates(1, lengthOfMonth) - .stream().distinct().sorted() - .collect(Collectors.toList()); - } else if (daysOfMonthCronField.getExpression() instanceof QuestionMark) { - return createDayOfWeekValueGeneratorInstance(daysOfWeekCronField, year, month, mondayDoWValue) - .generateCandidates(1, lengthOfMonth) - .stream().distinct().sorted() - .collect(Collectors.toList()); - } else if (daysOfWeekCronField.getExpression() instanceof QuestionMark) { - return createDayOfMonthValueGeneratorInstance(daysOfMonthCronField, year, month) - .generateCandidates(1, lengthOfMonth) - .stream().distinct().sorted() - .collect(Collectors.toList()); - } else { - Set candidates = new HashSet<>(createDayOfMonthValueGeneratorInstance(daysOfMonthCronField, year, month).generateCandidates(1, lengthOfMonth)); - Set daysOfWeek = new HashSet<>(createDayOfWeekValueGeneratorInstance(daysOfWeekCronField, year, month, mondayDoWValue).generateCandidates(1, lengthOfMonth)); - // Only the intersection of valid days from the days of week and valid days of month should be returned. - candidates.retainAll(daysOfWeek); - return candidates.stream().sorted().collect(Collectors.toList()); - } - } - - private Optional generateDayCandidatesUsingDoM(final ZonedDateTime reference) { - final LocalDate date = LocalDate.of(reference.getYear(), reference.getMonthValue(), 1); - final int lengthOfMonth = date.lengthOfMonth(); - final List candidates = createDayOfMonthValueGeneratorInstance(daysOfMonthCronField, reference.getYear(), reference.getMonthValue()) - .generateCandidates(1, lengthOfMonth) - .stream().distinct().sorted() - .collect(Collectors.toList()); - return candidates.isEmpty() ? Optional.empty() : Optional.of(new TimeNode(candidates)); - } - - private TimeNode generateDayCandidatesUsingDoW(final ZonedDateTime reference, final WeekDay mondayDoWValue) { - final LocalDate date = LocalDate.of(reference.getYear(), reference.getMonthValue(), 1); - final int lengthOfMonth = date.lengthOfMonth(); - final List candidates = createDayOfWeekValueGeneratorInstance(daysOfWeekCronField, reference.getYear(), reference.getMonthValue(), mondayDoWValue) - .generateCandidates(1, lengthOfMonth) - .stream().distinct().sorted() - .collect(Collectors.toList()); - return new TimeNode(candidates); - } - + private static final class ExecutionTimeResult { private final ZonedDateTime time; private final boolean isMatch; From 4dba1b39a5cca1a25e0107fdbce038fcb4998707 Mon Sep 17 00:00:00 2001 From: Roshni Joshi Date: Mon, 27 Nov 2023 20:13:11 -0400 Subject: [PATCH 8/8] refactor: Added required comments for CronParser --- .../java/com/cronutils/parser/CronParser.java | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/main/java/com/cronutils/parser/CronParser.java b/src/main/java/com/cronutils/parser/CronParser.java index 314e3188..ce05d4aa 100755 --- a/src/main/java/com/cronutils/parser/CronParser.java +++ b/src/main/java/com/cronutils/parser/CronParser.java @@ -115,6 +115,12 @@ public Cron parse(final String expression) { } } + /** + * Parse string with nickname cron expressions + * @param expression cron expression, never null + * @return Cron instance + * @throws java.lang.IllegalArgumentException if expression does not match cron definition + */ private Cron parseNicknameExpression(final String expression) { Set cronNicknames = cronDefinition.getCronNicknames(); if(cronNicknames.isEmpty()){ @@ -142,11 +148,23 @@ private Cron parseNicknameExpression(final String expression) { } } + /** + * Parse string with composite cron expressions + * @param expression cron expression, never null + * @return Cron instance + * @throws java.lang.IllegalArgumentException if expression does not match cron definition + */ private Cron parseCompositeExpression(final String expression) { List crons = Arrays.stream(expression.split("\\|\\|")).map(this::parse).collect(Collectors.toList()); return new CompositeCron(crons); } + /** + * Parse string with multiple cron expressions + * @param expression cron expression, never null + * @return Cron instance + * @throws java.lang.IllegalArgumentException if expression does not match cron definition + */ private Cron parseMultipleExpression(final String expression) { List crons = new ArrayList<>(); int cronscount = Arrays.stream(expression.split("\\s+")).mapToInt(s->s.split("\\|").length).max().orElse(0); @@ -164,6 +182,12 @@ private Cron parseMultipleExpression(final String expression) { return new CompositeCron(crons.stream().map(this::parse).collect(Collectors.toList())); } + /** + * Parse string with single cron expression + * @param noExtraSpaceExpression cleaned cron expression, never null + * @return Cron instance + * @throws java.lang.IllegalArgumentException if expression does not match cron definition + */ private Cron parseSingleExpression(final String noExtraSpaceExpression) { final String[] expressionParts = noExtraSpaceExpression.toUpperCase().split(" "); final int expressionLength = expressionParts.length;