10
\$\begingroup\$

I have an utility class to handle EnumMap-related features, such as creating/converting between Object -> Enum and Enum -> Object maps. To better my understanding around lambda expressions in Java, I have used a great deal of streams, collectors and functions, and I'm not sure if I have gone too overboard...

  • Is anything else I can make the code and unit test more Java-8-like?
  • I am still using new EnumMap<>(...) and new HashMap<>() in certain parts of code, as I cannot seem to easily replace them with a Supplier type for the collectors. Is there anything I can do to improve this aspect? In other words, I will like to pass them into my doMap() method, instead of putting the newly created Map from the method call into my new EnumMap/HashMap instances.
  • I am using an Enum to represent the test cases, and then using TestNG's DataProviders to iterate through them. My motivations are:

    • by overriding the toString() implementation, I have a fairly expressive way of providing descriptions for the test cases,
    • they serve nothing more than a container for actual (or what I like to refer to as result) and expected values, and
    • there is some practical elimination of duplicate @Test annotations and method declarations.
  • Are there any other downsides that I am not aware of, for choosing this non-conventional approach?

  • Please review the Javadocs and the overall readability of the main and test classes too, as I will like to know if I am being too verbose in these departments.

EnumMapUtils

import java.lang.reflect.InvocationTargetException;
import java.util.EnumMap;
import java.util.HashMap;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.Stream;

/**
 * An utilities class to handle {@link Enum}-related {@link Map}s.
 */
public final class EnumMapUtils {

    public static final class DuplicateKeysException extends RuntimeException {

        private static final long serialVersionUID = 6433540910901212502L;

        public DuplicateKeysException() {
            super("Key mapper has produced duplicate keys: ");
        }
    }

    /**
     * Private constructor for utility class.
     */
    private EnumMapUtils() {
        // intentionally blank
    }

    /**
     * Given a {@link Set}<code>&lt;T&gt;</code>, use two {@link Function}s to derive keys of type
     * <code>K</code> and values of type <code>V</code> where <code>K &#8594; V</code>.
     * <p>
     * The merge function used to resolve collisions between values associated with the same key
     * picks the later one.
     *
     * @param set the {@link Set} to stream on.
     * @param keyMapper the {@link Function} to use for deriving keys of type <code>K</code> from
     *            the {@link Set}'s elements.
     * @param valueMapper the {@link Function} to use for deriving values of type <code>V</code>
     *            from the {@link Set}'s elements.
     * @param checkDuplicateKeys <code>true</code> if a strict check on duplicate keys is required,
     *            by comparing the resulting {@link Map#size()} with <code>set.size()</code>.
     * @return a {@link Map} with mappings <code>K &#8594; V</code>.
     * @see Collectors#toMap(Function, Function, java.util.function.BinaryOperator)
     */
    private static <T, K, V> Map<K, V> doMap(final Set<T> set, final Function<T, K> keyMapper,
            final Function<T, V> valueMapper, boolean checkDuplicateKeys) {
        final Map<K, V> innerResult = set.stream().collect(
                Collectors.toMap(keyMapper, valueMapper, (earlier, later) -> later));
        if (checkDuplicateKeys && innerResult.size() != set.size()) {
            throw new DuplicateKeysException();
        }
        return innerResult;
    }

    /**
     * Gets the {@link Enum}'s values via reflection. All checked {@link Exception}s are wrapped and
     * thrown as {@link RuntimeException}s.
     *
     * @param forEnum the {@link Enum} to represent.
     * @return a {@link Set} containing the {@link Enum}'s values.
     */
    private static <E extends Enum<E>> Set<E> getEnumValues(final Class<E> forEnum) {
        validateArguments(forEnum);
        try {
            return Stream.of((E[]) forEnum.getDeclaredMethod("values").invoke(forEnum)).collect(
                    Collectors.toSet());
        } catch (NoSuchMethodException | SecurityException | IllegalAccessException
                | IllegalArgumentException | InvocationTargetException e) {
            throw new RuntimeException(e);
        }
    }

