On Mon, 30 Nov 2020 15:08:51 GMT, Pavel Rappo <[email protected]> wrote:
>> @johnlinp, thanks for updating the CSR draft; it is much better now.
>>
>> @stuart-marks, I think we could further improve this snippet. This `if`
>> statement seems to use an optimization:
>>
>> if (oldValue != null || map.containsKey(key))
>>
>> I don't think we should include an optimization into the specification
>> unless that optimization also improves readability. Is this the case here?
>> Could this be better?
>>
>> if (map.containsKey(key))
>
> I would even go as far as to rewrite that snippet like this:
>
> if (newValue == null) {
> remove(key);
> } else {
> put(key, newValue);
> }
> return newValue;
>
> This rewrite is possible thanks to the following properties of
> `Map.remove(Object key)`:
>
> 1. A call with an unmapped `key` has no effect.
> 2. A call with a mapped `key` has the same semantics regardless of the value
> that this key is mapped to.
>
> In particular, (2) covers `null` values.
>
> To me, this rewrite reads better; however, I understand that readability is
> subjective and that snippets used in `@implSpec` might be subject to
> additional requirements.
@pavelrappo The intended effect of the revised snippet is sensible, but again I
note that it differs from the actual default implementation. Specifically: if
the map does not contain the key and newValue is null, the default
implementation currently does nothing, whereas the revised snippet calls
`remove(key)`. This should have no effect _on the map_ but a subclass might
override `remove` and this behavior difference is observable. (The typical
example of this is maintaining a counter of the number of operations. I think
_Effective Java_ uses that example in discussing subclassing.) I think the main
priority here is fidelity to what the default implementation actually does --
at least, concerning operations on *this* -- and less on readability.
-------------
PR: https://git.openjdk.java.net/jdk/pull/714
