Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MultiDict.add should return this if no changes are made #215

Open
bblfish opened this issue Jul 11, 2023 · 3 comments
Open

MultiDict.add should return this if no changes are made #215

bblfish opened this issue Jul 11, 2023 · 3 comments

Comments

@bblfish
Copy link

bblfish commented Jul 11, 2023

The add method is defined as follows

  def add(key: K, value: V): MultiDict[K, V] =
    new MultiDict(elems.updatedWith(key) {
      case None     => Some(Set(value))
      case Some(vs) => Some(vs + value)
    })

but elms.updatedWith could easily return the same MultiDict if the value is already in the Set[V].
It would help if the signature made sure that the same object would be returned if nothing changed as that would allow one
to reduce unnecessary object creation (both in the call, and also in the creation of objects in the calling object.
Otherwise one ends up creating a lot of garbage.

Something like this seems better:

def add(key: K, value: V): MultiDict[K, V] =
   val newElems = elems.updatedWith(key) {
      case None     => Some(Set(value))
      case Some(vs) => Some(vs + value) // if vs + value == vs then this will return the original MD I believe
    }
    if newElems eq this.elems then this
    else new MultiDict(newElems)

That is how incl in the standard scala library for HashSet works.

@SethTisue
Copy link
Member

@scala/collections ?

@bblfish
Copy link
Author

bblfish commented Jul 13, 2023

yes, in scala collections for example IntMap line 390, but I had found another example in a more widely used collection yesterday.

@bblfish
Copy link
Author

bblfish commented Jul 14, 2023

I have a mini implementation of MultiDict for my needs in org/w3/banana/quiver/util/MultiDict.scala with tests that pass and two implementations of Graph that use it whose tests also pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants