TEXT-41 : Added WordUtils.abbreviate support to commons-text #38

ameyjadiye · 2017-05-03T19:16:40Z

No description provided.

coveralls · 2017-05-03T19:19:22Z

Coverage increased (+0.02%) to 96.577% when pulling 0d38c1b on ameyjadiye:TEXT-41 into 0b6a285 on apache:master.

ameyjadiye · 2017-05-03T19:26:13Z

Hi @chtompki @PascalSchumacher @kinow , I was going through each and every JIRA marked for 1.1 release and seems they are pending since long just like TEXT-41, pushing code for TEXT-41 for now, let me know if this looks good ?

chtompki · 2017-05-03T21:42:30Z

I'll try to give this a look sometime tonight or tomorrow morning.

kinow · 2017-05-04T00:41:37Z

Had a quick (really quick) look today in between meetings. Looks good to me. Wonder if it'd be better to use a StringBuilder rather than StringBuffer. And perhaps add a test with other negative values, not just -1 (because even though the contract states -1 is a valid option, users can still pass -100, -10, etc... for typo, bad use, bug, etc).

PascalSchumacher · 2017-05-04T08:41:32Z

src/main/java/org/apache/commons/text/WordUtils.java

+     *                    This is appended ONLY if the string was indeed abbreviated.
+     *                    The append does not count towards the lower or upper limits.
+     * @return the abbreviated String.
+     * @since 2.4


should be 1.1

Sorry I have to correct myself. The tag should be removed as the class already has @since 1.1.

PascalSchumacher · 2017-05-04T08:46:06Z

src/main/java/org/apache/commons/text/WordUtils.java

+
+        if (str.length() == 0) {
+            return StringUtils.EMPTY;
+        }


The two if clauses can be replaced by:

if (StringUtils.isEmpty(str)) { return str; }

PascalSchumacher · 2017-05-04T08:54:19Z

+1to StringBuilder

What about throwing IllegalArgumentExceptions for upper < -1 and upper < lower?

kinow · 2017-05-04T09:11:05Z

What about throwing IllegalArgumentExceptions for upper < -1 and upper < lower?

Makes sense IMO. The upper < -1 would be nice and could show users they have a bug (as values lower than -1 were not supposed to be used). upper < lower would be changing the behaviour from [lang]; which I'm fine... as this is a good chance, as we are migrating it to [text]... so +1 as well.

ameyjadiye · 2017-05-04T11:03:17Z

StringBuffer makes it implicitly threadsafe, if we use StringBuilder users have to take care threadsafty themselves, that could be the reason Commons lang used it before.

PascalSchumacher · 2017-05-04T11:09:58Z

@ameyjadiye The thread-safety of StringBuffer is only relevant if it is shared by multiple threads. A local variable of a method can not be accessed by multiple threads. Commons lang probably used StringBuffer because the code was written before StringBuilder was added in Java 1.5.

kinow · 2017-05-04T11:10:35Z

StringBuffer makes it implicitly threadsafe, if we use StringBuilder users have to take care threadsafty themselves, that could be the reason Commons lang used it before.

My guess would be that [lang] uses it as this method existed probably before Java 1.5.

If you look at the scope, there is no need to call synchronized methods.

ameyjadiye · 2017-05-04T14:00:32Z

@PascalSchumacher , got it , will fix everything today, let me know if rest things are good I'm happy to make it perfect for release.

coveralls · 2017-05-04T15:04:55Z

Coverage increased (+0.02%) to 96.575% when pulling aa91645 on ameyjadiye:TEXT-41 into d4890a8 on apache:master.

coveralls · 2017-05-04T15:07:02Z

Coverage increased (+0.02%) to 96.575% when pulling a611df5 on ameyjadiye:TEXT-41 into d4890a8 on apache:master.

coveralls · 2017-05-04T15:13:29Z

Coverage increased (+0.02%) to 96.573% when pulling e3ea9dd on ameyjadiye:TEXT-41 into d4890a8 on apache:master.

coveralls · 2017-05-04T15:13:29Z

Coverage increased (+0.02%) to 96.573% when pulling e3ea9dd on ameyjadiye:TEXT-41 into d4890a8 on apache:master.

coveralls · 2017-05-04T15:13:29Z

Coverage increased (+0.02%) to 96.573% when pulling e3ea9dd on ameyjadiye:TEXT-41 into d4890a8 on apache:master.

…pective testcases

coveralls · 2017-05-04T16:27:32Z

Coverage increased (+0.02%) to 96.575% when pulling 44847c5 on ameyjadiye:TEXT-41 into d4890a8 on apache:master.

ameyjadiye · 2017-05-04T16:57:00Z

@kinow @chtompki @PascalSchumacher , all discussed improvements and their tests are pushed and looks good to me now, let me know if anything else is required for this PR ?

