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