    /**
     * This method exists solely to throw {@link IllegalArgumentException} instead of
     * {@link NullPointerException}, when encountering <code>null</code> arguments.
     *
     * @param args the arguments to check for <code>null</code>.
     */
    private static void validateArguments(final Object... args) {
        if (args == null) {
            throw new IllegalArgumentException();
        }
        for (final Object o : args) {
            if (o == null) {
                throw new IllegalArgumentException();
            }
        }
    }

    /**
     * Reverses the <code>T &#8594; E</code> mappings of {@link Map}<code>&lt;T, E&gt;</code>
     * <em>while considering</em> the possibility of duplicate <code>E &#8594; T</code> mappings. As
     * such, the values of the resulting {@link Map} are of type {@link Set}<code>&lt;T&gt;</code>.
     * <p>
     * Internally, {@link EnumMap} is the implementation for the resulting {@link Map}, and values
     * for the {@link Set} are accumulated with {@link Collectors#toSet()}.
     *
     * @param forEnum the {@link Enum} to represent.
     * @param map the {@link Map} with mappings <code>T &#8594; E</code>.
     * @return a {@link Map} with mappings <code>E &#8594; Set&lt;T&gt;</code>.
     * @see #convertToSimpleEnumMap(Map)
     * @see Collectors#groupingBy(Function, java.util.stream.Collector)
     * @see Collectors#toSet()
     */
    public static <T, E extends Enum<E>> Map<E, Set<T>> convertToEnumMap(final Class<E> forEnum,
            final Map<T, E> map) {
        validateArguments(forEnum, map);
        final Map<E, Set<T>> result = new EnumMap<>(forEnum);
        result.putAll(map
                .entrySet()
                .stream()
                .collect(
                        Collectors.groupingBy(Entry::getValue,
                                Collectors.mapping(Entry::getKey, Collectors.toSet()))));
        return result;
    }

    /**
     * Reverses the <code>T &#8594; E</code> mappings of {@link Map}<code>&lt;T, E&gt;</code>
     * <em>without considering</em> the possibility of duplicate <code>E &#8594; T</code> mappings.
     * As such, the values of the resulting {@link Map} are of type <code>T</code>, with mappings
     * streamed later over-riding the earlier ones.
     * <p>
     * Internally, {@link EnumMap} is the implementation for the resulting {@link Map}.
     *
     * @param forEnum the {@link Enum} to represent.
     * @param map the {@link Map} with mappings <code>T &#8594; E</code>.
     * @return a {@link Map} with mappings <code>E &#8594; T</code>.
     * @see #convertToEnumMap(Class, Map)
     */
    public static <T, E extends Enum<E>> Map<E, T> convertToSimpleEnumMap(final Map<T, E> map) {
        validateArguments(map);
        return new EnumMap<>(doMap(map.entrySet(), Entry::getValue, Entry::getKey, false));
    }

    /**
     * Creates a {@link Map} with mappings <code>E &#8594; T</code>, where values are derived using
     * a {@link Function}.
     *
     * @param forEnum the {@link Enum} to represent.
     * @param enumMapper the {@link Function} to use to derive the values for the resulting
     *            {@link Map}.
     * @return a {@link Map} with mappings <code>E &#8594; T</code>.
     * @see #createEnumMap(Set, Function)
     */
    public static <T, E extends Enum<E>> Map<E, T> createEnumMap(final Class<E> forEnum,
            final Function<E, T> enumMapper) {
        return createEnumMap(getEnumValues(forEnum), enumMapper);
    }

    /**
     * Creates a {@link Map} with mappings <code>E &#8594; T</code>, where values are derived using
     * a {@link Function}.
     * <p>
     * Internally, {@link EnumMap} is the implementation for the resulting {@link Map}.
     *
     * @param enumSet the {@link Set} of {@link Enum} to represent.
     * @param enumMapper the {@link Function} to use to derive the values for the resulting
     *            {@link Map}.
     * @return a {@link Map} with mappings <code>E &#8594; T</code>.
     */
    public static <T, E extends Enum<E>> Map<E, T> createEnumMap(final Set<E> enumSet,
            final Function<E, T> enumMapper) {
        validateArguments(enumSet, enumMapper);
        return new EnumMap<>(doMap(enumSet, Function.identity(), enumMapper, false));
    }