chtompki · 2017-05-04T17:06:36Z

I'm open to these changes existing here, but do we have a solid reason for them not going in [lang]'s StringUtils with all of the other abbreviate methods?

Consider this:
https://github.com/apache/commons-lang/blob/ead7d965e709182157694490c8bc8d744ec64161/src/main/java/org/apache/commons/lang3/StringUtils.java#L5707-L5769
StringUtils as it existed when they took out abbreviate from WordUtils. It seems that the functionality is clearly different, but is this the correct place for it?

Lastly we'll want to be sure that we fix this bug:
https://issues.apache.org/jira/browse/LANG-673
if we do bring it back in.

Any thoughts here? I'm not very partial to either direction, putting this in WordUtils or StringUtils, I more just wanted to get the thoughts in to every one here's head.

ameyjadiye · 2017-05-04T17:20:32Z

I'm in favour of keeping version of WordUtils.abbreviate to work on words, I wish we should improve the functionality more. Now as the functionality differs in both WordUtils and StringUtils just name of method don't make them eligible to mark as redundant and to be removed from either utility class.

chtompki · 2017-05-04T17:35:21Z

I'm ok with that. I'm going to fiddle around with this a tad to see if I can re-create the problem that they were having in LANG-673.

PascalSchumacher · 2017-05-04T17:47:09Z

As it is functionally is different from the StringUtils variant and tries to abbreviate after a word I think this is the correct place for this functionality.

+1 to including the fix LANG-673.

chtompki · 2017-05-04T17:54:16Z

I'm still fiddling around with how to re-create the issue that LANG-673 is talking about. I couldn't get that test to cause an exception on the code here: apache/commons-lang@ead7d96

I'll keep fiddling around and let you know when I figure out what the issue is really getting after.

coveralls · 2017-05-04T17:55:01Z

Coverage increased (+0.02%) to 96.575% when pulling c2a8f93 on ameyjadiye:TEXT-41 into d4890a8 on apache:master.

PascalSchumacher

Thanks for all the updates so far! 👍

PascalSchumacher · 2017-05-04T17:52:32Z

src/main/java/org/apache/commons/text/WordUtils.java

+     * the String there. It will also append any String passed as a parameter
+     * to the end of the String. The upper limit can be specified to forcibly
+     * abbreviate a String.
+     *


I think we should add some examples to the javadoc. I often find these very helpful. What do you think?

Yeah Agreed [+1] , nothing is more helpful than samples in javadoc.

Not a bad idea. That's what [lang] seems to do.

PascalSchumacher · 2017-05-04T18:03:03Z

src/main/java/org/apache/commons/text/WordUtils.java

+        // throw IllegalArgumentException if upper value is less than lower value.
+        if (upper < lower && upper != -1) {
+            throw new IllegalArgumentException("upper value is less than lower value");
+        }


I think the validation should be done at the very start of the method.

Code could be slightly shortened by using the Validate class from commons-lang.

Comments for the validation can be removed imho the do not add anything beyond the code.

I have moved validation on method start but cant find appropriate Validation method to use the way we are doing validations, also i think code here is easy to understand anyway.

chtompki · 2017-05-04T18:15:30Z

I reproduced the issue and confirmed that you don't have it in your code. The test that should cover it is:

assertEquals("01 23 45 67 89",WordUtils.abbreviate("01 23 45 67 89", 40, 40, ""));

can you add that?

For the sake of rigor, that exception existed when the method appeared as:
https://github.com/apache/commons-lang/blob/LANG_2_4/src/java/org/apache/commons/lang/WordUtils.java#L605

public static String abbreviate(String str, int lower, int upper, String appendToEnd) {
        // initial parameter checks
        if (str == null) {
            return null;
        }
        if (str.length() == 0) {
            return StringUtils.EMPTY;
        }

        // if the upper value is -1 (i.e. no limit) or is greater
        // than the length of the string, set to the length of the string
        if (upper == -1 || upper > str.length()) {
            upper = str.length();
        }
        // if upper is less than lower, raise it to lower
        if (upper < lower) {
            upper = lower;
        }

        StringBuffer result = new StringBuffer();
        int index = StringUtils.indexOf(str, " ", lower);
        if (index == -1) {
            result.append(str.substring(0, upper));
            // only if abbreviation has occured do we append the appendToEnd value
            if (upper != str.length()) {
                result.append(StringUtils.defaultString(appendToEnd));
            }
        } else if (index > upper) {
            result.append(str.substring(0, upper));
            result.append(StringUtils.defaultString(appendToEnd));
        } else {
            result.append(str.substring(0, index));
            result.append(StringUtils.defaultString(appendToEnd));
        }
        return result.toString();
    }

