gitgabrio commented on code in PR #6299:
URL:
https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2039513888
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/DateAndTimeFunction.java:
##########
@@ -122,33 +126,33 @@ public FEELFnResult<TemporalAccessor>
invoke(@ParameterName( "year" ) Number yea
public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "year" )
Number year, @ParameterName( "month" ) Number month, @ParameterName( "day" )
Number day,
@ParameterName( "hour" )
Number hour, @ParameterName( "minute" ) Number minute, @ParameterName( "second"
) Number second,
@ParameterName( "hour offset"
) Number hourOffset ) {
- if ( year == null ) {
- return FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "year", "cannot be null"));
- }
- if ( month == null ) {
- return FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "month", "cannot be null"));
- }
- if ( day == null ) {
- return FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "day", "cannot be null"));
- }
- if ( hour == null ) {
- return FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "hour", "cannot be null"));
- }
- if ( minute == null ) {
- return FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "minute", "cannot be null"));
- }
- if ( second == null ) {
- return FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "second", "cannot be null"));
- }
-
try {
+ Optional<Integer> coercedYear = coerceIntegerNumber(year);
Review Comment:
Hi @ChinchuAjith
Code could be rewritten with a more idiomatic usage of Optional
```java
try {
int coercedYear = coerceIntegerNumber(year).orElseThrow();
int coercedMonth = coerceIntegerNumber(month).orElseThrow();
int coercedDay = coerceIntegerNumber(day).orElseThrow();
int coercedHour = coerceIntegerNumber(hour).orElseThrow();
int coercedMinute = coerceIntegerNumber(minute).orElseThrow();
int coercedSecond = coerceIntegerNumber(second).orElseThrow();
if (hourOffset != null) {
Optional<Integer> coercedHourOffset =
coerceIntegerNumber(hourOffset);
return
coercedHourOffset.<FEELFnResult<TemporalAccessor>>map(integer ->
FEELFnResult.ofResult(
OffsetDateTime.of(
coercedYear, coercedMonth, coercedDay,
coercedHour, coercedMinute, coercedSecond,
0,
ZoneOffset.ofHours(integer)))).orElseGet(() -> FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "hour offset", "could not be coerced to
Integer")));
} else {
return FEELFnResult.ofResult(
LocalDateTime.of(
coercedYear, coercedMonth, coercedDay,
coercedHour, coercedMinute, coercedSecond)
);
}
} catch (NoSuchElementException e) { // thrown by
Optional.orElseThrow()
return FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "coercion", "One or more input values
could not be coerced to Integer: either null or not a valid Number."));
} catch (DateTimeException e) {
return FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "input parameters date-parsing
exception", e));
}
```
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/util/NumberEvalHelper.java:
##########
@@ -66,12 +67,47 @@ public static BigDecimal getBigDecimalOrNull(Object value) {
return null;
}
+ /**
+ * The method handles various numeric types and converts the given object
to an Integer
+ * Returns null If the conversion is not possible.
+ *
+ * @param value : The object to be converted.
+ * @return : An Integer representation of the value, or null if conversion
is not possible.
Review Comment:
Thanks @ChinchuAjith
IINW, this method is used only once, and inside this very class.
Could you please:
1. remove public accessor
2. move it to bottom of class (readability)
3. refactor it to return an Optional<Integer> (let's reduce the
proliferation of `null`s 😄 )
4. unit test it
5. update javadoc
Thanks!
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/util/NumberEvalHelper.java:
##########
@@ -66,12 +67,47 @@ public static BigDecimal getBigDecimalOrNull(Object value) {
return null;
}
+ /**
+ * The method handles various numeric types and converts the given object
to an Integer
+ * Returns null If the conversion is not possible.
+ *
+ * @param value : The object to be converted.
+ * @return : An Integer representation of the value, or null if conversion
is not possible.
+ */
+ public static Integer getIntegerOrNull(Object value) {
+ if ( value instanceof BigDecimal ) {
+ return ((BigDecimal) value).intValue();
+ }
+ if ( value instanceof BigInteger ) {
+ return ((BigInteger) value).intValue();
+ }
+ if ( value instanceof Number ) {
+ return ((Number) value).intValue();
+ }
+ return null;
+ }
+
public static Object coerceNumber(Object value) {
if ( value instanceof Number && !(value instanceof BigDecimal) ) {
return getBigDecimalOrNull( value );
- } else {
+ } else {
return value;
}
}
+ /**
+ * Converts the given object to an integer if it is an instance of Number.
+ * else it is returned unchanged.
+ *
+ * @param value : the object to be converted
+ * @return : an integer representation of the number if applicable,
otherwise the original value
+ */
+ public static Optional<Integer> coerceIntegerNumber(Object value) {
Review Comment:
Could you please update the javadoc ?
Moreover, since the called method will return an Optional<Integer> by
itself, no need to wrap it inside another Optional
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/util/NumberEvalHelperTest.java:
##########
@@ -34,4 +36,53 @@ void getBigDecimalOrNull() {
assertThat(NumberEvalHelper.getBigDecimalOrNull(10000000000.5D)).isEqualTo(new
BigDecimal("10000000000.5"));
}
+ @Test
Review Comment:
Could you please add some unit test for NumberEvalHelper.getIntegerOrNull() ?
Thanks!
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/DateFunction.java:
##########
@@ -71,31 +75,28 @@ public FEELFnResult<TemporalAccessor>
invoke(@ParameterName( "from" ) String val
}
}
- public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "year" )
Number year, @ParameterName( "month" ) Number month, @ParameterName( "day" )
Number day) {
- if ( year == null ) {
- return FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "year", "cannot be null"));
- }
- if ( month == null ) {
- return FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "month", "cannot be null"));
- }
- if ( day == null ) {
- return FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "day", "cannot be null"));
- }
-
+ public FEELFnResult<TemporalAccessor> invoke(@ParameterName("year") Number
year, @ParameterName("month") Number month, @ParameterName("day") Number day) {
try {
- return FEELFnResult.ofResult( LocalDate.of( year.intValue(),
month.intValue(), day.intValue() ) );
+ Optional<Integer> coercedYear = coerceIntegerNumber(year);
Review Comment:
See previous comment
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/util/NumberEvalHelperTest.java:
##########
@@ -34,4 +36,53 @@ void getBigDecimalOrNull() {
assertThat(NumberEvalHelper.getBigDecimalOrNull(10000000000.5D)).isEqualTo(new
BigDecimal("10000000000.5"));
}
+ @Test
+ void coerceIntegerNumber_withBigDecimal() {
+ //Verifies that BigDecimal values are truncated (not rounded) when
coerced to integers.
+ Optional<Integer> result = NumberEvalHelper.coerceIntegerNumber(new
BigDecimal("99.99"));
Review Comment:
Could you please also test, in the same method, the other "extreme" value,
i.e. "99.00001": it also should return 99
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/util/NumberEvalHelperTest.java:
##########
@@ -34,4 +36,53 @@ void getBigDecimalOrNull() {
assertThat(NumberEvalHelper.getBigDecimalOrNull(10000000000.5D)).isEqualTo(new
BigDecimal("10000000000.5"));
}
+ @Test
+ void coerceIntegerNumber_withBigDecimal() {
+ //Verifies that BigDecimal values are truncated (not rounded) when
coerced to integers.
+ Optional<Integer> result = NumberEvalHelper.coerceIntegerNumber(new
BigDecimal("99.99"));
+ assertThat(result).isPresent();
+ assertThat(result.get()).isEqualTo(99);
+ }
+
+ @Test
+ void coerceIntegerNumber_withBigInteger() {
+ Optional<Integer> result = NumberEvalHelper.coerceIntegerNumber(new
BigInteger("1000"));
+ assertThat(result).isPresent();
+ assertThat(result.get()).isEqualTo(1000);
+ }
+
+ @Test
+ void coerceIntegerNumber_withDouble() {
Review Comment:
Could you please also test, in the same method, "extreme" values, e.g.
42.009 and 42.99999: both should return 42
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/DecimalFunction.java:
##########
@@ -38,14 +40,15 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n"
) BigDecimal n, @Para
if ( n == null ) {
return FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "n", "cannot be null"));
}
- if ( scale == null ) {
- return FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
+ Optional<Integer> scaleObj =
NumberEvalHelper.coerceIntegerNumber(scale);
+ if(scaleObj.isEmpty()) {
+ return FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "scale", "must be a non-null Number
value."));
}
+ int scaleInt = scaleObj.get();
// Based on Table 76: Semantics of numeric functions, the scale is in
range −6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 ||
scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111
to 6176."));
+ if (scaleInt < -6111 || scaleInt > 6176) {
Review Comment:
See comment on CeilingFunction
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/CeilingFunction.java:
##########
@@ -45,11 +47,14 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n"
) BigDecimal n, @Para
if ( scale == null ) {
return FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
}
- // Based on Table 76: Semantics of numeric functions, the scale is in
range −6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 ||
scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111
to 6176."));
+ Optional<Integer> scaleObj =
NumberEvalHelper.coerceIntegerNumber(scale);
Review Comment:
Hi @ChinchuAjith
Optional could be used in more idiomatic way to express the same behavior:
```java
Optional<Integer> scaleObj = NumberEvalHelper.coerceIntegerNumber(scale);
AtomicReference<FEELFnResult<BigDecimal>> toReturn = new AtomicReference<>();
scaleObj.ifPresentOrElse(scaleInt -> {
if (scaleInt < -6111 || scaleInt >
6176) {
toReturn.set(FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR,
"scale", "must be in range between -6111 and 6176.")));
} else {
toReturn.set(FEELFnResult.ofResult(n.setScale(scaleInt, RoundingMode.CEILING)));
}
},
() -> toReturn.set(FEELFnResult.ofError(new
InvalidParametersEvent(Severity.ERROR, "scale", "should be a Number type"))));
return toReturn.get();
```
Your choice, anyway
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