    /**
     * Creates a {@link Map} with <code>E</code> as the keys and E.{@link #toString()} for the
     * values.
     *
     * @param forEnum the {@link Enum} to represent.
     * @return a {@link Map} with mappings <code>E &#8594; String</code>.
     * @see #createEnumMap(Set)
     */
    public static <E extends Enum<E>> Map<E, String> createEnumMap(final Class<E> forEnum) {
        return createEnumMap(getEnumValues(forEnum));
    }

    /**
     * Creates a {@link Map} with <code>E</code> as the keys and E.{@link #toString()} for the
     * values.
     *
     * @param enumSet the {@link Set} of {@link Enum} to represent.
     * @return a {@link Map} with mappings <code>E &#8594; String</code>.
     * @see #createEnumMap(Class, EnumMapper)
     */
    public static <E extends Enum<E>> Map<E, String> createEnumMap(final Set<E> enumSet) {
        return createEnumMap(enumSet, (value) -> value.toString());
    }

    /**
     * Creates a {@link Map} with mappings <code>T &#8594; E</code>, where the keys are derived
     * using a {@link Function}.
     * <p>
     * Internally, {@link HashMap} is the implementation for the resulting {@link Map}.
     *
     * @param forEnum the {@link Enum} to represent.
     * @param enumMapper the {@link Function} to use for deriving the {@link Map}'s keys.
     * @return a {@link Map} with mappings <code>T &#8594; E</code>.
     * @see #createReverseEnumMap(Set, Function)
     * @throws DuplicateKeysException if the <code>enumMapper</code> produces duplicate keys.
     */
    public static <T, E extends Enum<E>> Map<T, E> createReverseEnumMap(final Class<E> forEnum,
            final Function<E, T> enumMapper) {
        return createReverseEnumMap(getEnumValues(forEnum), enumMapper);
    }

    /**
     * Creates a {@link Map} with mappings <code>T &#8594; E</code>, where the keys are derived
     * using a {@link Function}.
     * <p>
     * Internally, {@link HashMap} is the implementation for the resulting {@link Map}.
     *
     * @param enumSet the {@link Set} of {@link Enum} to represent.
     * @param enumMapper the {@link Function} to use for deriving the {@link Map}'s keys.
     * @return a {@link Map} with mappings <code>T &#8594; E</code>.
     * @see #modifyReverseEnumMap(Set, Function, Map)
     * @throws DuplicateKeysException if the <code>enumMapper</code> produces duplicate keys.
     */
    public static <T, E extends Enum<E>> Map<T, E> createReverseEnumMap(final Set<E> enumSet,
            final Function<E, T> enumMapper) {
        return modifyReverseEnumMap(enumSet, enumMapper, new HashMap<>());
    }

    /**
     * Creates a {@link Map} with <code>E</code>.{@link #toString()} for the keys and <code>E</code>
     * as the values.
     *
     * @param forEnum the {@link Enum} to represent.
     * @return a {@link Map} with mappings <code>String &#8594; E</code>.
     * @see #createReverseEnumMap(Set)
     * @throws DuplicateKeysException if <code>E</code>.{@link #toString()} produces duplicate keys.
     */
    public static <E extends Enum<E>> Map<String, E> createReverseEnumMap(final Class<E> forEnum) {
        return createReverseEnumMap(getEnumValues(forEnum));
    }

    /**
     * Reverses the <code>E &#8594; T</code> mappings of <code>map</code>.
     *
     * @param map the {@link Map} to derive the mappings from.
     * @return a {@link Map} with mappings <code>T &#8594; E</code>.
     * @throws DuplicateKeysException if there is more than one <code>E &#8594; T</code> mapping,
     *             producing duplicate keys.
     */
    public static <T, E extends Enum<E>> Map<T, E> createReverseEnumMap(final Map<E, T> map) {
        validateArguments(map);
        return doMap(map.entrySet(), Entry::getValue, Entry::getKey, true);
    }

    /**
     * Creates a {@link Map} with <code>E</code>.{@link #toString()} for the keys and <code>E</code>
     * as the values.
     *
     * @param enumSet the {@link Set} of {@link Enum} to represent.
     * @return a {@link Map} with mappings <code>String &#8594; E</code>.
     * @see #createReverseEnumMap(Set, Function)
     * @throws DuplicateKeysException if <code>E</code>.{@link #toString()} produces duplicate keys.
     */
    public static <E extends Enum<E>> Map<String, E> createReverseEnumMap(final Set<E> enumSet) {
        return createReverseEnumMap(enumSet, (value) -> value.toString());
    }

