8
\$\begingroup\$

I've written the following enum and have added a function, fromValue that allows the caller to map given int into the enum value. I was wondering if the value passed to the function can be validated, or is returning a null in case the value isn't present in the map (is an invalid enum) sufficient?

public enum TestEnum {

    A(0x00),
    B(0x01),
    C(0x02);

    int test;

    private static final Map<Integer, TestEnum> VALUE_TO_TEST_ENUM;
    static {
        final Map<Integer, TestEnum> tmpMap = new HashMap<>();
        for (TestEnum testEnum : TestEnum.values()) {
            tmpMap.put(testEnum.test, testEnum);
        }
        VALUE_TO_TEST_ENUM = ImmutableMap.copyOf(tmpMap);
    }

    TestEnum(final int test) {
        this.test = test;
    }

    public static TestEnum fromValue(final int value) {
        // Add validation?
        return VALUE_TO_TEST_ENUM.get(value);
    }
}
\$\endgroup\$
8
\$\begingroup\$

Every instance in an enum already has an ordinal (the 0-based position of the value in the declaration order of the enum). For example, your instance C.ordinal() will return 2. See: Enum.ordinal(). These are the same values as the ones you are assigning to test. Is that a coincidence?

Additionally, you're using a small range of 0-based values for the test field, and as a consequence, an array will be a better storage option than a Map. Even if the array is as much as 80% empty it would still be more efficient (space and performance) than the Map.

About the exception - yes, I would throw a NoSuchElementException if the user tries to get a value that does not exist. Enums are compile-time constants and any use of the enum that's not legal should be reported, and found as soon as possible. In a sense, it's for this reason that Enums exist - to give compile-time certainty that your code references meaningful constants. The very fact that you are mapping the enum values back to an int is itself a bit concerning.

There is no need to make the Map a read-only map. The map is completely contained/encapsulated in the enum and no other write accesses exist, and no user can write to it, so it's redundant to make it read-only.

If your values can span a (very) wide range I would keep your Map-based lookup, but change the code to be:

private static final Map<Integer, TestEnum> VALUE_TO_TEST_ENUM = new HashMap<>();
static {
    for (TestEnum testEnum : TestEnum.values()) {
        tmpMap.put(testEnum.test, testEnum);
    }
}

public static TestEnum fromValue(final int value) {
    // Add validation?
    TestEnum v = VALUE_TO_TEST_ENUM.get(value);
    if (v == null) {
         throw new NoSuchElementException("No enum with value '" + value + "'.");
    }
    return v;
}

If your values are in a small range, at, or close to 0, I would do:

private static final TestEnum[] VALUE_TO_TEST_ENUM;
static {
    int max = 0;
    for (TestEnum testEnum : TestEnum.values()) {
        max = Math.max(max, testEnum.test);
    }
    VALUE_TO_TEST_ENUM = new int[max + 1];
    for (TestEnum testEnum : TestEnum.values()) {
        VALUE_TO_TEST_ENUM[testEnum.test] = testEnum;
    }
}

public static TestEnum fromValue(final int value) {
    // Add validation?
    if (value < 0 || value >= VALUE_TO_TEST_ENUM.length) {
         throw new NoSuchElementException("No enum with value '" + value + "'.");
    }

    TestEnum v = VALUE_TO_TEST_ENUM[value];
    if (v == null) {
         throw new NoSuchElementException("No enum with value '" + value + "'.");
    }
    return v;
}

If your test values are from 0 to n-1 and are the same as the ordinals of the enums, then I would completely get rid of the test value, and have the code:

private static final TestEnum[] VALUE_TO_TEST_ENUM;
static {
    VALUE_TO_TEST_ENUM = TestEnum.values();
}

public static TestEnum fromValue(final int value) {
    // Add validation?
    if (value < 0 || value >= VALUE_TO_TEST_ENUM.length) {
         throw new NoSuchElementException("No enum with value '" + value + "'.");
    }

    return VALUE_TO_TEST_ENUM[value];
}
\$\endgroup\$
  • \$\begingroup\$ This is great! Thanks for the explanation. The values I'm assigning are test values; they're not necessarily same as the ordinal. The values I'm using are broad ranging, they're based on an RFC. Will throw NoSuchElementException. \$\endgroup\$ – user1071840 Feb 23 '17 at 11:30
5
\$\begingroup\$

If the ints are fact ordinals, you should maybe consider using an EnumMap. If not, your current approach is fine.

For fromValue, you could consider returning an Optional<TestEnum> instead of the value, null or throwing some exception. I've been using this pattern in my own enums.

public static Optional<TestEnum> fromValue(final int value) {
    // Add validation?
    return Optional.ofNullable(VALUE_TO_TEST_ENUM.get(value));
}
\$\endgroup\$
  • 1
    \$\begingroup\$ An EnumMap won't help, I don't think, but the Optional return value is a good suggestion. +1 \$\endgroup\$ – rolfl Feb 22 '17 at 17:26
1
\$\begingroup\$

It depends on semantics and assertions.

If your semantic is that any other value that 1 and 2 leads to the UNDEFINED constant AND you algorithms work then you will go fine with Martin Spamers solution.

Returning null or an Optional is equally good or bad if the reason to return uncertainty is bad. First you should go for a solution without returning null. In general it is a good idea to have a full covered enumeration. With that I mean that any case you can think of will always fall into exactly one of the categories the enum provides.

If you have a real 1:1 relationship between the value and the enum constant then you should strictly follow semantics. If an enum constant does not exists for a value then throw an exception or return null.

Returning an Optional should be considered carefully. For me an Optional is not an substitute for return a null value. For me it is something that states that something may be there or not NOT depending on any input parameter that provided by the caller. So you should use Optional to express a case when something currently is not available but you may come back later and the requested object will be there. And furthermore this object may disappear again in the future. Optional should be used if the requested object is nice to have but not essential. The process may not be that efficient but it will still do its job.

If you have a 1:1 relationship from value to enum constant and this is defined by an RFC you can assume that a missing enum constant for a value will either never comes available. Of course an enum constant could be added. But that is not what I mean. The question is: can the enum constant be resolved by an unknown value potentially at runtime? The answer is: No. That is why returning null or throwing an exception is appropriate and you should not use Optional.

\$\endgroup\$
1
\$\begingroup\$

I think null is reasonable if you consider its semantics to include being undefined that trying to avoid null often results in over engineering solutions. My solution includes a much more elegant and reusable approach to fromValue() function that can be readily used with numeric or String value enums.

However if you must return some value I would do it this way, simply create your own version of undefined. This is the solution I used when I want something that amounts to PENDING definition at the user data level.

public class EnumTest {
    public enum TestEnum {
        UNDEFINED(0),
        A(1),
        B(2);

        private final int number;
        private TestEnum(final int coValue) {
            this.number = coValue;
        }
        public static TestEnum fromValue(final int coValue) {
            for (final TestEnum value : values()) {
                if (value.number == coValue) {
                    return value;
                }
            }
            // return null; or
            return UNDEFINED;
        }
    }

    @Test
    public void testMissing() {
        assertEquals(TestEnum.UNDEFINED, TestEnum.fromValue(Integer.MIN_VALUE));
        assertEquals(TestEnum.UNDEFINED, TestEnum.fromValue(0));
        assertEquals(TestEnum.A, TestEnum.fromValue(1));
        assertEquals(TestEnum.B, TestEnum.fromValue(2));
        assertEquals(TestEnum.UNDEFINED, TestEnum.fromValue(Integer.MAX_VALUE));
    }
}
\$\endgroup\$
0
\$\begingroup\$

Just define

public enum TestEnum { A, B, C }

Everything else is built-in - you have useful methods like ordinal, valueOf, values.

To find constant at index 1 you write:

TestEnum.values()[1]
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.