map and null values

[Peter Levart]

On 01/02/2013 06:01 PM, Brian Goetz wrote:
>> >Also the assumption the null values are not legal is so strong, that the
>> >implementation of replace::
>> >
>> >   default boolean replace(K key, V oldValue, V newValue) {
>> >          if (*!containsKey(key) || !get(key).equals(oldValue)*)
>> >              return false;
>> >          put(key, newValue);
>> >          return true;
>> >      }
>> >
>> >throws NPE exception in the example below:
>> >
>> >mss.put("null", null);
>> >
>> >mss.replace("null", null, "null");
> Note that many existing Map implementations throw NPE when you try and
> put a null key or value into it anyway.
>
>
That's true, but the default *replace* method could do better for those 
(3rd party) Maps that allow null values. For example:

     default boolean replace(K key, V oldValue, V newValue) {
         if (!containsKey(key) || !Objects.equals(get(key), oldValue))
             return false;
         put(key, newValue);
         return true;
     }

The same with default *remove* method:

     default boolean remove(Object key, Object value) {
         if (!containsKey(key) || !Objects.equals(get(key), value))
             return false;
         remove(key);
         return true;
     }

Also the default *compute* methods doesn't play well:

         Map<Integer, String> map = new HashMap<>();
         map.compute(1, (k, v) -> "x");

...loops infinitely and:

         Map<Integer, String> map = new ConcurrentHashMap<>();
         map.compute(1, (k, v) -> "x");

...throws NPE.

I think the following implementation is better:

     default V compute(K key,
                       BiFunction<? super K, ? super V, ? extends V> 
remappingFunction) {
         V oldValue = get(key);
         for (;;) {
             if (oldValue != null || containsKey(key)) {
                 V newValue = remappingFunction.apply(key, oldValue);
                 if (newValue != null) {
                     if (replace(key, oldValue, newValue))
                         return newValue;
                 }
                 else if (remove(key, oldValue)) {
                     return null;
                 }
                 oldValue = get(key);
             }
             else {
                 V newValue = remappingFunction.apply(key, null);
                 if (newValue != null) {
                     if ((oldValue = putIfAbsent(key, newValue)) == null)
                         return newValue;
                 }
                 else
                     return null;
             }
         }
     }


Also, the default *merge* method is not good:

         Map<Integer, String> map = new HashMap<>();
         map.put(1, null);
         String newVal = map.merge(1, "x", (old, val) -> "y");
         System.out.println(newVal);
         System.out.println(map);

...prints:

x
{1=null}

The better variant is:

     default V merge(K key, V value,
                     BiFunction<? super V, ? super V, ? extends V> 
remappingFunction) {
         V oldValue = get(key);
         for (;;) {
             if (oldValue != null || containsKey(key)) {
                 V newValue = remappingFunction.apply(oldValue, value);
                 if (newValue != null) {
                     if (replace(key, oldValue, newValue))
                         return newValue;
                 }
                 else if (remove(key, oldValue)) {
                     return null;
                 }
                 oldValue = get(key);
             }
             else {
                 if (value != null) {
                     if ((oldValue = putIfAbsent(key, value)) == null)
                         return value;
                 }
                 else
                     return null;
             }
         }
     }


Will HashMap get optimized implementations? Accepting contributions?

Regards, Peter