    /**
     * Modifies a {@link Map} by putting mappings <code>T &#8594; E</code>, where keys are derived
     * using a {@link Function}.
     *
     * @param forEnum the {@link Enum} to represent.
     * @param enumMapper the {@link Function} to use for deriving the {@link Map}'s keys.
     * @param result the {@link Map} to put the mappings to.
     * @return the <code>result</code> {@link Map}.
     * @see #modifyReverseEnumMap(Set, Function, Map)
     * @throws DuplicateKeysException if the <code>enumMapper</code> produces duplicate keys.
     */
    public static <T, E extends Enum<E>> Map<T, E> modifyReverseEnumMap(final Class<E> forEnum,
            final Function<E, T> enumMapper, final Map<T, E> result) {
        return modifyReverseEnumMap(getEnumValues(forEnum), enumMapper, result);
    }

    /**
     * Modifies a {@link Map} by putting mappings <code>T &#8594; E</code>, where keys are derived
     * using a {@link Function}.
     *
     * @param enumSet the {@link Set} of {@link Enum} to represent.
     * @param enumMapper the {@link Function} to use for deriving the {@link Map}'s keys.
     * @param result the {@link Map} to put the mappings to.
     * @return the <code>result</code> {@link Map}.
     * @throws DuplicateKeysException if the <code>enumMapper</code> produces duplicate keys.
     */
    public static <T, E extends Enum<E>> Map<T, E> modifyReverseEnumMap(final Set<E> enumSet,
            final Function<E, T> enumMapper, final Map<T, E> result) {
        validateArguments(enumSet, enumMapper, result);
        result.putAll(doMap(enumSet, enumMapper, Function.identity(), true));
        return result;
    }

}

EnumMapUtilsTest

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;

import java.util.EnumMap;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;

import com.mypackage.EnumMapUtils;

/**
 * Unit testing for {@link EnumMapUtils}.
 */
public class EnumMapUtilsTest {

    static final Logger log = LoggerFactory.getLogger(EnumMapUtilsTest.class);
    static final Set<Alphabet> ALL = newSet(Alphabet.values());
    static final Set<Alphabet> RANGE = EnumSet.range(Alphabet.ALFA, Alphabet.CHARLIE);
    static final Function<Alphabet, Integer> GET_ASCII = Alphabet::getAsciiValue;
    static final Function<Alphabet, String> TO_STRING = Alphabet::toString;
    static final Function<Alphabet, Object> LAST_FUNCTION = Alphabet::toString;
    static final Map<Alphabet, Integer> ENUM_TO_INT = mapValues(ALL, GET_ASCII);
    static final Map<Alphabet, Integer> ENUM_TO_INT_RANGE = mapValues(RANGE, GET_ASCII);
    static final Map<Alphabet, String> ENUM_TO_STRING = mapValues(ALL, TO_STRING);
    static final Map<Alphabet, String> ENUM_TO_STRING_RANGE = mapValues(RANGE, TO_STRING);
    static final Map<Integer, Alphabet> INT_TO_ENUM = mapKeys(ALL, GET_ASCII);
    static final Map<Integer, Alphabet> INT_TO_ENUM_RANGE = mapKeys(RANGE, GET_ASCII);
    static final Map<String, Alphabet> STRING_TO_ENUM = mapKeys(ALL, TO_STRING);
    static final Map<String, Alphabet> STRING_TO_ENUM_RANGE = mapKeys(RANGE, TO_STRING);
    static final Map<Object, Alphabet> SOURCE = objectToEnumMap(new HashMap<>(),
            Alphabet::getAsciiValue, LAST_FUNCTION);
    static final Map<Alphabet, Set<Object>> EXPECTED = mapValues(ALL,
            (value) -> newSet(value.getAsciiValue(), value.toString()));
    static final Map<Alphabet, Object> EXPECTED_SIMPLE = mapValues(ALL, LAST_FUNCTION);

    static enum Alphabet {
        ALFA, BRAVO, CHARLIE;

