eclipse-jdt / eclipse.jdt.ui

Eclipse Public License 2.0
37 stars 90 forks source link

String support for if/else to switch cleanup #1514

Closed carstenartur closed 3 months ago

carstenartur commented 3 months ago

What it does

Since Java 7 there is support for "switch" statement making use of "String" objects. The cleanup for that in eclipse jdt ui so far only supports primitive types like char, int and so on. I just want to see what is needed to add support for the cleanup to refactor if/else chains with String constant comparisons to switch statements as well. It does not check the projects java version. Not sure if that makes sense any longer. Do we support java 6 today? In an ideal world there should be at least to tests in a java 6 test configuration and a java 7 test configuration. The currently used "CleanUpTest12" seems to be somehow random.

How to test

There is a single junit test so far. You can add testcases or use e.g. the jdt codebase for testing.

Author checklist

jjohnstn commented 3 months ago

Thanks @carstenartur . We just removed support for < Java 1.8 so JVM check is not needed. Not sure about the history of why all of this is in CleanUpTest12, but it doesn't matter.

carstenartur commented 3 months ago

Hi Jeff, please go ahead and create a N&N entry if you feel like doing it. Btw, should support for "enum" be added in this cleanup one day? Maybe it is pointless because everybody prefers "switch" beforehand when using enum anyway and so there is are no cases to cleanup but I'm not sure.

carstenartur commented 3 months ago

I do not see how to check for an "enum". The approach used in the other cases does not work.

jjohnstn commented 3 months ago

Hi @carstenartur Thanks for the changes. Enum support would be fine but as you mentioned, it is likely a user would have used a switch to begin with. To find an enum, the ITypeBinding has an isEnum() method. For an enum value, doing a resolveBinding() of the name will give an IVariableBinding which has isEnumConstant() method.

jjohnstn commented 3 months ago

@carstenartur I'll do the N&N for 4.33 M2.