Skip to content

Replaced usage of String.to[U/L]Case() with String.equalsIgnoreCase()#5219

Merged
mbien merged 1 commit intoapache:masterfrom
tbw777:case
Feb 17, 2023
Merged

Replaced usage of String.to[U/L]Case() with String.equalsIgnoreCase()#5219
mbien merged 1 commit intoapache:masterfrom
tbw777:case

Conversation

@tbw777
Copy link
Contributor

@tbw777 tbw777 commented Jan 5, 2023

No needed to convert case to compare strings in different case. The better in memory and cpu way is to use equalsIgnoreCase() method.

JMH tests showing that String.equalsIgnoreCase() method faster then String.to[U/L]Case().equals() about several times.

@mbien mbien added Code cleanup Label for cleanup done on the Netbeans IDE ci:all-tests [ci] enable all tests labels Jan 6, 2023
@apache apache locked and limited conversation to collaborators Jan 6, 2023
@apache apache unlocked this conversation Jan 6, 2023
@mbien mbien added this to the NB17 milestone Jan 6, 2023
@mbien mbien self-requested a review January 6, 2023 07:35
@neilcsmith-net
Copy link
Member

neilcsmith-net commented Jan 6, 2023

These don't necessarily have the same behaviour, as switching from something that is Locale dependent to something that isn't?! Which might not be a bad thing, and is hopefully not actually relevant anywhere.

@tbw777
Copy link
Contributor Author

tbw777 commented Jan 6, 2023

I think here is no user input or data input from external sources

@neilcsmith-net
Copy link
Member

I think here is no user input or data input from external sources

?? What is this in reference to?

@neilcsmith-net neilcsmith-net modified the milestones: NB17, NB18 Jan 7, 2023
@tbw777
Copy link
Contributor Author

tbw777 commented Jan 7, 2023

What the potencial source of different locales?

@neilcsmith-net
Copy link
Member

to[U/L]Case() are Locale sensitive, equalsIgnoreCase() is not - check Javadoc notes for the 3. Not having behaviour that is potentially different dependent on the Locale settings of the end user is probably a good thing anyway. But this PR is a shift (fix) in behaviour as well as a possible performance improvement.

@mbien
Copy link
Member

mbien commented Feb 7, 2023

I looked through this and I believe this should be fine @neilcsmith-net. A lot of this are language identifier comparisons, file endings, SQL queries etc. Whenever there is a comparison with a constant like "dev" it shouldn't matter if the cases are compared with a specific local or not.

The only area I am not 100% sure about is the bugzilla package - maybe you could take a look at this one. If you aren't sure either we can skip it.

No needed to convert case to compare strings in different case. The better in memory and cpu way is to use equalsIgnoreCase() method.
@tbw777 tbw777 requested a review from mbien February 7, 2023 05:20
@mbien mbien requested a review from neilcsmith-net February 7, 2023 06:53
@neilcsmith-net
Copy link
Member

neilcsmith-net commented Feb 8, 2023

I looked through this and I believe this should be fine @neilcsmith-net. A lot of this are language identifier comparisons, file endings, SQL queries etc.

I think it looks fine too, although I haven't had time to look at all areas (like bugzilla) yet.

To be more explicit about what I was saying above, I think this should be evaluated as a bug fix. I'm more interested in that aspect of it than any minor performance improvements. And we should probably look for other uses of to[U/L]Case() that are not part of this but questionable, and should be using Locale.ROOT / Locale.ENGLISH.

Whenever there is a comparison with a constant like "dev" it shouldn't matter if the cases are compared with a specific local or not.

That depends on what is being compared, and where the input string is from. Consider -

String input = "LIB";
boolean result = "lib".equals(input.toLowerCase());

The result might be true or false depending on whether you're Turkish or not!

Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

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

lets get this in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:all-tests [ci] enable all tests Code cleanup Label for cleanup done on the Netbeans IDE

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants