yesamer commented on code in PR #6299:
URL:
https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2057703592
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/CeilingFunction.java:
##########
@@ -45,11 +48,16 @@ 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."));
- }
-
- return FEELFnResult.ofResult( n.setScale( scale.intValue(),
RoundingMode.CEILING ) );
+ Optional<Integer> scaleObj =
NumberEvalHelper.coerceIntegerNumber(scale);
+ AtomicReference<FEELFnResult<BigDecimal>> toReturn = new
AtomicReference<>();
Review Comment:
@ChinchuAjith Thank you, I guess the performances are not impacted as you
said (but the memory footprint is undoubtedly increased any time you create an
AtomicReference).
In any case, despite your solution being correct from a functional point of
view, I still believe that removing the `Optional` could result in a
considerable simplification. From my point of view, the case you're managing is
an INVALID case, not an OPTIONAL case. The difference is subtle, let me explain
it:
Currently, you are:
- Coercing a `number` returning an `Optional`. If the given parameter is
valid, the Optional holds the number. If the given parameter is INVALID, an
empty optional is returned.
- The callers of the `NumberEvalHelper.coerceIntegerNumber` function will
take the number, if present in the Optional, or throw an exception, strengthen
my above statement that we are managing an INVALID case
- The exception is then caught and managed.
My proposal is NOT returning null in case of invalid parameters, but
immediately throwing an exception in that case, simplifying the code because
the second point of the above list is no longer necessary.
To be clear:
```
public static Integer coerceIntegerNumber(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();
}
throw new IllegalArgumentException("The provided value: " + value +
" is not a number");
}
```
Ping me in private in case, I'm available to talk!
--
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]
