[jira] [Commented] (GROOVY-7802) `MapWithDefault` shouldn't store its default value

["John Wagenleitner (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/GROOVY-7802?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15232341#comment-15232341 ] 

John Wagenleitner commented on GROOVY-7802:
-------------------------------------------

I find the current behavior of {{withDefault}} makes sense and I definitely would not want to see a breaking change made to it.  

For the alternatives, the first two would only apply to {{MapWithDefault}} maps, so seems odd to add those methods to {{Map}}.  So in my opinion the constructor parameter would be my choice of the options available.  Though I don't usually like boolean parameters I think in this case it would be better than an Enum.  I think the GDK docs for a {{#withDefault(boolean, Closure)}} method could make it sufficiently clear what the boolean switch will do in terms of behavior along with code samples in the docs.

I think the current behavior of {{get}} and {{containsKey}} are valid.  I don't agree that contains key should always return {{true}} or throw.  If the map doesn't contain the key it should return false.  Even though {{get}} will insert into the map that to me has no bearing on how {{containsKey}} should work, since I think it should work based on the current state of the map.

I find the proposed change more confusing than the current behavior, but that may just be that my use cases fit more with how it's currently implemented.  But if the change can be introduced without affecting the current behavior or by adding addtional methods to {{Map}} that only apply to {{MapWithDefault}} then it seems reasonable.  

Am curious what others may think.

> `MapWithDefault` shouldn't store its default value
> --------------------------------------------------
>
>                 Key: GROOVY-7802
>                 URL: https://issues.apache.org/jira/browse/GROOVY-7802
>             Project: Groovy
>          Issue Type: Bug
>          Components: groovy-jdk
>    Affects Versions: 2.4.6
>            Reporter: Pap Lőrinc
>
> As described in https://github.com/apache/groovy/pull/267, a Groovy map with a default value stores all the accessed elements, even if they weren't added explicitly, just queried.
> In certain algorithms (e.g. https://github.com/careercup/CtCI-6th-Edition-Groovy/blob/d116d65469bdf17d1e215e89f3e76ac3a97660a9/src/main/groovy/Ch04_TreesAndGraphs/_04_12_PathsWithSum.groovy#L21) the complexity would automatically be reduced, if default values would be deleted (i.e. a default of 0 that is incremented and decremented in a recursive algorithm).
> Topics of discussion:
> * Currently a `getAt` will `add` also, therefore `containsKey` isn't the same as `getAt(...) != null`
> * After the change `keySet()/entrySet()` wouldn't include default values
> I proposed alternatives:
> * `removeIfDefault(Object)` // removes current key, if value is equal to default
> * `consolidate()` // removes all default values
> * constructor parameter `STORE_DEFAULT storeDefaultValues = STORE_DEFAULT.FALSE`, or maybe with a `boolean`, though I find primitive parameters confusing
> others:
> * `containsKey` should always return `true`, or throw an exception, since its behavior is inconsistent
> Waiting for your thoughts :)



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)