        int getAsciiValue() {
            return ordinal() + 65;
        }

        @Override
        public String toString() {
            return ((char) getAsciiValue()) + super.toString().substring(1).toLowerCase();
        }
    }

    static enum TestCase {
        CONVERT_TO_ENUM_MAP(EnumMapUtils.convertToEnumMap(Alphabet.class, SOURCE), EXPECTED),
        CONVERT_TO_SIMPLE_ENUM_MAP(EnumMapUtils.convertToSimpleEnumMap(SOURCE), EXPECTED_SIMPLE),
        MAP_ENUM_TO_INTEGER(EnumMapUtils.createEnumMap(Alphabet.class, GET_ASCII), ENUM_TO_INT),
        MAP_RANGE_ENUM_TO_INTEGER(EnumMapUtils.createEnumMap(RANGE, GET_ASCII), ENUM_TO_INT_RANGE),
        MAP_ENUM_TO_STRING(EnumMapUtils.createEnumMap(Alphabet.class), ENUM_TO_STRING),
        MAP_RANGE_ENUM_TO_STRING(EnumMapUtils.createEnumMap(RANGE), ENUM_TO_STRING_RANGE),
        MAP_INTEGER_TO_ENUM(EnumMapUtils.createReverseEnumMap(Alphabet.class, GET_ASCII), INT_TO_ENUM),
        MAP_RANGE_INTEGER_TO_ENUM(EnumMapUtils.createReverseEnumMap(RANGE, GET_ASCII), INT_TO_ENUM_RANGE),
        MAP_STRING_TO_ENUM(EnumMapUtils.createReverseEnumMap(Alphabet.class), STRING_TO_ENUM),
        MAP_RANGE_STRING_TO_ENUM(EnumMapUtils.createReverseEnumMap(RANGE), STRING_TO_ENUM_RANGE),
        REVERSE_ENUM_MAP(EnumMapUtils.createReverseEnumMap(ENUM_TO_STRING), STRING_TO_ENUM),
        MODIFY_REVERSE_MAP(EnumMapUtils.modifyReverseEnumMap(Alphabet.class, GET_ASCII,
                new TreeMap<>()), new TreeMap<>(INT_TO_ENUM)),
        MODIFY_RANGE_REVERSE_MAP(EnumMapUtils.modifyReverseEnumMap(RANGE, GET_ASCII,
                newDescendingTreeMap(null)), newDescendingTreeMap(mapKeys(RANGE, GET_ASCII)));

        final Map<?, ?> result;
        final Map<?, ?> expected;

        <K, V> TestCase(final Map<K, V> result, Map<K, V> expected) {
            this.result = result;
            this.expected = expected;
        }

        @Override
        public String toString() {
            return super.toString().replace('_', ' ').toLowerCase();
        }

        void verify() {
            assertThat(result, equalTo(expected));
            log.debug("Results for testing {}:", toString());
            result.forEach((key, value) -> log.debug("Key [{}] => Value [{}]", key, value));
        }
    }

    /**
     * Create a {@link Map} by deriving keys from a {@link Set} of {@link Alphabet} enums as values.
     *
     * @param set the {@link Set} of {@link Alphabet} {@link Enum}s to use as values.
     * @param keyMapper the {@link Function} to use for deriving keys per {@link Enum} value.
     * @return a {@link Map} with mappings <code>K &#8594; Alphabet</code>.
     */
    private static <K> Map<K, Alphabet> mapKeys(final Set<Alphabet> set,
            final Function<Alphabet, K> keyMapper) {
        return set.stream().collect(Collectors.toMap(keyMapper, Function.identity()));
    }

    /**
     * Create a {@link Map} by deriving values from a {@link Set} of {@link Alphabet} enums as keys.
     *
     * @param set the {@link Set} of {@link Alphabet} {@link Enum}s to use as keys.
     * @param valueMapper the {@link Function} to use for deriving values per {@link Enum} key.
     * @return a {@link Map} with mappings <code>Alphabet &#8594; V</code>.
     */
    private static <V> Map<Alphabet, V> mapValues(final Set<Alphabet> set,
            final Function<Alphabet, V> valueMapper) {
        return new EnumMap<>(set.stream().collect(
                Collectors.toMap(Function.identity(), valueMapper)));
    }