ameyjadiye · 2017-05-04T18:30:26Z

Thanks much @chtompki, added test and current implementation is handing scenario as expected and not throwing exception.

chtompki · 2017-05-04T18:30:50Z

Sure glad to help there.

coveralls · 2017-05-04T18:33:49Z

Coverage increased (+0.02%) to 96.575% when pulling e9cafb5 on ameyjadiye:TEXT-41 into d4890a8 on apache:master.

coveralls · 2017-05-04T18:33:49Z

Coverage increased (+0.02%) to 96.575% when pulling e9cafb5 on ameyjadiye:TEXT-41 into d4890a8 on apache:master.

coveralls · 2017-05-04T19:42:54Z

Coverage increased (+0.02%) to 96.575% when pulling 83b3662 on ameyjadiye:TEXT-41 into d4890a8 on apache:master.

coveralls · 2017-05-04T19:55:14Z

Coverage increased (+0.02%) to 96.575% when pulling 9615df7 on ameyjadiye:TEXT-41 into d4890a8 on apache:master.

coveralls · 2017-05-04T19:55:14Z

Coverage increased (+0.02%) to 96.575% when pulling 9615df7 on ameyjadiye:TEXT-41 into d4890a8 on apache:master.

coveralls · 2017-05-04T19:55:14Z

Coverage increased (+0.02%) to 96.575% when pulling 9615df7 on ameyjadiye:TEXT-41 into d4890a8 on apache:master.

ameyjadiye · 2017-05-04T20:06:09Z

@chtompki , I think we have covered pretty much everything now, let me know anything pending/blocker to accept this PR.

PascalSchumacher · 2017-05-05T09:25:20Z

Thanks for the updates!

I think the pull request is ready to merge now. 👍

ameyjadiye added 2 commits May 4, 2017 00:41

Added WordUtils.abbreviate from commons lang

8b42549

Added WordUtils.abbreviate test cases

0d38c1b

PascalSchumacher reviewed May 4, 2017

View reviewed changes

Merge branch 'master' into TEXT-41

5eba72c

ameyjadiye added 3 commits May 4, 2017 20:28

removed redundunt @SInCE tag

b043de5

merged master and fixed failing textcases

aa91645

replaced StringBuffer with StringBuilder

a611df5

optimized empty check with StringUtils.isEmpty

e3ea9dd

raise IllegalArgumentException if upper < -1 or upper < lower and res…

44847c5

…pective testcases

added tests to verify LANG-673

c2a8f93

PascalSchumacher reviewed May 4, 2017

View reviewed changes

somemore tests to verify LANG-673

e9cafb5

added samples in javadoc

83b3662

moved validations on start of method

9615df7

asfgit merged commit 9615df7 into apache:master May 5, 2017

TEXT-41 : Added WordUtils.abbreviate support to commons-text #38

TEXT-41 : Added WordUtils.abbreviate support to commons-text #38

Conversation

ameyjadiye commented May 3, 2017

coveralls commented May 3, 2017 • edited

ameyjadiye commented May 3, 2017

chtompki commented May 3, 2017

kinow commented May 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PascalSchumacher commented May 4, 2017

kinow commented May 4, 2017

ameyjadiye commented May 4, 2017

PascalSchumacher commented May 4, 2017

kinow commented May 4, 2017

ameyjadiye commented May 4, 2017

coveralls commented May 4, 2017 • edited

coveralls commented May 4, 2017 • edited

coveralls commented May 4, 2017

coveralls commented May 4, 2017

coveralls commented May 4, 2017

coveralls commented May 4, 2017 • edited

ameyjadiye commented May 4, 2017

chtompki commented May 4, 2017

ameyjadiye commented May 4, 2017 • edited

chtompki commented May 4, 2017

PascalSchumacher commented May 4, 2017 • edited

chtompki commented May 4, 2017

coveralls commented May 4, 2017 • edited

PascalSchumacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chtompki commented May 4, 2017

ameyjadiye commented May 4, 2017

chtompki commented May 4, 2017

coveralls commented May 4, 2017

coveralls commented May 4, 2017 • edited

coveralls commented May 4, 2017 • edited

coveralls commented May 4, 2017

coveralls commented May 4, 2017

coveralls commented May 4, 2017

ameyjadiye commented May 4, 2017

PascalSchumacher commented May 5, 2017

coveralls commented May 3, 2017 •

edited

coveralls commented May 4, 2017 •

edited

coveralls commented May 4, 2017 •

edited

coveralls commented May 4, 2017 •

edited

ameyjadiye commented May 4, 2017 •

edited

PascalSchumacher commented May 4, 2017 •

edited

coveralls commented May 4, 2017 •

edited

coveralls commented May 4, 2017 •

edited

coveralls commented May 4, 2017 •

edited