julianhyde commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1370825644
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -431,12 +438,41 @@ private static boolean checkPosOccurrenceParamValues(int
position,
throw
RESOURCE.invalidIntegerInputForRegexpFunctions(Integer.toString(occurrencePosition),
"occurrence_position", methodName).ex();
}
- if (position <= value.length()) {
+ if (position > value.length()) {
return false;
}
return true;
Review Comment:
Good point, Tanner. It vindicates my philosophy that method's name and
javadoc should say what the method does and returns.
That a method is named `checkXxx` and returns `boolean` (or throws) is a
code smell. Note that the other `checkXxx` methods are `void`. As soon as a
method's return changes from `void` to `boolean` you should rename it and
re-write its javadoc.
The 'Helper for ...' pattern is harmful because it takes you out of that
discipline.
--
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]