    /**
     * Wrapper method for creating a {@link Set} from an array.
     *
     * @param values the values to create a {@link Set} for.
     * @return a {@link Set} containing <code>values</code>.
     */
    private static <T> Set<T> newSet(final T... values) {
        return Stream.of(values).collect(Collectors.toSet());
    }

    /**
     * Iteratively call {@link EnumMapUtils#modifyReverseEnumMap(Class, Function, Map)} with each
     * element of <code>enumMappers</code>.
     *
     * @param result the {@link Map} to use in
     *            {@link EnumMapUtils#modifyReverseEnumMap(Class, Function, Map)}.
     * @param enumMappers the {@link Function}s to use for
     *            {@link EnumMapUtils#modifyReverseEnumMap(Class, Function, Map)}.
     * @return the <code>result</code> {@link Map}.
     */
    private static Map<Object, Alphabet> objectToEnumMap(final Map<Object, Alphabet> result,
            final Function<Alphabet, Object>... enumMappers) {
        for (final Function<Alphabet, Object> current : enumMappers) {
            EnumMapUtils.modifyReverseEnumMap(Alphabet.class, current, result);
        }
        return result;
    }

    /**
     * Creates a new {@link TreeMap} that will sort the keys by descending order instead of the
     * default ascending order.
     *
     * @param map passed to {@link Map#putAll(Map)} if not null.
     * @return a new {@link TreeMap}.
     */
    private static Map<Integer, Alphabet> newDescendingTreeMap(final Map<Integer, Alphabet> map) {
        final Map<Integer, Alphabet> expected = new TreeMap<>((first, second) -> second - first);
        if (map != null) {
            expected.putAll(map);
        }
        return expected;
    }

    @DataProvider(name = "test-cases")
    public Iterator<Object[]> getTestCases() {
        return Stream.of(TestCase.values()).map((current) -> new Object[] { current }).iterator();
    }

    @Test(dataProvider = "test-cases")
    public void testCase(final TestCase current) {
        current.verify();
    }

    @Test(expectedExceptions = EnumMapUtils.DuplicateKeysException.class)
    public void testBadKeyMapper() {
        EnumMapUtils.createReverseEnumMap(Alphabet.class, (value) -> 0);
    }

}
\$\endgroup\$

2 Answers 2

7
\$\begingroup\$

I don't like validateArguments. I can't give you a definitive reason; I understand why it's there. It's certainly better than having all sorts of != null in all your other functions.

You throw IllegalArgumentException when I pass in null somewhere. But here's the catch.

I don't know that I passed you null. I gave you some sort of variable, and it comes from my pretty badly written code. And you say Illegal Argument!. I look through my code and I see nothing wrong.

Like a teacher who has too many tests to grade, I get a report back: My answers are wrong. I get an F. That's not good. Now I have to study the material again. I wish I knew what to study. Studying the whole material means I'll be busy all night.

If you said Illegal Argument! Null is not allowed! I'd have said "Oh yeah, that's right!" and been on my way.


I think you should either wrap a new NullPointerException() in your IllegalArgumentExceptions, or alter the message to contain "Null is not allowed!". Alternatively, take a look at Objects.requireNotNull(T object, String errorMessage): It validates for not null and allows you to pass a specific message.

I like the rest of the code. I read it thoroughly; All I'm wondering about is whether "over-riding" should be "overriding" instead.

\$\endgroup\$
0
5
\$\begingroup\$

Error Handling

