Refactorings #58
Refactorings #58
Conversation
… in JSONiq, the default mode)
| @@ -62,7 +62,6 @@ private String toGcmpString(Cmp cmp) { | |||
| case gt -> ">"; | |||
| case le -> "<="; | |||
| case lt -> "<"; | |||
| default -> ">="; | |||
There was a problem hiding this comment.
Seems unlikely, but I wonder if there is any value in having some sort of RuntimeException here in case a future version of Cmp could represent something other than one of the enumerated switch values (in the case where Cmp was updated but this switch wasn't)?
There was a problem hiding this comment.
As Cmp is an enum we should provide a String getValue() method or something like this and call cmp.getValue() instead.
| public TableJoinCursor(Cursor lc, int lSize, int pad) { | ||
| this.lc = lc; | ||
| this.lSize = lSize; | ||
| public TableJoinCursor(Cursor cursor, int size, int pad) { |
There was a problem hiding this comment.
As a newbie who has been reading the code lately, I appreciate these reader-friendly name refactorings :-)
There was a problem hiding this comment.
It was short for left cursor I assume. However, the hash table should be built on the operator with the lower tuple size, so it can be either the cursor over the right or left operator tuples. I hope I got this right.
| @@ -818,6 +818,7 @@ private AST optionDecl() throws TokenizerException { | |||
| decl.addChild(stringLiteral); | |||
|
|
|||
| if ("jn:jsoniq-boolean-and-null-literals".equals(eqnameLiteral.getStringValue())) { | |||
| assert stringLiteral != null; | |||
There was a problem hiding this comment.
I'm curious (being a newbie here), do you leave assertions on for the released code? Do you have strong opinions on assert vs. Objects.requireNonNull()?
There was a problem hiding this comment.
Usually I'd leave them on, but regarding a DBS query engine I'm not sure. But I guess over her assertions are better as they can be disabled.
No description provided.