search

openrewrite / rewrite-migrate-java

OpenRewrite recipes for migrating to newer versions of Java.
Apache License 2.0
102 stars 71 forks source link

Use StringBuilder#isEmpty() instead of comparing size() #336

Open koppor opened 11 months ago

koppor commented 11 months ago

Similar to https://docs.openrewrite.org/recipes/staticanalysis/isemptycalloncollections:

-                        if ((current.length() > 0) && !Character.isWhitespace(current.charAt(current.length() - 1))
+                        if ((!current.isEmpty()) && !Character.isWhitespace(current.charAt(current.length() - 1))

Example: https://github.com/JabRef/jabref/pull/10489/files#diff-7a00bcdda16c9953e17ec2db403404b78ad1a750671c76bd7ec885fdf8ffa776

jeexan2 commented 11 months ago

Hello @koppor, can I pick this up? And if the answer is yes, can you add a bit more description regarding this? cc: @timtebeek

timtebeek commented 11 months ago

Hi @jeexan2 ! Sure; if you open up a draft pull request containing just the tests of what you want to cover, I'll then assign the issue to you. You can start with a test class similar to the one we have already for collections, but then adapted to fit StringBuilder. https://github.com/openrewrite/rewrite-static-analysis/blob/d6dee9de9eff8136f77b5cf04748db147b51f591/src/test/java/org/openrewrite/staticanalysis/IsEmptyCallOnCollectionsTest.java#L37-L75

timtebeek commented 11 months ago

Implementation-wise the isEmpty call on StringBuilder comes in through java.lang.CharSequence#isEmpty, so you'll want to target that in your recipe. The recipe itself can fit in right next to these existing String recipes in openrewrite/rewrite-migrate-java

timtebeek commented 11 months ago

Actually; come to think of it we already have such recipes in https://github.com/openrewrite/rewrite-migrate-java/blob/v2.1.1/src/main/java/org/openrewrite/java/migrate/lang/UseStringIsEmpty.java , but we need to broaden their matchers to use CharSequence instead of String!

timtebeek commented 11 months ago

The work still to do for this recipe then becomes:

Thanks a lot for the suggestion & offer to help!

timtebeek commented 11 months ago

@jeexan2 (or anyone else looking to pick this up) I've added some clickable links to the above comment; hope that helps!

gundamaiaha commented 10 months ago

@timtebeek I am a new contributer. Shall I pick this ?

timtebeek commented 10 months ago

That would be appreciated, yes please! Feel free to start with a draft pull request containing just a test, and then I'll assign the issue to you.

gundamaiaha commented 10 months ago

@timtebeek sure.

koppor commented 8 months ago

I tried replicating this one, but it seems this test already passes unexpectedly when added to

If I understand the test correctly, it assures that nothing happens if > 1 is used.

My request was a) for > 0 (!)

timtebeek commented 8 months ago

Ah yes, of course; I'm multitasking too much. This one is interesting. image

We could fit that in that way if we opt for the Java 9+ recipes we're adding in: