I just realised I sent my last reply to Anthony only... apologies. Please
see below.
Cheers,
Abraham
El mié., 19 jun. 2019 a las 17:43, Abraham Marin-Perez (<
abraham.marin.pe...@gmail.com>) escribió:
> Hi Anthony,
>
> There is code before and after compareIfPresent, but yeah, we could have
> reordered that code and include an isEmpty check to avoid the problem.
> However, let’s take a step back and have a look at what this entails.
>
> Having documentation is great, needing documentation not so much. Whenever
> possible, methods should have a name that describes in plain English what
> they do so as to minimise the need to go and check the documentation. This
> makes code intuitive. Let’s analyse the plain English for computeIfPresent.
> Compute if present. It doesn’t say what happens if not present, but one
> would argue that a plain English interpretation would be “it not present,
> nothing happens”.
>
> We’ve already seen that there are some powerful reasons for which this is
> not quite the case (as Roger laid out in detail). Reasons that lead us to
> the unfortunate situation that the plain English interpretation is not
> quite correct, but calling it
> “computeIfPresentAndIfNotNothingHappensUnlessTheMapIsImmutable” would be
> absurd, so we live with that. The side effect is we have a method whose
> behaviour causes confusion to developers and forces them to check the
> documentation.
>
> Once the developer has checked the documentation, then we need to adapt
> the code and add some guards that, on face value, shouldn’t be needed. Why
> should you need to check whether a map is empty before attempting
> computeIfPresent? You shouldn’t, the fact that you do is just a side effect
> of the an implementation decision.
>
> So my point is: we could definitely do what you suggested, but I am not
> entirely sure that such measures should be needed.
>
> Cheers,
> Abraham
>
> Sent from my iPhone
>
> On 19 Jun 2019, at 11:58, Anthony Vanelverdinghe <anthonyv...@outlook.com>
> wrote:
>
> 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 <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>
>
>
--
Amazon Page: https://amazon.com/author/abraham.marin.perez
Twitter: @AbrahamMarin <https://twitter.com/AbrahamMarin>
LinkedIn: http://lnkd.in/TTHC8W
