Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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);
    }
}
share|improve this question

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];
}
share|improve this answer

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));
}
share|improve this answer
1  
An EnumMap won't help, I don't think, but the Optional return value is a good suggestion. +1 – rolfl 17 hours ago

I think null is reasonable and that trying to avoid this is over engineering. However if you must return a value I would do it this way.

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));
    }
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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