Java Map value access

Question

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?

• user33306: 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.
• Farewell Stack Exchange: 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.
• cplusplusrat: 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.
• 200_success: Please provide a real-world example usage where you think this function would be advantageous.
• slowy: 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.

Answer 1

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.

Answer 2

 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.

• cplusplusrat: Yep, I will likely save one additional lookup by getting the value directly and doing a null check instead of containsKey.

Answer 3

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