2

I'm trying to add objects from the List<Farm> to Map<Animal, List<Farm>>

public class Farm {
    private String farmName;
    private EnumSet<Animal> animals = EnumSet.noneOf(Animal.class);
    /* ... */
}

Farm f1 = new Farm("Farm A", EnumSet.of(Animal.CAT, Animal.DOG, Animal.DUCK));
Farm f2 = new Farm("Farm B", EnumSet.of(Animal.PIG, Animal.CAT, Animal.HORSE));
Farm f3 = new Farm("Farm C", EnumSet.of(Animal.DUCK));

Task 1: add Objects to List<Farm>

List<Farm> list = new ArrayList<>();
list.add(f1);
list.add(f2);
list.add(f3);

Task 2: Add the objects from list to a map (Key: Animal, Value: List <Farm>) I did this task in this way:

Map<Animal, List<Farm>> map = new HashMap<>();

for(Farm farm: list) {
    for(Animal an: farm.getAnimals()) {
        if(!map.containsKey(an)) {
            List<Farm> new_list = new ArrayList<>();
            new_list.add(farm);
            map.put(an, new_list);
        }else {     
            List<Farm> old_list = map.get(an);
            if(!old_list.contains(farm)) {
                old_list.add(farm);
                    }
            }
        }
    }

Is there a second / more efficient solution? Something like this:

Map<Animal, List<Farm>> map = list.stream().collect(Collectors.groupingBy(Farm::getAnimals)));

This does not work because getAnimals returns an EnumSet<Animal>.

Hans Meier
  • 33
  • 5

2 Answers2

3

You probably want to stay with the loop, but modernize it:

Map<Animal, List<Farm>> map = new EnumMap<>(Animal.class);
for(Farm farm: list)
    for(Animal an: farm.getAnimals())
        map.computeIfAbsent(an, x -> new ArrayList<>()).add(farm);

In your loop, add(farm) redundantly appeared in both branches, as you always add it to the List. Then, computeIfAbsent allows to eliminate the conditional, as it will return an existing value or construct a new value, put and return it. The groupingBy collector also uses this method internally.

Using a Stream operation for the same has the disadvantage that you need a temporary holder for two values, e.g.

Map<Animal, List<Farm>> map = list.stream()
    .flatMap(farm -> farm.getAnimals().stream()
        .map(animal -> new AbstractMap.SimpleImmutableEntry<>(animal, farm)))
    .collect(Collectors.groupingBy(Map.Entry::getKey,
        () -> new EnumMap<>(Animal.class),
        Collectors.mapping(Map.Entry::getValue, Collectors.toList())));
Holger
  • 285,553
  • 42
  • 434
  • 765
0

I started from the opposite side, but I suppose it's helpful

    Map<Animal, List<Farm>> map = Arrays.stream(Animal.values())
            .collect(Collectors.toMap(an -> an, an -> list.stream().filter(f -> f.getAnimals().contains(an)).collect(Collectors.toList())));

There could be empty sets for an animal in the case when none of the farms contains it, but that could be easily filtered

Michel_T.
  • 2,741
  • 5
  • 21
  • 31
  • 1
    That works, but repeatedly iterating over the list is rather inefficient. – Holger Jun 27 '19 at 11:32
  • Well, it works in the same way as the original code: 4 animals are checked for 3 farms. So does any another code on this page. Mine just starts from different side, that made the code shorter and, in my humble opinion, easier for a reader. – Michel_T. Jun 27 '19 at 11:54
  • 1
    It does *not* work like the original code. You are running through the entire list for each `Animal` that has been defined. The original code only runs through the sets of `Animal` values actually associated with `Farm` instances in the list. As a consequence, for `Animal` constants not currently used in any `Farm` in the `list`, your code produces a map entry with an empty list which the original code doesn’t. There are, by the way, at least five constants in `Animal`, but no-one ever said that only these five constants exist. Even with an empty `list`, your code will add all keys to the map. – Holger Jun 27 '19 at 14:26
  • Yes, I know, that's because I've made a short note about empty sets of the animals. And still, I see no problem with that. – Michel_T. Jun 27 '19 at 15:16