[
https://issues.apache.org/jira/browse/OFBIZ-10485?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16569306#comment-16569306
]
Mathieu Lirzin commented on OFBIZ-10485:
----------------------------------------
{quote}I'm not sure replacing {{stackList}} with maps in patch 9 adds clarity.
Maybe both names are vague and do not convey purpose. How about calling it
{{contexts}}, or {{contextMaps}}, or something like that?
{quote}
I like {{contexts}}, thanks for proposing.
{quote}for multi-line comments, I recommend the /* */ comment style, maybe that
would be a bit cleaner?
{quote}
I agree.
{quote}Also, what do you mean by checking a key instead of null allows for
overriding, it means override with a null instead of of skipping? If yes,
aren't we changing the logic of the system and does that introduce any bugs or
new behavior?
{quote}
It means that {{null}} is considered has a valid value which has the effect of
masking any value associated with the same key in following context maps. If we
were using\{{curMap.get(key) != null}} then this will not work since getting a
{{null}} value would mean the absence of value. This is matching previous
implementation so this doesn't introduce any new behavior.
{quote}Nice work on the entryStream, I like it. Did you apply tests from your
side to ensure no edge cases are broken after the replacement from the
for-loops?
{quote}
Thanks :). In fact I have an even more compact (better?) version of it which
avoids the use of explicit boolean values inside an {{if}} statement:
{code:java}
// Return a sequential stream of actual entries.
private Stream<Map.Entry<K, V>> entryStream() {
Set<K> seenKeys = new HashSet<>();
return contexts.stream()
.flatMap(m -> m.entrySet().stream())
// Ensure that `flatMap` preserves the encountered order.
.sequential()
// Remove entries with duplicate keys which are considered as
masked.
.filter(e -> seenKeys.add(e.getKey()));
}
{code}
I have not run tests besides ensuring that it doesn't broke everything by
launching OFBiz and navigating manually to check that the resolution of request
maps was still working. This is indeed not ideal but I am quite confident in
its correctness.
{quote}Why are we using a functional interface in patch #12? it feels a bit
complicated, why not just apply 2 streams and be done with it?'
{quote}
Indeed, this is related to the the fact that {{null}} must be considered as a
valid value (as describe above). As a consequence it is not possible to write
something like:
{code:java}
public V get(Object key) {
return contexts.stream()
.filter(m -> m.containsKey(key))
.map(m -> m.get(key))
.findFirst()
.orElse(null);
}
{code}
In this implementation {{findFirst}} returns an {{Optional}} which throws when
getting a {{null}} value. It is possible to workaround this issue with hacks by
replacing {{findFirst().orElse(null)}} with {{limit(1).reduce(null, (default,
val) -> val)}} but IMO this hurts readability.
The {{withMapContainingKey}} principal benefit is that it avoids having to
maintain the code and comment regarding the necessity of using {{containsKey}}
in multiple places. By the way this method could be renamed
{{withContextContainingKey}} to follow your suggestion.
Thanks for your time.
> Refactor MapContext
> -------------------
>
> Key: OFBIZ-10485
> URL: https://issues.apache.org/jira/browse/OFBIZ-10485
> Project: OFBiz
> Issue Type: Improvement
> Components: base
> Reporter: Mathieu Lirzin
> Assignee: Taher Alkhateeb
> Priority: Minor
> Fix For: Upcoming Branch
>
> Attachments: OFBIZ-10485_0001-Remove-MapContext-dead-code.patch,
> OFBIZ-10485_0002-Add-missing-Override-in-MapContext.patch,
> OFBIZ-10485_0003-Use-the-Deque-interface-in-MapContext.patch,
> OFBIZ-10485_0004-Rewrite-MapContext-isEmpty.patch,
> OFBIZ-10485_0005-Rewrite-MapContext-containsKey.patch,
> OFBIZ-10485_0006-Rewrite-MapContext-keySet.patch,
> OFBIZ-10485_0007-Remove-MapContext.ListSet-class.patch,
> OFBIZ-10485_0008-Inline-MapContext-getMapContext.patch,
> OFBIZ-10485_0009-Rename-stackList-to-maps.patch,
> OFBIZ-10485_0010-Rework-comments.patch,
> OFBIZ-10485_0011-Add-entryStream.patch,
> OFBIZ-10485_0012-Add-withMapContainingKey.patch,
> OFBIZ-10485_0013-Rewrite-size.patch
>
>
> Following conversation
> [https://lists.apache.org/thread.htmlf1729ef5eafcf71adfbed0c3ea61dfb73225dff82abc4a57c3de8ed5@%3Cdev.ofbiz.apache.org%3E]
> on d...@ofbiz.apache.org.
> Here is a first batch of patches to clean things up. Those patches are meant
> to be applied in order.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
