Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
refactor: Resolved Feature Envy design smell
  • Loading branch information
Roshni Joshi committed Nov 21, 2023
commit f8ae51b0e7110e58528e01c0a32252a5222ea23a
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -66,6 +69,22 @@ public int getEndRange() {
public Set<SpecialChar> 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.
Expand All @@ -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.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -84,18 +82,18 @@ 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;
}

@Override
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;
}
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -32,6 +35,7 @@ public class FieldConstraintsTest {
private int startRange;
private int endRange;
private boolean strictRange;
private FieldConstraints fieldConstraints;

@BeforeEach
public void setUp() {
Expand All @@ -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
Expand All @@ -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));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}

}