0
\$\begingroup\$

I am more or less a Java noob and wanted some feedback on a method I wrote. I wanted to avoid the pattern of checking if a key exists before accessing the value and throw an exception if the key does not exist.

Instead of doing this:

if (map.containsKey(key)) {
    return map.get(key)
}
else {
    throw new IllegalArgumentException();
}

everywhere in the code, I came up with the following solution:

class Utils {
    public static <K, V> V safeGet(final K key, Map<K,V> map) {
        if (map.containsKey(key)) {
            return map.get(key);
        } else {
            throw new IllegalArgumentException();
        }
    }
}

Is there anything wrong with this method? Do you see any problems with this approach?

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Since you're putting it in a separate function any way, wouldn't it be better to return a boolean indicating whether the get was successful? This way you can handle the error without throw an exception. \$\endgroup\$ – tinstaafl Aug 29 '17 at 18:18
  • \$\begingroup\$ I think that you did very well. I don't find anything wrong with this method and I don't see any problems with this approach. \$\endgroup\$ – Farewell Stack Exchange Aug 29 '17 at 18:54
  • \$\begingroup\$ If I have to return a boolean, I might as well return a map.get(key) and do a null check before using the value returned. \$\endgroup\$ – cplusplusrat Aug 29 '17 at 21:26
  • 1
    \$\begingroup\$ Please provide a real-world example usage where you think this function would be advantageous. \$\endgroup\$ – 200_success Aug 29 '17 at 21:36
  • \$\begingroup\$ As 200_success mentioned: I have a hard time to really see a good reason to use that. It looks like, somewhere in your app, you get a key from somewhere, which you shouldn't have received in the first place. And: You might want to check the Optional type. \$\endgroup\$ – slowy Aug 29 '17 at 21:58
1
\$\begingroup\$

As some other stated I am also convinced that there is less or no beneficial usage for enriching a simple associative map to throw an exception if an element is not found.

I suggest to go with a use case specific exception handling as the context is important where a not available value is an exceptional case.

\$\endgroup\$
0
\$\begingroup\$
        if (map.containsKey(key)) {
            return map.get(key);
        } else {
            throw new IllegalArgumentException();
        }

The map.get method does a contains call as part of it. So you could do

        V value = map.get(key);

        if (value == null) {
            throw new IllegalArgumentException();
        }

        return value;

This changes the result slightly. The original could return a null value if the map contains key/value pair where the value is null. If that is desirable, then your original code might be better.

But if that's a bug, and you only want to return non-null values, then this code is better. It explicitly checks for the bad result.

It's unclear what the compiler does with the original code. Perhaps it optimizes out the extra containsKey check. Or perhaps it does the same calculations twice, unnecessarily. This might be more efficient. Or maybe the compiler fixes it. Someone would have to look at the generated byte-code to see.

\$\endgroup\$
1
  • \$\begingroup\$ Yep, I will likely save one additional lookup by getting the value directly and doing a null check instead of containsKey. \$\endgroup\$ – cplusplusrat Aug 30 '17 at 2:14
0
\$\begingroup\$

You can use the getOrDefault method to require only one lookup for most map implementations (esp. important for concurrent maps that may contain null values to ensure reliable results).

private static final Object NOT_PRESENT = new Object();
public static <K, V> V safeGet(K key, Map<K, V> map) {
    Map m = map; Object v;
    if ((v = m.getOrDefault(key, NOT_PRESENT)) == NOT_PRESENT)
        throw new IllegalArgumentException();
    return (V) v;
}
\$\endgroup\$

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.