Skip to content

Conversation

@AlekSimpson
Copy link
Contributor

…a string with a StringBuilder

What's changed?

This PR adds a new recipe that finds when .equals() is being used to compare a string and a StringBuilder, StringBuffer or CharSequence.

What's your motivation?

#22

Anything in particular you'd like reviewers to focus on?

I think the precondition checks should be correct but maybe they could be narrowing things down more.

Anyone you would like to review specifically?

@timtebeek

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've added the license header to any new files through ./gradlew licenseFormat
  • I've used the IntelliJ auto-formatter on affected files
  • I've updated the documentation (if applicable)

@AlekSimpson AlekSimpson requested a review from timtebeek June 16, 2023 21:10
@AlekSimpson AlekSimpson self-assigned this Jun 16, 2023
@AlekSimpson AlekSimpson marked this pull request as draft June 16, 2023 21:10
Comment on lines 74 to 78
Stream<JavaType> TYPES = Stream.of(
JavaType.buildType("java.lang.StringBuilder"),
JavaType.buildType("java.lang.StringBuffer"),
JavaType.buildType("java.lang.CharSequence")
);
Copy link
Member

Choose a reason for hiding this comment

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

We can probably also move these to a field above.

Copy link
Member

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

In terms of tests it would be good to have cases where either the first or the second select is not a string variable or constant, but for instance a method invocation, to be sure we handle those fairly predictable scenarios well.

@@ -0,0 +1,87 @@
/*
* Copyright 2021 the original author or authors.
Copy link
Member

Choose a reason for hiding this comment

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

Might want to adjust ./gradle/licenceHeader.txt and this here to set the right year.

@AlekSimpson
Copy link
Contributor Author

I added that test for if the recipe will run correctly on selects that are a method call.

@AlekSimpson AlekSimpson marked this pull request as ready for review June 17, 2023 18:08
@timtebeek
Copy link
Member

Great work! I've added another polish commit in 18287b2; with changes such as

  • Use markdown in getDisplayName() and getDescription() around code elements, for better readability in the docs
  • Remove nested if statements in favor of early returns; this helps keep the code readable
  • Minimize use of loops through a wildcard matcher on java.lang.* toString() and TypeUtils.isAssignableTo("java.lang.CharSequence", JavaType); we try to avoid Streams for performance reasons (fewer object allocations), and it leads less code as well.
  • Minimize public visibility in tests

Hope you don't mind, and I look forward to the next one!

@timtebeek timtebeek merged commit e0b54ca into main Jun 17, 2023
@timtebeek timtebeek deleted the alek/EqualsToContentEquals branch June 17, 2023 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants