[LANG-1593] Common behavior for StringUtils join APIs when called with char or String delimiter by HubertWo · Pull Request #784 · apache/commons-lang

HubertWo · 2021-08-04T20:09:52Z

Added dedicated StringUtils.join methods for all Java primitives.
All new methods accept String as a delimiter.
From now on, old methods StringUtils.join which accept char as delimiter internally use String.valueOf(delimiter).

I have added addition test cases.

coveralls · 2021-08-05T00:07:44Z

Coverage decreased (-0.003%) to 94.943% when pulling 554f8c7 on HubertWo:fix/LANG-1593-join-string-delimiter into a3a0645 on apache:master.

…NG-1593-join-string-delimiter

garydgregory · 2021-08-24T13:27:34Z

+     * @since 3.13.0
+     */
+    public static String join(final char[] elements, final String separator) {
+        return elements == null ? null : join(elements, separator, 0, elements.length);


The null checks in these methods are superfluous as the join method called already checks for the same condition. Less boilerplate is better IMO.

The method is public - so users may pass null as elements which will cause NPE in the same line elements.length

assertNull(StringUtils.join((char[]) null, "-")); // throws NPE if null check is removed

This case was not tested, so I added additional assertions in tests to cover it.

garydgregory · 2021-08-24T13:28:38Z

        assertEquals("foo2", StringUtils.join(MIXED_TYPE_LIST));
    }

-    @Disabled


Was this test removed because the code is already covered elsewhere? Did you confirm this by looking at the JaCoCo report?

This is exactly the test that exposes this bug: testLang1593.
Please find the same assertion here testJoin_ArrayOfInt in StringUtilsTest

assertEquals("1,2", StringUtils.join(INT_PRIM_LIST, SEPARATOR));

When it comes to coverage, it looks like join methods have 100% coverage.

garydgregory · 2021-08-24T13:29:14Z

@HubertWo
Thank you for your update. Please see my comments.

XenoAmess · 2021-08-24T19:59:34Z

I just think it use too many String.valueOf
Is it really performance acceptable?
I do think it it need a jmh...

garydgregory · 2021-08-31T14:14:20Z

        assertNull(StringUtils.join((short[]) null, SEPARATOR_CHAR, 0, 1));
        assertEquals(StringUtils.EMPTY, StringUtils.join(SHORT_PRIM_LIST, SEPARATOR_CHAR, 0, 0));
        assertEquals(StringUtils.EMPTY, StringUtils.join(SHORT_PRIM_LIST, SEPARATOR_CHAR, 1, 0));
+        assertEquals("1,2", StringUtils.join(SHORT_PRIM_LIST, SEPARATOR));


@HubertWo
The point of String vs char separators is that a String can be longer than one character, so you should test for String separators longer than one character to make sure the right work takes place under the covers.

Sorry for the late reply, I was on vac.
I will take a look on your comments and try to apply changes later this week.

The joining part is done via java.util.StringJoiner with accepts onlyCharSequence in constructor, so it's made to use more than one character as separator by default. Don't you think that testing that would be redundant?

…NG-1593-join-string-delimiter

HubertWo · 2021-09-03T03:30:06Z

@XenoAmess
Could you please be more descriptive? The only String.valueOf I have added are in high level methods, main logic does not. Actually I removed String.valueOf from loops. Sorry for answering in comment, but there's no way to add reply to your comment.

HubertWo · 2021-09-03T03:32:40Z

@garydgregory looks like I've addressed all the questions.
Could you please review again?
Hope it will be LGTM this time :)

HubertWo · 2021-09-20T05:36:12Z

@garydgregory BUMP

XenoAmess · 2021-10-05T00:52:05Z

@XenoAmess Could you please be more descriptive? The only String.valueOf I have added are in high level methods, main logic does not. Actually I removed String.valueOf from loops. Sorry for answering in comment, but there's no way to add reply to your comment.

@HubertWo

Hi.

I find some time today for this question (finally).

The only String.valueOf I have added are in high level methods, main logic does not

This statement be correct, but also be where I worried about performance.

Please look at the sources in JDK, where java.lang.AbstractStringBuilder#append(boolean b)

In short, for basic types, should not convert them to String, but use StringBuilder directly, will bring far better performance.

BUT I just foundout that you did not bring this issue, this issue is already in commons-lang before your pr.

So I will not stop you from merging this in, but still I will find time to refactor the whole join serial functions later.

HubertWo · 2021-10-06T15:35:52Z

@XenoAmess Could you please be more descriptive? The only String.valueOf I have added are in high level methods, main logic does not. Actually I removed String.valueOf from loops. Sorry for answering in comment, but there's no way to add reply to your comment.

