guava icon indicating copy to clipboard operation
guava copied to clipboard

Add putEntry Method to Multimap

Open Adrodoc opened this issue 3 years ago • 12 comments

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());
}

Adrodoc avatar Jan 20 '22 19:01 Adrodoc

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.

eamonnmcmanus avatar Jan 21 '22 17:01 eamonnmcmanus

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 👍

WojciechZankowski avatar Jan 21 '22 23:01 WojciechZankowski

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.

kevinb9n avatar Mar 28 '22 15:03 kevinb9n

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

ayushdubey755 avatar Aug 03 '22 09:08 ayushdubey755

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.

Adrodoc avatar Aug 06 '22 13:08 Adrodoc

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

bhavrana avatar Dec 26 '22 08:12 bhavrana

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

Adrodoc avatar Dec 29 '22 10:12 Adrodoc

@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 :)

bhavrana avatar Jan 02 '23 08:01 bhavrana

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

anii1827 avatar Feb 08 '23 07:02 anii1827

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.

Adrodoc avatar Feb 08 '23 09:02 Adrodoc

hi @bhavrana, are you still working on this?

anii1827 avatar Feb 08 '23 12:02 anii1827

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.

kevinb9n avatar Feb 08 '23 21:02 kevinb9n