Skip to content

Add RemovePrivateFieldUnderscores recipe #643

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add RemovePrivateFieldUnderscores recipe #643

wants to merge 6 commits into from

Conversation

jhl221123
Copy link

@jhl221123 jhl221123 commented Jul 19, 2025 •
edited by timtebeek
Loading

What's changed?

This recipe removes prefix or suffix underscores from private class field names. This helps enforce modern Java coding standards.

For clarity and consistency, the recipe also adds a qualifier to all updated field accesses. This prevents ambiguity with local variables and ensures a uniform style throughout the class.

What's your motivation?

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@github-project-automation github-project-automation bot moved this to In Progress in OpenRewrite Jul 19, 2025
@greg-at-moderne greg-at-moderne mentioned this pull request Jul 22, 2025
Open
@timtebeek
Copy link
Member

Let us know if you'd need any guidance @jhl221123 ! Perhaps this session could help you get started:
https://www.moderne.ai/content-library/introduction-to-openrewrite

@jhl221123
Add RemovePrivateFieldUnderscores recipe
0e9ede6 
This recipe removes prefix or suffix underscores from private class field names.
This helps enforce modern Java coding standards.

For clarity and consistency, the recipe also adds a qualifier to all updated field accesses.
This prevents ambiguity with local variables and ensures a uniform style throughout the class.

Fixes openrewrite#630
@jhl221123 jhl221123 force-pushed the remove-private-field-underscores branch from 264ca2a to 0e9ede6 Compare August 4, 2025 10:23
@jhl221123 jhl221123 marked this pull request as ready for review August 4, 2025 10:25
timtebeek
timtebeek reviewed Aug 22, 2025
View reviewed changes
src/test/java/org/openrewrite/staticanalysis/RemovePrivateFieldUnderscoresTest.java
java(
"""
public class ParseLocation {
private String _ruleName;
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to add a test showing either class_ or _class to show we do not end up renaming to restricted keywords.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for the delay. I've added the test case!

timtebeek and others added 2 commits August 22, 2025 21:11
@timtebeek @github-actions
Update src/main/java/org/openrewrite/staticanalysis/RemovePrivateFiel…
49f319d 
…dUnderscores.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@jhl221123
Add test case for reserved keyword handling
4f72198
dsgrieve
dsgrieve reviewed Sep 5, 2025
View reviewed changes
src/test/java/org/openrewrite/staticanalysis/RemovePrivateFieldUnderscoresTest.java Outdated
private String ruleName;

public String getRuleName() {
return this.ruleName;
Copy link

@dsgrieve dsgrieve Sep 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

this is unnecessary where there is no ambiguity. I think a recipe should do one thing which, in this case, is remove the underscore. IMO, this behavior should be removed from the visitor.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback! Your point about recipes having a single responsibility makes a lot of sense. I'll get this updated shortly.

@dsgrieve
Copy link

dsgrieve commented Sep 5, 2025

Another case:

String foo(String bar) {
    String _bar = bang(bar); 
    // ... do something else with bar ... 
    return _bar;
}

In this case, I would expect no change.

@jhl221123
Refine recipe behavior based on feedback
ccfaec0 
- Qualify with this. only when a field is shadowed by a local variable or parameter.
- Align tests with the new behavior and improve their clarity.
@jhl221123
Copy link
Author

Thanks for the detailed feedback, @dsgrieve. I believe I've addressed everything now. Ready for another look when you have a moment.

@dsgrieve
Copy link

dsgrieve commented Sep 8, 2025

Sorry. I keep stumbling upon use cases. Be aware of JEP 456 which allows an _ to be used as an un-named variable.

@jhl221123
Prevent renaming fields to the unnamed variable
ed84a1c 
- Skips renaming if the resulting name would be empty, _, or still start/end with an underscore.
@jhl221123
Copy link
Author

@dsgrieve No problem at all, and thank you for catching these important edge cases! I've pushed an update to handle it. Please let me know if you spot anything else.

dsgrieve
dsgrieve reviewed Sep 9, 2025
View reviewed changes
Copy link

@dsgrieve dsgrieve left a comment

Choose a reason for hiding this comment

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

LGTM, but I defer to @timtebeek for approval.

@timtebeek timtebeek self-requested a review September 9, 2025 16:11
greg-at-moderne
greg-at-moderne reviewed Sep 10, 2025
View reviewed changes
Copy link
Contributor

@greg-at-moderne greg-at-moderne left a comment

Choose a reason for hiding this comment

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

Another test case worth adding is using underscore in the middle of the name like:
String first_name;. No change expected then.

greg-at-moderne
greg-at-moderne reviewed Sep 10, 2025
View reviewed changes
src/main/java/org/openrewrite/staticanalysis/RemovePrivateFieldUnderscores.java
return id;
}

private boolean isAmbiguous() {
Copy link
Contributor

@greg-at-moderne greg-at-moderne Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I am wondering if it wouldn't be better to re-use the existing logic of org.openrewrite.java.VariableNameUtils#generateVariableName. I'd call it with the newName as the baseName and refrain from any changes if the result is different.

This way:

  • the code is shorter here
  • and likely more thoroughly tested / better coverage of the naming logic

@greg-at-moderne
Copy link
Contributor

Nice work on the recipe.
Don't worry about the timing, we are not in a hurry in any way ;)

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

@dsgrieve dsgrieve dsgrieve left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

@greg-at-moderne greg-at-moderne greg-at-moderne left review comments

@timtebeek timtebeek Awaiting requested review from timtebeek

Assignees

@jhl221123 jhl221123

Labels
recipe
Projects
OpenRewrite
Status: In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Recipe to remove underscores in private field names
4 participants
@jhl221123 @timtebeek @dsgrieve @greg-at-moderne