4
\$\begingroup\$

Given a Map<K, Object>, I'm returning Map<K, String> or throwing an Exception for at least one non-String value.

     private static <K> Map<K, String> validate(final Map<K, Object> map) 
                        throws NonStringValueException {
    final List<Object> invalidValues = new ArrayList<Object>();
    final Map<K, String> result = new HashMap<K, String>();

    for(Map.Entry<K, Object> entry : map.entrySet()) {
        if(!(entry.getValue() instanceof String)) {
            invalidValues.add(entry.getValue());
        }
        else {
            result.put(entry.getKey(), (String) entry.getValue());
        }
    }

    if(!invalidValues.isEmpty()) {
        throw new NonStringValueException("Invalid Values: 
            must be `String` type: " + invalidValues);
    }

    return result;
}

Besides using instanceOf and then casting, is there a better way to write this method?

\$\endgroup\$
4
\$\begingroup\$

Some simple things first - there's no need to repeat the generic type of variables on the right-hand-side of a new assignment. Java 7 introduced the "diamond operator" <> and the generic type is inferred. So, lines like:

final List<Object> invalidValues = new ArrayList<Object>();
final Map<K, String> result = new HashMap<K, String>();

can be just:

final List<Object> invalidValues = new ArrayList<>();
final Map<K, String> result = new HashMap<>();

Your use of the static method and generic type <K> is good. I like how you have made the variables final in the method too. Additionally, your use of the for-each loop on the Map's entries is also good.

A comment on your generics, your input map should be Map<K, ?> instead of Map<K, Object> .... the ? allows more flexibility.

Java introduced a couple of helper methods on Class a while back, and I think you could use these to make this method even more generic.... why force it to only be a String?

Consider the following two methods:

private static <K, V> Map<K, V> validate(Class<V> coerce, final Map<K, ?> map) 
        throws IllegalArgumentException {
    final List<Object> invalidValues = new ArrayList<>();
    final Map<K, V> result = new HashMap<>();

    for(Map.Entry<K, ?> entry : map.entrySet()) {
        if(coerce.isInstance(entry.getValue())) {
            result.put(entry.getKey(), coerce.cast(entry.getValue()));
        } else {
            invalidValues.add(entry.getValue());
        }
    }

    if(!invalidValues.isEmpty()) {
        throw new IllegalArgumentException("Invalid Values: must be `" + coerce.getName() +  "` type: " + invalidValues);
    }

    return result;
}

public static <K> Map<K, String> validate(Map<K, ?> map) {
    return validate(String.class, map);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for your thoughtful answer - I liked the generic Class<V> that you taught me. Lastly, what are your thoughts about using a checked exception instead - to let the compiler help us out more? \$\endgroup\$ – Kevin Meredith Jul 14 '15 at 14:15
  • \$\begingroup\$ The checked exception is very much dependent on your context. I am not sure whether it is "right", but I seldom create custom exceptions any more, and rely on native exceptions. Further, I am using unchecked more often too (like IllegalArgumentException) because it helps when using Java 8 streams. \$\endgroup\$ – rolfl Jul 14 '15 at 14:18
  • \$\begingroup\$ allows more flexibility. And that's because of what Joshua Bloch says in Item 23 of Effective Java: you can't put any element (other than null) into a Collection<?>.? That's the reason for using ? over Object in your answer? \$\endgroup\$ – Kevin Meredith Jul 14 '15 at 18:00
  • \$\begingroup\$ @KevinMeredith - it's a little complicated, but the <?> will accept maps of any generic type, like String, Integer, etc... whereas <Object> will take only Object.... see this code, and try to compile it: pastebin.com/Ypj3exaP \$\endgroup\$ – rolfl Jul 14 '15 at 18:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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