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

Add putEntry Method to Multimap #5886

Open
Adrodoc opened this issue Jan 20, 2022 · 12 comments
Open

Add putEntry Method to Multimap #5886

Adrodoc opened this issue Jan 20, 2022 · 12 comments

Comments

@Adrodoc
Copy link

Adrodoc commented Jan 20, 2022

Problem

When you have a method returning an entry there is currently no good way to insert this entry into a Multimap without using a local variable. This is quite annoying when you have multiple calls returning multiple entries.

Consider the following code example:

public static void main(String[] args) {
  ArrayListMultimap<LocalDate, Double> multimap = ArrayListMultimap.create();

  // Workaround 1: Using code blocks
  {
    Entry<LocalDate, Double> entry = rollDice();
    multimap.put(entry.getKey(), entry.getValue());
  }
  {
    Entry<LocalDate, Double> entry = rollDice();
    multimap.put(entry.getKey(), entry.getValue());
  }

  // Workaround 2: Using different Variable Names
  Entry<LocalDate, Double> entry = rollDice();
  multimap.put(entry.getKey(), entry.getValue());
  Entry<LocalDate, Double> entry2 = rollDice();
  multimap.put(entry2.getKey(), entry2.getValue());

  // Workaround 3: Reusing variable
  entry = rollDice();
  multimap.put(entry.getKey(), entry.getValue());
  entry = rollDice();
  multimap.put(entry.getKey(), entry.getValue());

  // Workaround 4: Using putAll
  multimap.putAll(ImmutableMultimap.copyOf(Arrays.asList(rollDice())));
  multimap.putAll(ImmutableMultimap.copyOf(Arrays.asList(rollDice())));

  // Desired
  multimap.putEntry(rollDice());
  multimap.putEntry(rollDice());
}

public static Entry<LocalDate, Double> rollDice() {
  return new SimpleImmutableEntry<>(LocalDate.now(), Math.random());
}

As you can see the four workarounds are all quite verbose, which can make the code hard to understand, when there is a bit more going on than just a simple call to roleDice().

Proposal

It would be cool to have a default method putEntry on the Multimap interface to allow writing the last version in the above example:

@CanIgnoreReturnValue
default boolean putEntry(Map.Entry<? extends K, ? extends V> entry) {
  return put(entry.getKey(), entry.getValue());
}
@eamonnmcmanus
Copy link
Member

The idea isn't unreasonable. We already have put(Map.Entry) methods in ImmutableSetMultimap.Builder and ImmutableListMultimap.Builder. On the other hand, Map doesn't have a put(Entry) method, though your argument would seem to apply just as well there. I also wasn't able to find any places in Google's giant source base where the case you mention arises. Not to say there aren't any, but in every case I saw with put(someEntry.getKey(), someEntry.getValue()), someEntry needed to exist anyway, for example because it was the variable in a for-each loop or lambda.

If we did add a method, I think it would be called put rather than putEntry. That's what we do in the various immutable map builders.

@WojciechZankowski
Copy link
Contributor

WojciechZankowski commented Jan 21, 2022

If put method would be added then in theory it opens also a discussion for other methods from the interface like:

boolean remove(key, value);
boolean containsEntry(key, value);

where similar concept could be applied. Of course with method's overloading and default methods it doesn't bloat the interface too much. Simply the question is how many things you would like to have there.

If you decide one way or another then I can provide you a PR with the change 👍

@kevinb9n
Copy link
Contributor

I like the idea of addressing these cases consistently (while I'm not expressing an opinion on doing them all or none).

The Multimap API is actually much smaller than Map, so I don't think there's a major bloat issue.

@ayushdubey755
Copy link

HI @Adrodoc is anyone working on this issue , or i can add your proposal solution into respective interface

@Adrodoc
Copy link
Author

Adrodoc commented Aug 6, 2022

HI @Adrodoc is anyone working on this issue , or i can add your proposal solution into respective interface

I am not aware of anyone working on this.

@bhavrana
Copy link

Hi @Adrodoc,
Does this issue still need to be worked on? If yes, I would like to contribute.

@Adrodoc
Copy link
Author

Adrodoc commented Dec 29, 2022

@bhavrana Yes, there does not seem to be anyone working on it and this is still relevant. I would love to see this added.

@bhavrana
Copy link

bhavrana commented Jan 2, 2023

@bhavrana Yes, there does not seem to be anyone working on it and this is still relevant. I would love to see this added.

Thanks @Adrodoc, I will get started with this :)

@anii1827
Copy link

anii1827 commented Feb 8, 2023

hi @Adrodoc
is that issue fixed or still work required?

@Adrodoc
Copy link
Author

Adrodoc commented Feb 8, 2023

hi @Adrodoc is that issue fixed or still work required?

@anii1827 you should ask @bhavrana. I don't know anything that is not written here.

@anii1827
Copy link

anii1827 commented Feb 8, 2023

hi @bhavrana,
are you still working on this?

@kevinb9n
Copy link
Contributor

kevinb9n commented Feb 8, 2023

The comments above from myself and @eamonnmcmanus are from Guava team members, and although they are vaguely non-negative, the fact still remains that Guava is not really in a "growth mode" right now, and it is not clear whether we would decide to add this method. Writing any code would be premature. And sadly, even the activity of deciding whether to add it or not is not a high priority for us right now. For the most part, we think Guava is okay as it is.

Please read over https://github.com/google/guava/wiki/HowToContribute#code-contributions carefully; it explains some of this, and better ways to help the project, which would be appreciated if we're lucky enough to get them.

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

No branches or pull requests

7 participants