Unexpected Bound - Why can't I use something like T extends Collection in this Collector?-CodePudding

Hi I have written this custom collector for converting a List of elements into a Multimap:

public class MultiMapCollector implements Collector<Entity,
        Map<String, Collection<String>>, Map<String, Collection<String>>> {

    @Override
    public Supplier<Map<String, Collection<String>>> supplier() {
        return HashMap::new;
    }

    @Override
    public BiConsumer<Map<String, Collection<String>>, Entity> accumulator() {
        return (map, e) -> map.computeIfAbsent(e.getName(),
                k -> new HashSet<>()).add(e.getValue());
    }

    @Override
    public BinaryOperator<Map<String, Collection<String>>> combiner() {
        return (map1, map2) -> {
            map2.keySet().forEach(key -> map1.computeIfAbsent(key, k -> new HashSet<>()).addAll(map2.get(key)));
            return map1;
        };
    }

    @Override
    public Function<Map<String, Collection<String>>, Map<String, Collection<String>>> finisher() {
        return Collections::unmodifiableMap;
    }

    @Override
    public Set<Characteristics> characteristics() {
        return Set.of(Characteristics.UNORDERED);
    }
}

Now I want to be able to pass a List or a Set.

So instead of Map<String, Collection<String>> I want something like Map<String, V extends Collection<String>>

But when I do that I get a vague error saying :

Unexpected Bounds

What am I doing wrong ?

I can't seem to wrap my head around this.

CodePudding user response：

You need to add a generic type parameter (let's call it C) after the class name, not in the implements clause.

Then, add a constructor that takes in a Supplier<C> telling you how to create a C, and store the supplier somewhere. Replace all the occurrences of new HashSet<>() with get calls to the supplier instead.

class MultiMapCollector<C extends Collection<String>> implements Collector<Entity, Map<String, C>, Map<String, C>> {

     private final Supplier<C> supplier;

     public MultiMapCollector(Supplier<C> supplier) {

         this.supplier = supplier;
     }

    @Override
    public Supplier<Map<String, C>> supplier() {
        return HashMap::new;
    }

    @Override
    public BiConsumer<Map<String, C>, Entity> accumulator() {
        return (map, e) -> map.computeIfAbsent(e.getName(),
            k -> supplier.get()).add(e.getValue());
    }

    @Override
    public BinaryOperator<Map<String, C>> combiner() {
        return (map1, map2) -> {
            map2.keySet().forEach(key -> map1.computeIfAbsent(key, k -> supplier.get()).addAll(map2.get(key)));
            return map1;
        };
    }

    @Override
    public Function<Map<String, C>, Map<String, C>> finisher() {
        return Collections::unmodifiableMap;
    }

    @Override
    public Set<Characteristics> characteristics() {
        // now you aren't sure whether the collection is ordered or not
        return Set.of();
    }
}

Now every time you create a MultiMapCollector, you need to pass in a supplier:

new MultiMapCollector<>(HashSet::new)

If you want it to use a default of HashSet when you don't provide anything, add static factory methods and make the constructor private:

 public static MultiMapCollector<Set<String>> get() {
     return new MultiMapCollector<>(HashSet::new);
 }

 public static <C extends Collection<String>> MultiMapCollector<C> with(Supplier<C> supplier) {
     return new MultiMapCollector<>(supplier);
 }

Just like how Collectors.groupingBy is a factory method.

More generally, you can take a full downstream collector instead of just a Supplier<C>, and that is what the 2-parameter overload of Collectors.groupingBy does. groupingBy just handles the grouping, and the downstream collector (the 2nd argument) handles the groups.

Here is a sketch of how you would do this:

class MultiMapCollector<R> implements Collector<Entity, Map<String, R>, Map<String, R>> {

     private final Collector<String, R, R> downstream;

     private MultiMapCollector(Collector<String, R, R> downstream) {

         this.downstream = downstream;
     }

     public static MultiMapCollector<Set<String>> get() {
         return new MultiMapCollector<>(Collectors.toSet());
     }

     public static <R> MultiMapCollector<R> with(Collector<String, R, R> downstream) {
         return new MultiMapCollector<>(downstream);
     }

    @Override
    public Supplier<Map<String, R>> supplier() {
        return HashMap::new;
    }

    @Override
    public BiConsumer<Map<String, R>, Entity> accumulator() {
        return (map, e) -> downstream.accumulator().accept(map.computeIfAbsent(e.getName(),
            k -> downstream.supplier().get()), e.getValue());
    }

    @Override
    public BinaryOperator<Map<String, R>> combiner() {
        return (map1, map2) -> {
            map2.keySet().forEach(key -> 
                downstream.combiner().apply(
                    map1.computeIfAbsent(key, k -> downstream.supplier().get()),
                    map2.get(key)
                )
            );
            return map1;
        };
    }

    @Override
    public Function<Map<String, R>, Map<String, R>> finisher() {
        return Collections::unmodifiableMap;
    }

    @Override
    public Set<Characteristics> characteristics() {
        return Set.of();
    }
}

CodePudding user response：

If you want to allow different implementations of Collection<String> to be used as the values of your maps, then you need to define a type parameter for your MultiMapCollector. You'll also need to modify the code to accept some sort of object, such as a Supplier, that knows how to create instances of the specific collection type.

For example:

import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.function.BiConsumer;
import java.util.function.BinaryOperator;
import java.util.function.Function;
import java.util.function.Supplier;
import java.util.stream.Collector;

public class MultiMapCollector<V extends Collection<String>>
    implements Collector<Entity, Map<String, V>, Map<String, V>> {

  private final Supplier<? extends V> valueSupplier;

  public MultiMapCollector(Supplier<? extends V> valueSupplier) {
    this.valueSupplier = valueSupplier;
  }

  @Override
  public Supplier<Map<String, V>> supplier() {
    return HashMap::new;
  }

  @Override
  public BiConsumer<Map<String, V>, Entity> accumulator() {
    return (map, e) -> map.computeIfAbsent(e.getName(), k -> valueSupplier.get()).add(e.getValue());
  }

  @Override
  public BinaryOperator<Map<String, V>> combiner() {
    return (map1, map2) -> {
      for (var entry : map2.entrySet()) {
        map1.merge(
            entry.getKey(),
            entry.getValue(),
            (curValue, newValue) -> {
              curValue.addAll(newValue);
              return curValue;
            });
      }
      return map1;
    };
  }

  @Override
  public Function<Map<String, V>, Map<String, V>> finisher() {
    return Collections::unmodifiableMap;
  }

  @Override
  public Set<Characteristics> characteristics() {
    return Set.of(Characteristics.UNORDERED);
  }
}

Note MultiMapCollector now has the type parameter <V extends Collection<String>>, and that everywhere else in the code where Collection<String> was used has been replaced with V. And then the constructor accepts a Supplier<? extends V> which is used to create instances of V in the accumulator() consumer.

Also, note I updated your combiner() function to make use of Map#merge(...), though that's not strictly necessary.

By the way, if you want to actually see how Collectors.groupingBy(...) is implemented, then you can always look at the source code.