@HubertWo

Hi.

I find some time today for this question (finally).

The only String.valueOf I have added are in high level methods, main logic does not

This statement be correct, but also be where I worried about performance.

Please look at the sources in JDK, where java.lang.AbstractStringBuilder#append(boolean b)

In short, for basic types, should not convert them to String, but use StringBuilder directly, will bring far better performance.

BUT I just foundout that you did not bring this issue, this issue is already in commons-lang before your pr.

So I will not stop you from merging this in, but still I will find time to refactor the whole join serial functions later.

I was not aware of that.
I will be happy to introduce changes (with JMH tests) related to your suggestions in separate PR.
I've crated new JIRA for it: https://issues.apache.org/jira/browse/LANG-1675

Thank you for your time @XenoAmess .

Update:
I've created PR with StringBuilder in StringUtils.join and JMH benchmarks.
Please find it here: #812

HubertWo added 18 commits July 22, 2021 17:15

Failing test cases

8e687c3

Fix for: long[]

79b47b8

Join: byte[] and boolean[]

43a603e

Join: char[]

5ecaa66

Join: double[]

4162096

Join: short[] and float[]

05a4a17

Code cleanup

e83115d

Removed disabled test

1719242

Code cleanup - method order

8fc19c9

Removed import of Streams.* and added missing join method for long[]

b0f5bd3

Removed generic from method signatures

d106a3f

Code cleanup

aa208fd

Removed toStringOrEmpty

b7a7d51

Test cases for null separator

ae3325d

Test case for join char[] with null separator

d235935

JavaDoc

7d53cfa

JavaDoc

feee787

Fix: removed unused import

ed64d9c

garydgregory reviewed Aug 4, 2021

View reviewed changes

Comment thread src/main/java/org/apache/commons/lang3/StringUtils.java

garydgregory reviewed Aug 4, 2021

View reviewed changes

Comment thread src/main/java/org/apache/commons/lang3/StringUtils.java Outdated

garydgregory reviewed Aug 4, 2021

View reviewed changes

Comment thread src/main/java/org/apache/commons/lang3/StringUtils.java Outdated

garydgregory reviewed Aug 4, 2021

View reviewed changes

Comment thread src/main/java/org/apache/commons/lang3/StringUtils.java Outdated

HubertWo added 4 commits August 5, 2021 18:40

Code review fix - remove useless parentheses

b7693a3

Code review fix - brought back helper method

ffe3177

Code review fix - Javadocs

170c7cb

Code review fix - added missing Javadocs

9c29274

HubertWo marked this pull request as ready for review August 5, 2021 19:24

HubertWo added 2 commits August 17, 2021 08:00

Merge branch 'apache:master' into fix/LANG-1593-join-string-delimiter

3dc68c2

Merge branch 'master' of github.com:HubertWo/commons-lang into fix/LA…

f18c98e

…NG-1593-join-string-delimiter

garydgregory reviewed Aug 21, 2021

View reviewed changes

Comment thread src/main/java/org/apache/commons/lang3/StringUtils.java Outdated

Fix: Javadoc - removed paragraph tag from first line

5b1a23e

garydgregory reviewed Aug 24, 2021

View reviewed changes

garydgregory reviewed Aug 31, 2021

View reviewed changes

HubertWo added 2 commits September 3, 2021 04:39

Merge branch 'master' of github.com:HubertWo/commons-lang into fix/LA…

bc156ae

…NG-1593-join-string-delimiter

Tests for StringUtils.join - null array, String separator

26f3b05

Merge branch 'apache:master' into fix/LANG-1593-join-string-delimiter

554f8c7

This was referenced Oct 9, 2021

LANG-1675 - Improve performance of StringUtils.join for primitives - preview HubertWo/commons-lang#4

Closed

LANG-1675 - Improve performance of StringUtils.join for primitives #812

Merged

Conversation

HubertWo commented Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coveralls commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

garydgregory Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

HubertWo Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

garydgregory Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

HubertWo Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory commented Aug 24, 2021

Uh oh!

XenoAmess commented Aug 24, 2021

Uh oh!

garydgregory Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

HubertWo Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

HubertWo Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

HubertWo commented Sep 3, 2021

Uh oh!

HubertWo commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HubertWo commented Sep 20, 2021

Uh oh!

XenoAmess commented Oct 5, 2021

Uh oh!

HubertWo commented Oct 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HubertWo commented Aug 4, 2021 •

edited

Loading

coveralls commented Aug 5, 2021 •

edited

Loading

HubertWo Sep 3, 2021 •

edited

Loading

HubertWo commented Sep 3, 2021 •

edited

Loading

HubertWo commented Oct 6, 2021 •

edited

Loading