HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Validate input that maps int to enum

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
validateenummapsinputthatint

Problem

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 VALUE_TO_TEST_ENUM;
    static {
        final Map 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);
    }
}

Solution

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 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 = 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 = VALUE_TO_TEST_ENUM.length) {
         throw new NoSuchElementException("No enum with value '" + value + "'.");
    }

    return VALUE_TO_TEST_ENUM[value];
}

Code Snippets

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;
}
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;
}
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];
}

Context

StackExchange Code Review Q#156013, answer score: 8

Revisions (0)

No revisions yet.