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

HHH-12346: Replace StringHelper#join by Java's String#join #2174

Merged
merged 1 commit into from
Mar 29, 2018

Conversation

kinow
Copy link
Contributor

@kinow kinow commented Mar 3, 2018

This pull request replaces a couple of StringHelper#join methods and its occurrences by Java 8's String#join.

As I mentioned in the JIRA ticket, this is a left-over after I submitted another pull request. I had noted this class methods, and noticed at least the join could be replaced.

Had to use streams in three places to convert from Integer to String.

Not a problem if it is decided not to touch this method for being in core module, or because it just works :-) Otherwise, let me know if further testing, syntax, formatting, etc, is necessary. Happy to update the ticket with new commits, squash, rebase, etc.

Cheers
Bruno

@kinow
Copy link
Contributor Author

kinow commented Mar 3, 2018

Travis CI failed in the check part. I did only clean test (more familiar with Maven, sorry). Updated code, re-run with check, and confirmed this part passed now. Let's see if Travis CI can build it fine now.

@sebersole
Copy link
Member

I'd prefer to not use Streams for the conversion. We have run perf tests on using Streams for such operations and they perform significantly worse. I did not see such calls in my quick look over the changes - just based in your comment.

A better option might have been to simply have overloaded methods on StringHelper to take Strings versus Objects. Calls involving Strings could have delegated to String#join

@kinow
Copy link
Contributor Author

kinow commented Mar 3, 2018

Good point @sebersole . I've found situations where debugging problems was trickier because of streams, so I don't see why not try a different approach.

Reviewing the changes, also noted another part that could be simplified. Now instead of streams, or another method, simply calling #iterator() appears to work fine. WDYT?

Updated, ./gradlew clean test check passed after some 20 minutes locally, so hopefully Travis build will be OK as well.

@sebersole
Copy link
Member

sebersole commented Mar 23, 2018

simply calling #iterator() appears to work fine. WDYT?

I'm not sure what this means. Can you point me to an example of what you mean?

@kinow
Copy link
Contributor Author

kinow commented Mar 24, 2018

Sorry for not being clear on the iterator use @sebersole

I meant that instead of maintaining a mehod for iterable, used in only a couple of places, we can simply maintain an iterator method, and call .iterator ou iterables, keySet, etc... like in here https://github.com/kinow/hibernate-orm/blob/918a50326ff8c7b893edf6aee74638c82c10d29b/hibernate-core/src/main/java/org/hibernate/query/internal/ParameterMetadataImpl.java#L68

@sebersole sebersole merged commit ed575e4 into hibernate:master Mar 29, 2018
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.

2 participants