6

Model:

public class AgencyMapping {
    private Integer agencyId;
    private String scoreKey;
}

public class AgencyInfo {
    private Integer agencyId;
    private Set<String> scoreKeys;
}

My code:

List<AgencyMapping> agencyMappings;
 Map<Integer, AgencyInfo> agencyInfoByAgencyId = agencyMappings.stream()
            .collect(groupingBy(AgencyMapping::getAgencyId,
                    collectingAndThen(toSet(), e -> e.stream().map(AgencyMapping::getScoreKey).collect(toSet()))))
            .entrySet().stream().map(e -> new AgencyInfo(e.getKey(), e.getValue()))
            .collect(Collectors.toMap(AgencyInfo::getAgencyId, identity()));

Is there a way to get the same result and use more simpler code and faster?

Ousmane D.
  • 54,915
  • 8
  • 91
  • 126
Jack
  • 119
  • 1
  • 6

2 Answers2

4

You can simplify the call to collectingAndThen(toSet(), e -> e.stream().map(AgencyMapping::getScoreKey).collect(toSet())))) with a call to mapping(AgencyMapping::getScoreKey, toSet()).

Map<Integer, AgencyInfo> resultSet = agencyMappings.stream()
                .collect(groupingBy(AgencyMapping::getAgencyId,
                        mapping(AgencyMapping::getScoreKey, toSet())))
                .entrySet()
                .stream()
                .map(e -> new AgencyInfo(e.getKey(), e.getValue()))
                .collect(toMap(AgencyInfo::getAgencyId, identity()));

A different way to see it using a toMap collector:

Map<Integer, AgencyInfo> resultSet = agencyMappings.stream()
                .collect(toMap(AgencyMapping::getAgencyId, // key extractor
                        e -> new HashSet<>(singleton(e.getScoreKey())), // value extractor
                        (left, right) -> { // a merge function, used to resolve collisions between values associated with the same key
                            left.addAll(right);
                            return left;
                        }))
                .entrySet()
                .stream()
                .map(e -> new AgencyInfo(e.getKey(), e.getValue()))
                .collect(toMap(AgencyInfo::getAgencyId, identity()));

The latter example is arguably more complicated than the former. Nevertheless, your approach is pretty much the way to go apart from using mapping as opposed to collectingAndThen as mentioned above.

Apart from that, I don't see anything else you can simplify with the code shown.

As for faster code, if you're suggesting that your current approach is slow in performance then you may want to read the answers here that speak about when you should consider going parallel.

Ousmane D.
  • 54,915
  • 8
  • 91
  • 126
1

You are collecting to an intermediate map, then streaming the entries of this map to create AgencyInfo instances, which are finally collected to another map.

Instead of all this, you could use Collectors.toMap to collect directly to a map, mapping each AgencyMapping object to the desired AgencyInfo and merging the scoreKeys as needed:

Map<Integer, AgencyInfo> agencyInfoByAgencyId = agencyMappings.stream()
    .collect(Collectors.toMap(
            AgencyMapping::getAgencyId,
            mapping -> new AgencyInfo(
                    mapping.getAgencyId(), 
                    new HashSet<>(Set.of(mapping.getScoreKey()))),
            (left, right) -> {
                left.getScoreKeys().addAll(right.getScoreKeys());
                return left;
            }));

This works by grouping the AgencyMapping elements of the stream by AgencyMapping::getAgencyId, but storing AgencyInfo objects in the map instead. We get these AgencyInfo instances from manually mapping each original AgencyMapping object. Finally, we're merging AgencyInfo instances that are already in the map by means of a merge function that folds left scoreKeys from one AgencyInfo to another.

I'm using Java 9's Set.of to create a singleton set. If you don't have Java 9, you can replace it with Collections.singleton.

Ousmane D.
  • 54,915
  • 8
  • 91
  • 126
fps
  • 33,623
  • 8
  • 55
  • 110
  • more than welcome mate!. your saying of “singletonSet” was understandable though considering there is also “singletonList” and “singletonMap”. It feels inconsistent that they didn’t call it “singletonSet”. – Ousmane D. Mar 26 '18 at 15:47
  • 1
    @Aominè Actually, I remember a question asking exactly about that inconsistency. The answer was something like *by definition, a singleton is a set that consists of only one element* (with a link to wikipedia included). And the guy who had asked responded something like *yeah, I know, but why the incosistency in the name?* and someone responded in a not-very-nice way saying that you don't say a singleton set, because set is implicit in singleton, etc, etc, etc. It was quite fun (for about 20 seconds)... – fps Mar 26 '18 at 15:54