Error-prone Java enum refactoring

Question

I'm refactoring some old code to use enum's instead of String constants. I was reviewing my code when I noticed that comparing enum to String won't throw an exception. I can't delete the old constants because other projects are still using them.

I can't override the equals because JLS specifically forbids this:

The equals method in Enum is a final method that merely invokes super.equals on its argument and returns the result, thus performing an identity comparison.

The code looks like this:

public enum Gender{
    MALE,
    FEMALE
}

// Constants for genders
public static final String MALE = "Male";
public static final String FEMALE = "Female";

//following are obviously false
MALE.equals(Gender.MALE) 
Gender.MALE.equals(MALE)

For a regular object I could override equals and throw an exception but for my example it will just return false. Also there was a method like getGender that was returning string and now is return an enum so there can be places I missed and a string is compared to enum

This is error-prone. FindBugs is also not reporting any bug. Is there anyway I can protect against this?

But don't you have type checking where you pass these constants? I.e. methods that expect `String` will compile-time shout that you are passing an enum to them? Or are you just using `Object` or raw lists? — RealSkeptic, Aug 10 '15 at 20:35
there was a method like getGender and was returning a string; now it's returning an enum so the code still works — user1995187, Aug 10 '15 at 20:38
@RealSkeptic You could pass `CONSTANT.name()`. But if the method expects a `String`, you're not fully realizing the advantages of using `enum`. The method should expect a `Gender`. — Kevin Krumwiede, Aug 10 '15 at 20:38
@HovercraftFullOfEels yes but for an regular object you can catch this in equals and throw an exception; for an enum I can't; it will just return false — user1995187, Aug 10 '15 at 20:41
@KevinKrumwiede I think you mistake what I ask. I am not thinking of a way to bypass the incompatibility - that's not what the OP is asking. I'm thinking of a way to use type safety to help prevent such a mixup. — RealSkeptic, Aug 10 '15 at 20:42
"for an regular object you can catch this in equals and throw an exception" - You can, but you shouldn't. When passed the wrong type, your overridden `equals(...)` methods should return `false` just like `enum`'s does. — Kevin Krumwiede, Aug 10 '15 at 20:47
I cannot tell what you're really asking for. Do you want a way to write this "refactoring" class so that the confusion doesn't happen? Do you want a way to separate the use of the enum from the uses of the strings? Do you want a way to write this so that uses of the String will show errors when they start getting mixed with the use of the enum? What? — arcy, Aug 10 '15 at 20:48
Throwing an exception (other than NPE) from `equals` definitely breaks the principle of least astonishment (and the [contract](http://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#equals(java.lang.Object))). — Mick Mnemonic, Aug 10 '15 at 20:48
@arcy just a way of catching if I missed something like obj.getGender().equals("Male") when getGender returns an enum — user1995187, Aug 10 '15 at 20:52
@KevinKrumwiede is completely correct: you should never be throwing exceptions from equals. — Hovercraft Full Of Eels, Aug 10 '15 at 21:00
@MickMnemonic It shouldn't even throw NPE. If you pass `null`, it should just return `false`. That's what SDK classes do, and what `equals(...)` methods generated by Eclipse do. — Kevin Krumwiede, Aug 11 '15 at 02:07
@KevinKrumwiede, I agree that the contract doesn't mention throwing a NPE and doing so would be against the convention. However, one could also argue that the contract itself is broken. Good discussion on the topic in [Is it a bad idea if equals(null) throws NullPointerException instead](http://stackoverflow.com/questions/2887761/is-it-a-bad-idea-if-equalsnull-throws-nullpointerexception-instead). — Mick Mnemonic, Aug 11 '15 at 11:34

Kevin Krumwiede · Accepted Answer · 2015-08-11T02:18:10.323

5

As noted in the comments, Object#equals(...) is not typesafe. There's nothing you can do to prevent users of your API from passing it an object of the wrong type. In that situation, it should simply return false. If someone does this, eventually they'll notice that it's always returning false and go looking for the bug.

You should deprecate the String constants to draw attention to the preferred way of doing things:

/**
 * New code should use {@link Gender#MALE}.
 */
@Deprecated
public static final String MALE = "Male";
/**
 * New code should use {@link Gender#FEMALE}.
 */
@Deprecated
public static final String FEMALE = "Female";

edited Aug 11 '15 at 02:18

answered Aug 10 '15 at 20:43

Kevin Krumwiede

9,868
4
34
82

1

Compiling with `javac -Xlint:deprecation` will issue warnings on all uses of `@Deprecated` items (and there are also options for these in Eclipse etc.), which will help eliminate missed occurrences where the string was not replaced with the enum. – RealSkeptic Aug 11 '15 at 16:22

nerdy900 · Answer 2 · 2015-08-10T20:48:19.070

I would do something like this:

public enum Gender {
    MALE("Male"),
    FEMALE("Female");

    private final String val;

    Gender(String val) {
        this.val = val;
    }

    public static Gender getEnum(String value) {
        for (Gender a : values()) {
            if (a.getVal().equalsIgnoreCase(value)) {
                return a;
            }
         }
         return throw new IllegalArgumentException("no gender known");
    }

     public String getVal() {
         return val;
     }
}

It allows you to create an instance of the enum from a string and then compare the enum

Gender genderFromString = Gender.getEnum(someString);
genderFromString.equals(Gender.MALE);

By converting the string to an instance of the enum and then comparing the 2 enums, a more reliable result is obtained

score 1 · Answer 3 · answered Aug 10 '15 at 20:38

1

You could move the final String variables into a separate class, say StringConstants.

public class StringConstants {
    public static final String MALE = "Male";
    public static final String FEMALE = "Female";
}

Then in your code it would be a more obvious mistake when someone tries to compare StringConstants.MALE to Gender.MALE.

Also, renaming the enum to GenderEnum could help even further, because then it would be even more obvious that you don't want to do GenderEnum.MALE.equals(StringConstants.MALE).

answered Aug 10 '15 at 20:38

Andrew Mairose

10,615
12
60
102

Moving them would break source compatibility. The OP mentions that other projects are using these constants, so that would be very bad. – Kevin Krumwiede Aug 11 '15 at 05:30
True, but with most modern IDEs, it would be quite simple to find all usages of the String constants and prepend `StringConstants`. – Andrew Mairose Aug 11 '15 at 05:43
You should never force other developers to do that. Once a public API is out of alpha, you should consider it carved in stone. Changing it looks extremely incompetent and unprofessional. The only time you should ever make breaking changes to a public API is when you bump the major version number. And even then, only do it when necessary to enable functional changes, never just to reorganize or rename things. – Kevin Krumwiede Aug 11 '15 at 06:30

Error-prone Java enum refactoring

3 Answers3