Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[LANG-1593] Common behavior for StringUtils join APIs when called with char or String delimiter #784

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

HubertWo
Copy link
Contributor

@HubertWo HubertWo commented Aug 4, 2021

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
Copy link

coveralls commented Aug 5, 2021

Coverage Status

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

@HubertWo HubertWo marked this pull request as ready for review August 5, 2021 19:24
* @since 3.13.0
*/
public static String join(final char[] elements, final String separator) {
return elements == null ? null : join(elements, separator, 0, elements.length);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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
  1. This case was not tested, so I added additional assertions in tests to cover it.

@@ -1355,15 +1372,6 @@ public void testJoin_Objectarray() {
assertEquals("foo2", StringUtils.join(MIXED_TYPE_LIST));
}

@Disabled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@HubertWo HubertWo Sep 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Screen Shot 2021-09-03 at 8 21 40 PM

@garydgregory
Copy link
Member

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

@XenoAmess
Copy link
Contributor

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

@@ -1226,6 +1241,8 @@ public void testJoin_ArrayOfShorts() {
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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@HubertWo
Copy link
Contributor Author

HubertWo commented Sep 3, 2021

@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
Copy link
Contributor Author

HubertWo commented Sep 3, 2021

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

@HubertWo
Copy link
Contributor Author

@garydgregory BUMP

@XenoAmess
Copy link
Contributor

@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
Copy link
Contributor Author

HubertWo commented Oct 6, 2021

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants