Bin merge test & input validation #11275
Conversation
fuzhaoyuan
force-pushed
the
demo-rfc80-poc-bin-range-input-validation
branch
6 times, most recently
from
dde6703 to
abdabc7
Compare
| @@ -138,29 +143,29 @@ public static <T extends DataFilter> List<T> mergeDataFilters(List<T> filters) { | |||
| // leave non-numerical values as they are | |||
| if (dataFilterValue.getValue() != null) { | |||
| nonNumericalValues.add(dataFilterValue); | |||
| continue; | |||
There was a problem hiding this comment.
@fuzhaoyuan can we add a comment explaining why non-null means non numerical? i know that is true i just think someone coming to this for the first time would be baffled by it.
There was a problem hiding this comment.
also, a comment explaining why we're going to jump to next iteration (continue)
There was a problem hiding this comment.
i actually think that making an else clause would be clearer/preferable to continue.
There was a problem hiding this comment.
in general i think this logic here is very difficult to grok. because the number of items (bins) is always going to be negligible, i would prioritize readability and adopt a functional approach where we just filter out the non numericals and numericals into a separate array, sort the numericals and then use reduce to iteratively merge them depending on adjacency.
There was a problem hiding this comment.
I used continue not else because otherwise SonarQube would complain the cognitive complexity.
I have added comments throughout the process. Basically yes we filter out non-numericals and then for numericals we merge them depending on adjacency.
The sorting would be a hassle because of the structure of class DataFilterValue, the actual bin range is hidden deep inside, so I just prevented unsorted filters in the input validation.
fuzhaoyuan
force-pushed
the
demo-rfc80-poc-bin-range-input-validation
branch
from
ccde38e to
f51eb55
Compare
|
Describe changes proposed in this pull request: