[GitHub] [commons-text] kinow commented on a change in pull request #181: Add validate for WordUtils.java

[GitBox <gi...@apache.org>]

kinow commented on a change in pull request #181:
URL: https://github.com/apache/commons-text/pull/181#discussion_r514544829



##########
File path: src/main/java/org/apache/commons/text/WordUtils.java
##########
@@ -859,7 +859,7 @@ public static boolean isDelimiter(final int codePoint, final char[] delimiters)
     public static String abbreviate(final String str, int lower, int upper, final String appendToEnd) {
         Validate.isTrue(upper >= -1, "upper value cannot be less than -1");
         Validate.isTrue(upper >= lower || upper == -1, "upper value is less than lower value");
-
+        Validate.isTrue(lower >= 0, "lower value cannot be lower than 0");

Review comment:
       @bjmbing thanks for the pull request. Initially this change made sense to me. Then looking at the `abbreviate` code, looks like the `lower` limit is used for `StringUtils.indexOf(str, " ", /* startPos */ lower)`.
   
   `StringUtils.indexOf` javadocs say: "* @param startPos  the start position, negative treated as zero".
   
   Currently, if you pass `-1` or any other negative value, it is treated as zero, in accord with the `StringUtils.indexOf` javadocs.
   
   I think a better solution would be to document that negative values are treated as zero, essentially replicating the `StringUtils` docs. This way, existing code using negative values won't get a runtime exception (which `Validate.isTrue` would do). What do you think?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org