patternjavaMinor
Validate input that maps int to enum
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
Additionally, you're using a small range of 0-based values for the
About the exception - yes, I would throw a
There is no need to make the
If your values can span a (very) wide range I would keep your Map-based lookup, but change the code to be:
If your values are in a small range, at, or close to 0, I would do:
If your test values are from 0 to
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.