1

I am trying to write my own CollectionUtils helper class that other application will use. Here is my first method

    
    public static <T, K, V> 
    Map<K, List<V>> listToMap(List<T> inputList, 
            Function<? super T, ? extends K> keyMapper, 
            Function<? super T, ? extends V> valueMapper)
    {
        Collector c = Collectors.toMap(keyMapper, valueMapper);
        return inputList.stream()
                .collect(c);
        
        
    }
    
    public void test()
    {
        // trying to get a Map<String, List<String>> showing student's name and their activities.
        listToMap(StudentDatabase.getAllStudents(), Student::getName, Student::getActivites);
    }

However, I am getting lots of compilation error that I do not understand how to solve. Can I get some help here? Is there any third party library that already does that (but it has to be using java stream api) I can use so that I do not need to write my own?

I tried above and having compilation issue.

markspace
  • 10,621
  • 3
  • 25
  • 39
  • would you mind provide me the example? I am trying this but still compilation issue:public static R listToMap(List inputList, Function super T, ? extends K> keyMapper, Function super T, ? extends V> valueMapper) { Collector c = Collectors.toMap(keyMapper, valueMapper); return inputList.stream() .collect(c); } – blue mountain Feb 13 '23 at 00:59
  • Why are you using a raw Collector? – shmosel Feb 13 '23 at 01:03
  • @bluemountain There was no reason to add `R` or `A` to the type parameters. You already have all the necessary generic information. Part of the problem is that `c` is declared to be a **raw** `Collector`. You need to parameterize it. My answer goes into more detail. – Slaw Feb 13 '23 at 01:16
  • This whole helper seems pretty pointless. – shmosel Feb 13 '23 at 01:22
  • @shmosel Eh, maybe. If this pattern is used many times throughout the code (or other projects), then I can see making this helper method. – Slaw Feb 13 '23 at 01:35

1 Answers1

2

There's a couple of problems with the current code:

  1. The Collector interface is generic. You should parameterize it like you're doing for all the other generic types in the code. See What is a raw type and why shouldn't we use it? for more information.

  2. You've defined the return type as Map<K, List<V>>. That would seem to indicate you're trying to implement a grouping operation. However, there are three other parts of your code indicating otherwise:

    • You use Collectors#toMap(Function,Function)

    • Your valueMapper maps to V, not List<V>

    • You call listToMap with Student::getActivities as an argument for the valueMapper, and I can only assume that method returns a list of activities (or some other collection).

    So, given all that, you should change the return type to Map<K, V>. That gives the caller full control over the value type of the map, rather than forcing them to use a list. But if you are trying to implement a grouping operation, and you always want the value type to be a List<V>, then consider using Collectors#groupingBy(Function,Collector) instead.

Fixing those two things will give you something like:

public static <T, K, V> Map<K, V> listToMap(
        List<T> list,
        Function<? super T, ? extends K> keyMapper,
        Function<? super T, ? extends V> valueMapper) {
    Collector<T, ?, Map<K, V>> collector = Collectors.toMap(keyMapper, valueMapper);
    return list.stream().collect(collector);
}

And here's a minimal example using the above:

import java.util.List;
import java.util.Map;
import java.util.function.Function;
import java.util.stream.Collector;
import java.util.stream.Collectors;

public class Main {

    public record Student(String name, List<String> activities) {}

    public static <T, K, V> Map<K, V> listToMap(
            List<T> list, 
            Function<? super T, ? extends K> keyMapper, 
            Function<? super T, ? extends V> valueMapper) {
        Collector<T, ?, Map<K, V>> collector = Collectors.toMap(keyMapper, valueMapper);
        return list.stream().collect(collector);
    }

    public static void main(String[] args) {
        List<Student> students = List.of(
            new Student("John", List.of("Piano", "Running")),
            new Student("Jane", List.of("Soccer", "Video Games")),
            new Student("Bob", List.of("Snowboarding"))
        );
        Map<String, List<String>> map = listToMap(students, Student::name, Student::activities);
        System.out.println(map);
    }
}

Output:

{Bob=[Snowboarding], John=[Piano, Running], Jane=[Soccer, Video Games]}
Slaw
  • 37,820
  • 8
  • 53
  • 80
  • I think we are getting close. Now we have the listToMap method fixed. However, I am getting issue on this line when I am trying to invoke that method, it is complaining here Student::getName, Student::getActivites saying "The type Student does not define getName(T) that is applicable here" and "The type Student does not define getActivites(T) that is applicable here" – blue mountain Feb 13 '23 at 01:25
  • @bluemountain I added a minimal example using the `listToMap` method. Note I made `Student` a record, so the "getters" are named e.g., `name()` instead of `getName()`, but that's mostly immaterial. You can see it gives what you want. If you're still having issues, then please describe the error you're getting. – Slaw Feb 13 '23 at 01:32
  • One should also consider to handle the case if the input happens to produce duplicate keys (i.e. if there are two equal names, for the given example ) the above will throw `java.lang.IllegalStateException: Duplicate key` – Eritrean Feb 13 '23 at 01:35
  • Excellent. it worked and all the errors are gone now. Thanks so much for your help. – blue mountain Feb 13 '23 at 01:39
  • 2
    @Eritrean That's true. Though the fix for that is to use the `toMap` overload that accepts a `BinaryOperator` (the "merge function"). Unless there is a way the OP wants to merge duplicate keys in all cases, that means adding a `BinaryOperator` argument to the `listToMap` method (possibly via an overload). And at that point, I'm of the opinion that creating this helper method loses any benefits it may have been providing. It'd be better to just stream-and-collect "directly", given the `Collectors` class is already essentially a utility class (i.e., nothing but helper methods). – Slaw Feb 13 '23 at 01:39
  • 1
    I don’t think that this utility class had a benefit in the first place. Being able to write `listToMap(StudentDatabase.getAllStudents(), Student::getName, Student::getActivites)` instead of `StudentDatabase.getAllStudents().stream() .collect(toMap(Student::getName, Student::getActivites))` does not look like an improvement at all. – Holger Feb 13 '23 at 10:02
  • @Holger The only benefit I see is that the `listToMap` name possibly makes it clearer what is happening at first glance, but that's really up to the person/team writing the code. Though personally, I would not create this utility class either. – Slaw Feb 13 '23 at 17:10
  • 1
    The only additional information, the `listToMap` name provides, is that `getAllStudents()` returns a `List`, as the “`toMap`” is contained in the stream operation as well. But that’s the information I don’t care about, as the whole operation would also work if `getAllStudents()` returned a different `Collection` type, like a `Set`, or anything else that has an appropriate `stream()` method. Granted, the stream operation has slightly more syntactic elements, on the other hand, those elements are easier to read if you’re used to left-to-right writing systems, in contrast to the nested invocation – Holger Feb 13 '23 at 17:17
  • @Holger The difference is the `toMap` is _first_ rather than _last_. That may matter to someone regarding readability. And the OP could change it to `Collection` (or even `Iterable`, which would add more justification to the helper method). Though, again, I think it's opinion-based whether or not to create the above helper. The cost is about 10-30 minutes of time (including unit tests), so if the developer/team thinks it's useful, I say let them. – Slaw Feb 13 '23 at 19:05