Why not use NullPointerException (by way of validateArguments, which - as @Pimgd noted - is better done using Objects.requireNotNull(Object, String). An NPE is, very commonly, a specific kind of invalid argument. The exception heirarchy isn't set up that way, but semantically they provide clear information: "Actual argument must not be null for 'abc' in method 'xyz'" is preferable to "Actual argument is (somehow) invalid for 'abc' in method 'xyz'" - no? You get this just by using Objects.requireNotNull(abc, "abc");

private doMap(..., boolean checkDuplicateKeys) - when checking, only indicates that "some" duplicates exist, but doesn't provide even a single specific duplicated key. Providing that level of feedback would be nice, but requires either a different map collection logic and/or extra logic deferred to when the "must have had at least one dupe - let's find one specific item to report" is being done. Such extra effort should only be done once the error is detected, when it's already known to be worth the richer error reporting effort.

If you are going to validate args (vs. null or otherwise), you should do so consistently in all API methods - even when you "know" that validation would also done by other methods called in that method's body. True, such checks will be redundant, but I would rather be sure that it's done consistently than have to think about if the called function validates (and reports) a violation in the way it should. This is especially true when argument names change from what the API client used to what is reported at failure (assuming you were to follow something like @Pimgd's suggestion).

Unless this is a "high-performance" utility (which I don't think it is), you shouldn't bother about any inefficiency due to argument re-validation. Even so, even if this were an HP utility, I prefer to validate arguments at the class surface (API / public methods) and then transfer control to private methods that can blithely (efficiently) continue without further checking, knowing it was done at the API level. (Simple check-then-delegate API methods would likely be inlined, and would be fine even for HP. If you're still concerned about efficiency, then you should be writing micro-code...)

API Names

Why convertToEnumMap and convertToSimpleEnumMap? Perhaps toEnumMapToSet and toEnumMapToValue

Sometimes create** sometimes convertTo**? How about consistently to**?

Use established language from mathematics about function inverses, where a Map is considered a discrete finite function: createReverseEnumMap should be toInverseMapToEnum

When a name part is **EnumMap**, it should refer to the already existing class EnumMap, not merely "some map with enum values on one side or the other." When you mean "Map of Enum to T" you should use **MapOfEnum** (which may, or may not be, an EnumMap). Conversely, **MapToEnum** for a map (not necessarily an EnumMap, which would under this naming imply an enum-to-enum mapping, where the two enum's might (not) be the same Enum).

Why This Way?

What purpose would createEnumMap(E) serve? Rather than String s = eMap.get(enumValue) why not String s = enumValue.toString()? I suppose there might be times when some existing code wants such a mapping (and it's not open to refactoring), or ... hmmm... when? (Actually, I can see the point to such a converter, even in a green-field project. I would like to see the JavaDoc suggest that such a mapping be avoided in favor of the already-available enum.toString() when that is sufficient.) Ditto, re: createEnumMap(Set<E>)

Also, the reverse situation with a Map<String, E> is also of uncertain value. Isn't that what E.valueOf(String) is for? If you don't like client code dealing with exceptions, then a utility class which swallows IllegalArgumentException and/or NullPointerException and merely returns null seems right. BTW - such a utility method wouldn't belong to the EnumMapUtils class, but rather an EnumUtils or Enums class in a utility package.

I know you're playing with Java 8-isms, but getEnumValues seems overwrought. Why not just return EnumSet.allOf(forEnum); (I made this modification to your code and all tests continue to pass.)

Since Collectors.toMap(...) takes a merge function, why not expose that capability to the clients of this utility class? You might still default it, if you like defaults, but for clients that need to keep the first item in a merge, or the greatest, or the closest to a reference, or ... - they can't use your utility as it stands (making it less utile(?)).

Documentation (and related type)

When a method is documented as returning an EnumMap (or HashMap), the return type of that method should be specialized to match. If you don't want to specialize the return type, then you shouldn't mention it in the JavaDoc. The doc describes the contract - and the code should be as close to it as possible. Thus, as per the current JavaDoc, I prefer: public static <T, E extends Enum<E>> HashMap<T, E> createReverseEnumMap(...)

I like your use of the right-arrow in the JavaDoc, but the "raw" docs are much more readable if you use &rarr; rather than &#8594;.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the comments! Here's a SO question discussing IllegalArgumentException vs NullPointerException, and I think I was partly influenced about that 1.5 years ago... I've already refactored parts of this code to include the changes suggested here, including the use of EnumSet.allOf() instead of the bespoke getEnumValues(). :) And your comments about namings are good too! \$\endgroup\$
    – h.j.k.
    Commented Apr 5, 2016 at 2:58
  • 1
    \$\begingroup\$ @h.j.k. Glad it helped you (and hopefully others); it also helped me understand a few things I hadn't though of until I was in "deep critique" mode. If you end up producing something significantly different, please post as a new review and link them together. \$\endgroup\$
    – Stevel
    Commented Apr 5, 2016 at 7:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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