Hi Abraham
Wouldn’t it have been possible to use Map::isEmpty? Ideally just returning at
the very beginning of your method if `data` is an empty Map. That way you’d
still be able to use computeIfPresent, and use emptyMap() as well.
Kind regards,
Anthony
________________________________
From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on behalf of
Abraham Marín Pérez <abraham.marin.pe...@gmail.com>
Sent: Wednesday, June 19, 2019 9:51:46 AM
To: Roger Riggs
Cc: core-libs-dev@openjdk.java.net
Subject: Re: Suggestion for a more sensible implementation of EMPTY_MAP
Hi Stuart, Roger,
First of all, thank you for such a detailed response, this really shows the big
picture. I guess no implementation will be perfect, there will always be some
wrinkles that we need to accept, and the most suitable implementation will be
the one with the fewest or least impacting wrinkles. Given this, maybe showing
the particular wrinkle that I faced can shed light on the overall development
experience that the current implementation provides.
For multiple reasons that I cannot explain at this point, I have a method that
looks like the following:
private void decorate(Map<String, String> data) {
//...
data.computeIfPresent("field", (k, v) -> highlightDifferences(v,
otherValue));
//...
}
At one point an emptyMap() was passed to this method, causing an UOE. This left
me with two choices:
1. Change code to:
if(data.hasKey(“field"))
data.compute("field", (k, v) -> highlightDifferences(v, otherValue));
2. Avoid usage of emptyMap() and use new HashMap<>() instead
The first option defeats the purpose of having a computeIfPresent method, and
causes confusion among team members (people keep asking why computeIfPresent
isn't used). The second option is pretty much what Roger mentioned (and what we
ended up doing).
On the other hand, there are some points that you mentioned that I believe
merit some extra debate, please find some comments below inline.
In general, I tend to agree that a stricter implementation is better since it
usually prevents bugs. However, at least in this particular case, I believe a
stricter implementation also has the side effect of worsening the development
experience and adding confusion (see below comments). Of course, as I mentioned
before, I understand that no implementation will be perfect, and maybe the
current option is the lesser evil, but I wanted to throw in an alternative for
consideration.
Many thanks,
Abraham
El vie., 14 jun. 2019 a las 14:18, Roger Riggs (<roger.ri...@oracle.com
<mailto:roger.ri...@oracle.com>>) escribió:
Hi Stuart,
Not withstanding all the details. It would be useful (and possibly
expected) that an EMPTY_MAP
could be substituted for a Map with no entries. As it stands, the
caller needs know whether any optional
possibly modifying operations will be used and decide whether to create
an empty map or an EMPTY_MAP.
That makes using an EMPTY_MAP a risk and less useful and a cautious
programmer will avoid it.
$.02, Roger
On 6/13/19 8:42 PM, Stuart Marks wrote:
>
>
> On 6/10/19 7:34 AM, Abraham Marín Pérez wrote:
>> I agree that it makes sense for EMPTY_MAP to have the same logic as
>> Map.of() or unmodifiable(new HashMap()), which means that my
>> suggestion cannot be applied to just EMPTY_MAP... but maybe that’s
>> ok: maybe we can change all of them? If we keep the default
>> implementation for Map.computeIfPresent in EMPTY_MAP, Map.of(),
>> unmodifiableMap, etc., instead of overriding it to throw an
>> exception, then:
>>
>> - For cases where the key isn’t present (regardless of empty or
>> non-empt map), the call will be a safe no-op.
>> - For cases where the key is present, the call will still throw UOE
>> via the implementation of put()
>>
>> I believe this would still respect the spec for Map.computeIfPresent
>> while providing a more forgiving implementation (in the sense that it
>> will avoid throwing an exception when possible).
>
> Hi Abraham,
>
> The specification does not mandate one behavior or another. Either
> behavior is permitted, and in fact both behaviors are present in the JDK.
>
> The second and more significant point is raised by your last statement
> suggesting a "more forgiving implementation." The question is, do we
> actually want a more forgiving implementation?
>
> The collections specs define certain methods as "optional", but it's
> not clear whether this means that calling such a method should always
> throw an exception ("strict" behavior) or whether the method should
> throw an exception only when it attempts to do something that's
> disallowed ("lenient" behavior).
>
> Take for example the putAll() method. What happens if we do this?
>
> Map<String, String> map1 = Collections.emptyMap();
> map1.putAll(otherMap);
>
> The answer is that it depends on the state of otherMap. If otherMap is
> empty, then the operation succeeds (and does nothing). However, if
> otherMap is non-empty, then the operation will throw
> UnsupportedOperationException. That's an example of lenient behavior.
> What's notable about this is that you can't predict the outcome unless
> you know the state of otherMap.
>
> Now, how about this example?
>
> Map<String, String> map2 = Map.of();
> map2.putAll(otherMap);
>
> This always throws UnsupportedOperationException, regardless of the
> state of otherMap. I'd call this strict behavior.
>
> More recent implementations, including the Java 8 default methods, and
> the Java 9 unmodifiable collections (Map.of et al), have all preferred
> strict behavior. This is because less information about the state of
> the system is necessary in order to reason about the code, which leads
> to fewer bugs, or at least earlier detection of bugs. It's unfortunate
> that emptyMap().putAll() has this lenient behavior, but we're stuck
> with it for compatibility reasons.
>
> Now there is a slight wrinkle with computeIfPresent(). If we have
>
> Map<String, String> map = Collections.emptyMap();
> map.computeIfPresent(key, remappingFunction);
>
> then as you point out, this can never modify the map, since the key is
> never present. Thus, there isn't any behavior that's dependent upon
> additional state.
>
> But, as Michael observed, there is now an inconsistency with Map.of()
> and Collections.unmodifiableMap(). So, continuing with your
> suggestion, we might change those implementations to allow
> computeIfPresent() as well. Thus,
>
> Map<String, String> map1 = Collections.emptyMap();
> Map<String, String> map2 = Map.of();
> Map<String, String> map3 = Collections.unmodifiableMap(new
> HashMap<>());
>
> and then
>
> {map1,map2,map3}.computeIfPresent(key, remappingFunction);
>
> all are no-ops. Well, maybe. What if we had retained a reference to
> the HashMap wrapped by the unmodifiableMap, and then we added an
> element? Now it contains a mapping; what should happen then? Should
> computeIfPresent() throw an exception because it *might* attempt to
> modify the map? Or should it throw an exception *only* if it attempts
> to modify the map? State-dependent behavior has slipped back in.
Whilst I find the Collections.unmodifiable* family of methods very useful, I
always thought they were a bit of a misnomer: they don’t really make the
underlying collection unmodifiable, they just wrap it in an
unmodifiable-looking cloak. I believe it is a common expectation that, if you
do keep a reference to the underlying, mutable collection, and you make changes
to it, then those changes are to transpire elsewhere. Consider the following:
Map<String, String> map1 = new HashMap<>();
Map<String, String> map2 = Collections.unmodifiableMap(map1);
map2.get(“key”); // returns null
map1.put(“key”, “value”);
map2.get(“key”); // returns “value”
The second call to map2.get(“key”) would yield a different result to the first
one, exhibiting state-dependent behaviour… but I don’t think anybody would be
surprised by that. In fact, I believe people would be more surprised of the
opposite. In this case, state-dependent behaviour actually feels more natural
and desirable than state-independent behaviour.
This might be part of a wider conversation, but I think we might be at risk of
mistaking a desire for immutability (which I agree with) with a desire for
state-independence. Objects have an internal state and behaviour will always
depend on it, the question is whether that state can change or not; if state
can indeed change, then it is my believe that behaviour should reflect that
change.
>
> We also need to consider the impact on the other compute* methods.
> Consider:
>
> Map<String, String> map = Collections.emptyMap();
> map.compute(key, (k, v) -> null);
>
> This can never change the map, but it currently throws UOE. Should it
> be changed to a no-op as well?
>
> **
>
> What we have now is strict rule: calling mutator methods on
> unmodifiable collections throws an exception. It's simple to describe
> and reason about, and it's (mostly) consistent across various
> collections. However, it prohibits some operations that sometimes
> people want to do.
>
> If we were to make the system more lenient, or more forgiving, then
> weird inconsistencies and conditional behaviors pop up in other
> places. This makes the system more complicated and harder to reason
> about, and that leads to more bugs.
I guess at this point we are approaching the realm of personal opinion, since I
actually see the opposite: more lenient behaviour would make the code easier to
understand. Stricter behaviour in this case means you cannot just use the type
and properties of the variable at hand to deduce behaviour, you also need to
know how the particular object was created. A more lenient behaviour would
remove oddities like the one I had to face.
>
> s'marks
>
--
Amazon Page: https://amazon.com/author/abraham.marin.perez
<https://amazon.com/author/abraham.marin.perez>
Twitter: @AbrahamMarin <https://twitter.com/AbrahamMarin>
LinkedIn: http://lnkd.in/TTHC8W <http://lnkd.in/TTHC8W>
