[refactor] Restructure RangeIndexAttributeCondition#5259
[refactor] Restructure RangeIndexAttributeCondition#5259line-o wants to merge 7 commits intoeXist-db:developfrom
Conversation
- only keep operator and two flags in memor - value, lowercasevalue and numericvalue were replaced by two functions - indexPredicate and queryPredicate
reinhapa
left a comment
reinhapa
left a comment
There was a problem hiding this comment.
Ony a small change found ;-)
...indexes/range/src/main/java/org/exist/indexing/range/RangeIndexConfigAttributeCondition.java
Outdated
Show resolved
Hide resolved
|
|
@reinhapa I believe the changes you requested were addressed in the meantime |
|
@reinhapa could you please re-review, I believe it's good to go |
There was a problem hiding this comment.
I am not sure if this also addresses a bug or not? If it is just refactoring of existing code, then I have concerns:
- The newer code is more complex and difficult to follow; I don't see what advantage it offers over the existing code.
- The newer code exhibits symptoms of "information hiding". e.g. the variable
caseSensitive(amongst others) is no longer knowable within an instance of the class at runtime; one side effect of that is that it will make debugging any future issues much more difficult. - I haven't tested it with JMH myself, but my intuition is that the new code will perform worse that the older code. Has this been tested with JMH yet?
| } | ||
| operator = getOperator(elem); | ||
| commutative = (operator == Operator.EQ || operator == Operator.NE); | ||
| commutativeNegate = (operator == Operator.GT || operator == Operator.LT || operator == Operator.GE || operator == Operator.LE); |
There was a problem hiding this comment.
commutativeNegate is a very strange name, at least in English. Perhaps notCommutative would be better to describe what you are trying to model?
|
commutativeNegate could be renamed to converse (see https://en.m.wikipedia.org/wiki/Converse_relation and https://en.m.wikipedia.org/wiki/Inequality_(mathematics) ). |
5 participants
Description:
These private properties of RangeIndexAttributeCondition
were replaced by
indexPredicate and queryPredicate are references to one-line lambdas.
Before, a lot of switching and checking happened on each call to
findandmatches