Improve XPathPathExpr code quality #307
Conversation
Codecov Report
@@ Coverage Diff @@
## master #307 +/- ##
============================================
+ Coverage 45.91% 45.94% +0.02%
- Complexity 2680 2684 +4
============================================
Files 233 234 +1
Lines 13316 13323 +7
Branches 2589 2580 -9
============================================
+ Hits 6114 6121 +7
- Misses 6390 6391 +1
+ Partials 812 811 -1
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| return new XPathNodeset(nodesetRefs, m, ec); | ||
| public XPathNodeset eval(final DataInstance unusedDataInstance, final EvaluationContext ec) { |
There was a problem hiding this comment.
I would argue to remove these finals here. Maybe the one for TreeReference ref in getReference() is useful while it gets smaller but here it's just one line of code and those variables are just passed downstream.
There was a problem hiding this comment.
Thanks. I intended to remove them, and will now. They were part of the evolution of the code within this refactoring (originally the first parameter was never read, but reassigned!).
There was a problem hiding this comment.
Wow, it's great that you fixed that one :)
There was a problem hiding this comment.
At least I made it obvious that all those callers are passing in something that is ignored.
| return false; | ||
| } | ||
|
|
||
| return ExtUtil.arrayEquals(steps, x.steps) && (init_context == INIT_CONTEXT_EXPR ? filtExpr.equals(x.filtExpr) : true); | ||
| return ExtUtil.arrayEquals(steps, x.steps) && (init_context != INIT_CONTEXT_EXPR || filtExpr.equals(x.filtExpr)); |
There was a problem hiding this comment.
I might have to wait for tomorrow to be fresh again to really be sure that this does the same, but I'm guessing you've just let IntelliJ simplify the boolean expression 👍
There was a problem hiding this comment.
Yes. I would not be confident making that change on my own.
|
|
||
| parentsAllowed = false; | ||
| if (func.args.length != 1) { | ||
| throw new XPathUnsupportedException("instance() function used with " + func.args.length + " arguments. Expecting 1 argument"); |
There was a problem hiding this comment.
"arguement" sounds much more interesting. Spelling is overrated :D
…and clean up its code (cherry picked from commit 2d5998d)
Move eval method from XPathPathExpr to new class, XPathPathExprEval, and modernize its code and make it much more understandable.
(cherry picked from commit 2d5998d)
What has been done to verify that this works as intended?
I saw that all tests still pass, and I stepped through the code in the debugger.
Why is this the best possible solution? Were any other approaches considered?
The goal was to separate the
evalconcern from the rest of XPathPathExpr, and to modernize code using automatic IDEA refactorings.Are there any risks to merging this code? If so, what are they?
The risk seems low. I used refactorings like extract method, which are safe.