eclipse-jdt / eclipse.jdt.ui

Eclipse Public License 2.0
38 stars 93 forks source link

Field Name Recommendation Based on Program-Specific Naming Conventions #661

Open DongChunHao opened 1 year ago

DongChunHao commented 1 year ago

Here we present the second heuristics to improve the name recommendation for Rename Field refactorings. Detailed explanations are provided below through examples.

The following code snippet comes from the apache/pinot (pinot-integration tests/src/test/java/com/linkedin/pinot/integration/tests/ HybridScanBasedCommandLineTestRunner.java, commit:31a6b95200cc5845706d27304fc2ed4767ec2aab). In this example, the original developers renamed the field "multiThreaded" to a new name "_multiThreaded" (Line 101) because all fields within the same class are named with common prefix "_" except for the original name of this field.

image

For the given example, IntelliJ IDEA recommends “aBoolean” instead, that is substantially different from the one chosen by the original developers.

As a generic rule, we recommend to append the common prefix to the original name (e.g., _multiThreded for multiThreded). We also suggest to remove the prefix of the original name if other fields within the same class do not contain the prefix.

We have validated the heuristic on 8489 field renamings actually conducted by the original developers in open-source applications. In total, the heuristic recommended 239 names where 512 were exactly the same as what the original developers finally used. That is , the precision is 47%, substantially higher than that of IntelliJ IDEA.

DongChunHao commented 1 year ago

@jjohnstn Please kindly have a look at the issue and the solution. Thanks.

jjohnstn commented 1 year ago

Hi @DongChunHao I think this change is very specific to a scenario you experienced but I don't feel it adds functionality to Eclipse JDT that would be useful to general users. For example, hitting CTRL+1 on B1 using your change:

public class TestRenaming {

    public int A1;
    public int A2;
    public int B1;

}

it recommends renaming B1 to AB1. That doesn't really make any sense. This clearly isn't a case of a naming convention. It is just a coincidence that most fields start with A.

Each quick-fix/assist needs to do calculations to determine if it is relevant and if a change can be created and this takes resources that ultimately adds up. Thus, we want quick-fixes/assists to provide value to general users, not just for a one-off scenario. A project-specific naming convention is just that: project-specific and doesn't apply to other projects/users. If a particular project wants all fields to be prefixed with a special prefix it can add scripts to verify code check-ins (e.g. github actions) or the project could run verification scripts occasionally that are published to the development team. To infer a naming convention by looking at code is a leap and isn't really feasible using frequency as shown by my simple example above.

robstryker commented 12 months ago

I could forsee a situation where a particular coding convention were set in a .settings file or property page, and then the quick fix only needs to check if this feature is enabled or not. This would certainly speed up the calculations for users who don't want to use the feature (a simple property check on the file) while allowing the logic to run for those who want it. A discussion on whether thats even advisable is at least worth something.

But I agree that in its current form it provides limited utility with a possible performance hit.